PA: Improve wildcard exact blocklist implementation#7218
Conversation
727ea02 to
0451789
Compare
0451789 to
549a101
Compare
549a101 to
ffe860d
Compare
jsha
left a comment
There was a problem hiding this comment.
Thanks for this refactoring. To make sure I understand it correctly:
Previously, willingToIssueWildcard, called willingToIssue, which called checkHostLists. For subdomain matching against the blocklist, it would be fine (and correct) to pass *.example.com to checkHostLists. But because willingToIssue rejected wildcards, we have to pretend the hostname was a non-wildcard (replace *. with x.).
This PR fixes that by hoisting up the checkHostLists call so that it happens in both wildcard and non-wildcard branches. Now instead of calling checkHostLists("x.example.com"), this code will call checkHostLists("*.example.com"). In other words, the code doesn't have to pretend a wildcard domain name is a normal domain name, because it doesn't have to jam wildcard domains through a function that is meant to validate non-wildcard domains. But it does call ValidDomain("example.com") to syntactically validate the non-wildcard portion.
aarongable
left a comment
There was a problem hiding this comment.
LGTM with one question (see below) and one top-level comment: this exposes two top-level methods in the policy package: ValidDomain and ValidWildcardDomain. Part of the purpose of this PR is to make #7200 simpler and cleaner. Doesn't having these two separate methods mean that the ratelimit validation methods in the RA will all have to look like
if strings.Count(domain, "*") > 0 {
err := ValidWildcardDomain(domain)
} else {
err := ValidDomain(domain)? Is that how we want to structure this, or would we prefer for policy.ValidDomain to operate similarly to PA.WillingToIssue, and do the wildcard/non-wildcard differentitation within itself?
I was just considering making the same change this morning. However, I realized that use-cases like |
|
Conceptually it seems like we want two functions:
|
This allows us to give a user-meaningful error about malformed names early on, instead of propagating internal errors from the new rate limiting system. This moves the well-formedness logic from `WillingToIssue` into a new function `WellFormedDomainNames`, which calls `ValidDomain` on each name and combines the errors into suberrors if there is more than one. `WillingToIssue` now calls `WellFormedDomainNames` to keep the existing behavior. Additionally, WFE calls `WellFormedDomainNames` before checking rate limits. This creates a slight behavior change: If an order contains both malformed domain names and wellformed but blocked domain names, suberrors will only be generated for the malformed domain names. This is reflected in the changes to `TestWillingToIssue_Wildcard`. Adds a WFE test case for receiving malformed identifiers in a new-order request. Follows up on #3323 and #7218 Fixes #7526 Some small incidental fixes: - checkWildcardHostList was checking `pa.blocklist` for `nil` before accessing `pa.wildcardExactBlocklist`. Fix that. - move table test for WillingToIssue into a new test case for WellFormedDomainNames - move two standalone test cases into the big table test
Revamp WillingToIssueWildcards to WillingToIssue. Remove the need for identifier.ACMEIdentifiers in the WillingToIssue(Wildcards) method. Previously, before invoking this method, a slice of identifiers was created by looping over each dnsName. However, these identifiers were solely used in error messages.
Segment the validation process into distinct parts for domain validation, wildcard validation, and exact blocklist checks. This approach eliminates the necessity of substituting *. with x. in wildcard domains.
Introduce a new helper, ValidDomain. It checks that a domain is valid and that it doesn't contain any invalid wildcard characters. Functionality from the previous ValidDomain is preserved in ValidNonWildcardDomain.
Fixes #3323