wfe: check well-formedness of requested names early (#7530)

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

policy/pa.go

@@ -190,7 +190,7 @@ var (
 	errWildcardNotSupported = berrors.MalformedError("Wildcard domain names are not supported")
 )
 
-// ValidNonWildcardDomain checks that a domain isn't:
+// validNonWildcardDomain checks that a domain isn't:
 //   - empty
 //   - prefixed with the wildcard label `*.`
 //   - made of invalid DNS characters
@@ -203,7 +203,7 @@ var (
 //   - exactly equal to an IANA registered TLD
 //
 // It does NOT ensure that the domain is absent from any PA blocked lists.
-func ValidNonWildcardDomain(domain string) error {
+func validNonWildcardDomain(domain string) error {
 	if domain == "" {
 		return errEmptyName
 	}
@@ -296,7 +296,7 @@ func ValidNonWildcardDomain(domain string) error {
 // from any PA blocked lists.
 func ValidDomain(domain string) error {
 	if strings.Count(domain, "*") <= 0 {
-		return ValidNonWildcardDomain(domain)
+		return validNonWildcardDomain(domain)
 	}
 
 	// Names containing more than one wildcard are invalid.
@@ -323,7 +323,7 @@ func ValidDomain(domain string) error {
 	if baseDomain == icannTLD {
 		return errICANNTLDWildcard
 	}
-	return ValidNonWildcardDomain(baseDomain)
+	return validNonWildcardDomain(baseDomain)
 }
 
 // forbiddenMailDomains is a map of domain names we do not allow after the
@@ -351,7 +351,7 @@ func ValidEmail(address string) error {
 	}
 	splitEmail := strings.SplitN(email.Address, "@", -1)
 	domain := strings.ToLower(splitEmail[len(splitEmail)-1])
-	err = ValidNonWildcardDomain(domain)
+	err = validNonWildcardDomain(domain)
 	if err != nil {
 		return berrors.InvalidEmailError(
 			"contact email %q has invalid domain : %s",
@@ -365,12 +365,69 @@ func ValidEmail(address string) error {
 	return nil
 }
 
+// subError returns an appropriately typed error based on the input error
+func subError(name string, err error) berrors.SubBoulderError {
+	var bErr *berrors.BoulderError
+	if errors.As(err, &bErr) {
+		return berrors.SubBoulderError{
+			Identifier:   identifier.DNSIdentifier(name),
+			BoulderError: bErr,
+		}
+	} else {
+		return berrors.SubBoulderError{
+			Identifier: identifier.DNSIdentifier(name),
+			BoulderError: &berrors.BoulderError{
+				Type:   berrors.RejectedIdentifier,
+				Detail: err.Error(),
+			},
+		}
+	}
+}
+
 // WillingToIssue determines whether the CA is willing to issue for the provided
-// domains. It expects each domain to be lowercase to prevent mismatched cases
+// domain names.
-// breaking queries.
 //
-// We place several criteria on domains we are willing to issue for:
+// It checks the criteria checked by `WellFormedDomainNames`, and additionally checks
-//   - MUST contain only bytes in the DNS hostname character set
+// whether any domain is on a blocklist.
+//
+// If multiple domains are invalid, the error will contain suberrors specific to
+// each domain.
+//
+// Precondition: all input domain names must be in lowercase.
+func (pa *AuthorityImpl) WillingToIssue(domains []string) error {
+	err := WellFormedDomainNames(domains)
+	if err != nil {
+		return err
+	}
+
+	var subErrors []berrors.SubBoulderError
+	for _, domain := range domains {
+		if strings.Count(domain, "*") > 0 {
+			// The base domain is the wildcard request with the `*.` prefix removed
+			baseDomain := strings.TrimPrefix(domain, "*.")
+
+			// The base domain can't be in the wildcard exact blocklist
+			err = pa.checkWildcardHostList(baseDomain)
+			if err != nil {
+				subErrors = append(subErrors, subError(domain, err))
+				continue
+			}
+		}
+
+		// For both wildcard and non-wildcard domains, check whether any parent domain
+		// name is on the regular blocklist.
+		err := pa.checkHostLists(domain)
+		if err != nil {
+			subErrors = append(subErrors, subError(domain, err))
+			continue
+		}
+	}
+	return combineSubErrors(subErrors)
+}
+
+// WellFormedDomainNames returns an error if any of the provided domains do not meet these criteria:
+//
+//   - MUST contains only lowercase characters, numbers, hyphens, and dots
 //   - MUST NOT have more than maxLabels labels
 //   - MUST follow the DNS hostname syntax rules in RFC 1035 and RFC 2181
 //
@@ -387,65 +444,21 @@ func ValidEmail(address string) error {
 //   - That the wildcard character is the leftmost label
 //   - That the wildcard label is not immediately adjacent to a top level ICANN
 //     TLD
-//   - That the wildcard wouldn't cover an exact blocklist entry (e.g. an exact
-//     blocklist entry for "foo.example.com" should prevent issuance for
-//     "*.example.com")
 //
-// If any of the domains are not valid then an error with suberrors specific to
+// If multiple domains are invalid, the error will contain suberrors specific to
-// the rejected domains will be returned.
+// each domain.
-func (pa *AuthorityImpl) WillingToIssue(domains []string) error {
+func WellFormedDomainNames(domains []string) error {
 	var subErrors []berrors.SubBoulderError
-	appendSubError := func(name string, err error) {
-		var bErr *berrors.BoulderError
-		if errors.As(err, &bErr) {
-			subErrors = append(subErrors, berrors.SubBoulderError{
-				Identifier:   identifier.DNSIdentifier(name),
-				BoulderError: bErr,
-			})
-		} else {
-			subErrors = append(subErrors, berrors.SubBoulderError{
-				Identifier: identifier.DNSIdentifier(name),
-				BoulderError: &berrors.BoulderError{
-					Type:   berrors.RejectedIdentifier,
-					Detail: err.Error(),
-				},
-			})
-		}
-	}
-
 	for _, domain := range domains {
-		if strings.Count(domain, "*") > 0 {
+		err := ValidDomain(domain)
-			// Domain contains a wildcard, check that it is valid.
-			err := ValidDomain(domain)
-			if err != nil {
-				appendSubError(domain, err)
-				continue
-			}
-
-			// The base domain is the wildcard request with the `*.` prefix removed
-			baseDomain := strings.TrimPrefix(domain, "*.")
-
-			// The base domain can't be in the wildcard exact blocklist
-			err = pa.checkWildcardHostList(baseDomain)
-			if err != nil {
-				appendSubError(domain, err)
-				continue
-			}
-		} else {
-			// Validate that the domain is well-formed.
-			err := ValidNonWildcardDomain(domain)
-			if err != nil {
-				appendSubError(domain, err)
-				continue
-			}
-		}
-		// Require no match against hostname block lists
-		err := pa.checkHostLists(domain)
 		if err != nil {
-			appendSubError(domain, err)
+			subErrors = append(subErrors, subError(domain, err))
-			continue
 		}
 	}
+	return combineSubErrors(subErrors)
+}
+
+func combineSubErrors(subErrors []berrors.SubBoulderError) error {
 	if len(subErrors) > 0 {
 		// If there was only one error, then use it as the top level error that is
 		// returned.
@@ -478,7 +491,7 @@ func (pa *AuthorityImpl) checkWildcardHostList(domain string) error {
 	pa.blocklistMu.RLock()
 	defer pa.blocklistMu.RUnlock()
 
-	if pa.blocklist == nil {
+	if pa.wildcardExactBlocklist == nil {
 		return fmt.Errorf("Hostname policy not yet loaded.")
 	}
 

policy/pa_test.go

@@ -28,7 +28,7 @@ func paImpl(t *testing.T) *AuthorityImpl {
 	return pa
 }
 
-func TestWillingToIssue(t *testing.T) {
+func TestWellFormedDomainNames(t *testing.T) {
 	testCases := []struct {
 		domain string
 		err    error
@@ -102,13 +102,25 @@ func TestWillingToIssue(t *testing.T) {
 		{`bq---abwhky3f6fxq.jakacomo.com`, errInvalidRLDH},
 		// Three hyphens starting at second char of first label.
 		{`h---test.hk2yz.org`, errInvalidRLDH},
+		{`co.uk`, errICANNTLD},
+		{`foo.bd`, errICANNTLD},
 	}
 
-	shouldBeTLDError := []string{
+	// Test syntax errors
-		`co.uk`,
+	for _, tc := range testCases {
-		`foo.bd`,
+		err := WellFormedDomainNames([]string{tc.domain})
+		if tc.err == nil {
+			test.AssertNil(t, err, fmt.Sprintf("Unexpected error for domain %q, got %s", tc.domain, err))
+		} else {
+			test.AssertError(t, err, fmt.Sprintf("Expected error for domain %q, but got none", tc.domain))
+			var berr *berrors.BoulderError
+			test.AssertErrorWraps(t, err, &berr)
+			test.AssertContains(t, berr.Error(), tc.err.Error())
+		}
 	}
+}
 
+func TestWillingToIssue(t *testing.T) {
 	shouldBeBlocked := []string{
 		`highvalue.website1.org`,
 		`website2.co.uk`,
@@ -162,19 +174,6 @@ func TestWillingToIssue(t *testing.T) {
 	err = pa.LoadHostnamePolicyFile(yamlPolicyFile.Name())
 	test.AssertNotError(t, err, "Couldn't load rules")
 
-	// Test syntax errors
-	for _, tc := range testCases {
-		err := pa.WillingToIssue([]string{tc.domain})
-		if tc.err == nil {
-			test.AssertNil(t, err, fmt.Sprintf("Unexpected error for domain %q, got %s", tc.domain, err))
-		} else {
-			test.AssertError(t, err, fmt.Sprintf("Expected error for domain %q, but got none", tc.domain))
-			var berr *berrors.BoulderError
-			test.AssertErrorWraps(t, err, &berr)
-			test.AssertContains(t, berr.Error(), tc.err.Error())
-		}
-	}
-
 	// Invalid encoding
 	err = pa.WillingToIssue([]string{"www.xn--m.com"})
 	test.AssertError(t, err, "WillingToIssue didn't fail on a malformed IDN")
@@ -186,15 +185,6 @@ func TestWillingToIssue(t *testing.T) {
 	test.AssertNotError(t, err, "WillingToIssue failed on a properly formed domain with IDN TLD")
 	features.Reset()
 
-	// Test domains that are equal to public suffixes
-	for _, domain := range shouldBeTLDError {
-		err := pa.WillingToIssue([]string{domain})
-		test.AssertError(t, err, "domain was not correctly forbidden")
-		var berr *berrors.BoulderError
-		test.AssertErrorWraps(t, err, &berr)
-		test.AssertContains(t, berr.Detail, errICANNTLD.Error())
-	}
-
 	// Test expected blocked domains
 	for _, domain := range shouldBeBlocked {
 		err := pa.WillingToIssue([]string{domain})
@@ -305,11 +295,12 @@ func TestWillingToIssue_Wildcards(t *testing.T) {
 	}
 }
 
-// TestWillingToIssueWildcards tests that more than one rejected identifier
+// TestWillingToIssue_SubErrors tests that more than one rejected identifier
 // results in an error with suberrors.
-func TestWillingToIssue_Wildcard(t *testing.T) {
+func TestWillingToIssue_SubErrors(t *testing.T) {
 	banned := []string{
 		"letsdecrypt.org",
+		"example.com",
 	}
 	pa := paImpl(t)
 
@@ -325,47 +316,74 @@ func TestWillingToIssue_Wildcard(t *testing.T) {
 	err = pa.LoadHostnamePolicyFile(f.Name())
 	test.AssertNotError(t, err, "Couldn't load policy contents from file")
 
-	domains := []string{
+	// Test multiple malformed domains and one banned domain; only the malformed ones will generate errors
-		"perfectly-fine.com",             // fine
+	err = pa.WillingToIssue([]string{
-		"letsdecrypt.org",                // banned
+		"perfectly-fine.com",      // fine
-		"ok.*.this.is.a.*.weird.one.com", // malformed
+		"letsdecrypt_org",         // malformed
-		"also-perfectly-fine.com",        // fine
+		"example.comm",            // malformed
-	}
+		"letsdecrypt.org",         // banned
+		"also-perfectly-fine.com", // fine
+	})
+	test.AssertDeepEquals(t, err,
+		&berrors.BoulderError{
+			Type:   berrors.RejectedIdentifier,
+			Detail: "Cannot issue for \"letsdecrypt_org\": Domain name contains an invalid character (and 1 more problems. Refer to sub-problems for more information.)",
+			SubErrors: []berrors.SubBoulderError{
+				{
+					BoulderError: &berrors.BoulderError{
+						Type:   berrors.Malformed,
+						Detail: "Domain name contains an invalid character",
+					},
+					Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "letsdecrypt_org"},
+				},
+				{
+					BoulderError: &berrors.BoulderError{
+						Type:   berrors.Malformed,
+						Detail: "Domain name does not end with a valid public suffix (TLD)",
+					},
+					Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.comm"},
+				},
+			},
+		})
 
-	err = pa.WillingToIssue(domains)
+	// Test multiple banned domains.
+	err = pa.WillingToIssue([]string{
+		"perfectly-fine.com",      // fine
+		"letsdecrypt.org",         // banned
+		"example.com",             // banned
+		"also-perfectly-fine.com", // fine
+	})
 	test.AssertError(t, err, "Expected err from WillingToIssueWildcards")
 
-	var berr *berrors.BoulderError
+	test.AssertDeepEquals(t, err,
-	test.AssertErrorWraps(t, err, &berr)
+		&berrors.BoulderError{
-	for _, err := range berr.SubErrors {
+			Type:   berrors.RejectedIdentifier,
-		fmt.Println(err)
+			Detail: "Cannot issue for \"letsdecrypt.org\": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy (and 1 more problems. Refer to sub-problems for more information.)",
-	}
+			SubErrors: []berrors.SubBoulderError{
-	test.AssertEquals(t, len(berr.SubErrors), 2)
+				{
-	test.AssertEquals(t, berr.Error(), "Cannot issue for \"letsdecrypt.org\": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy (and 1 more problems. Refer to sub-problems for more information.)")
+					BoulderError: &berrors.BoulderError{
-
+						Type:   berrors.RejectedIdentifier,
-	subErrMap := make(map[string]berrors.SubBoulderError, len(berr.SubErrors))
+						Detail: "The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy",
-
+					},
-	for _, subErr := range berr.SubErrors {
+					Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "letsdecrypt.org"},
-		subErrMap[subErr.Identifier.Value] = subErr
+				},
-	}
+				{
-
+					BoulderError: &berrors.BoulderError{
-	subErrA, foundA := subErrMap["letsdecrypt.org"]
+						Type:   berrors.RejectedIdentifier,
-	subErrB, foundB := subErrMap["ok.*.this.is.a.*.weird.one.com"]
+						Detail: "The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy",
-	test.AssertEquals(t, foundA, true)
+					},
-	test.AssertEquals(t, foundB, true)
+					Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"},
-
+				},
-	test.AssertEquals(t, subErrA.Type, berrors.RejectedIdentifier)
+			},
-	test.AssertEquals(t, subErrB.Type, berrors.Malformed)
+		})
 
 	// Test willing to issue with only *one* bad identifier.
 	err = pa.WillingToIssue([]string{"letsdecrypt.org"})
-	// It should error
+	test.AssertDeepEquals(t, err,
-	test.AssertError(t, err, "Expected err from WillingToIssueWildcards")
+		&berrors.BoulderError{
-
+			Type:   berrors.RejectedIdentifier,
-	test.AssertErrorWraps(t, err, &berr)
+			Detail: "Cannot issue for \"letsdecrypt.org\": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy",
-	// There should be *no* suberrors because there was only one error overall.
+		})
-	test.AssertEquals(t, len(berr.SubErrors), 0)
-	test.AssertEquals(t, berr.Error(), "Cannot issue for \"letsdecrypt.org\": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy")
 }
 
 func TestChallengesFor(t *testing.T) {

ratelimits/bucket.go

@@ -239,6 +239,9 @@ func (builder *TransactionBuilder) OrdersPerAccountTransaction(regId int64) (Tra
 // of Transactions for the provided order domain names. An error is returned if
 // any of the order domain names are invalid. This method should be used for
 // checking capacity, before allowing more authorizations to be created.
+//
+// Precondition: orderDomains must all pass policy.WellFormedDomainNames.
+// Precondition: len(orderDomains) < maxNames.
 func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId int64, orderDomains []string, maxNames int) ([]Transaction, error) {
 	if len(orderDomains) > maxNames {
 		return nil, fmt.Errorf("order contains more than %d DNS names", maxNames)
@@ -318,6 +321,9 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendO
 //
 // When a CertificatesPerDomainPerAccount override is not configured, a check-
 // and-spend Transaction is returned for each per domain bucket.
+//
+// Precondition: orderDomains must all pass policy.WellFormedDomainNames.
+// Precondition: len(orderDomains) < maxNames.
 func (builder *TransactionBuilder) CertificatesPerDomainTransactions(regId int64, orderDomains []string, maxNames int) ([]Transaction, error) {
 	if len(orderDomains) > maxNames {
 		return nil, fmt.Errorf("order contains more than %d DNS names", maxNames)

ratelimits/names.go

@@ -187,14 +187,7 @@ func validateFQDNSet(id string) error {
 		return fmt.Errorf(
 			"invalid fqdnSet, %q must be formatted 'fqdnSet'", id)
 	}
-	for _, domain := range domains {
+	return policy.WellFormedDomainNames(domains)
-		err := policy.ValidDomain(domain)
-		if err != nil {
-			return fmt.Errorf(
-				"invalid domain, %q must be formatted 'fqdnSet': %w", id, err)
-		}
-	}
-	return nil
 }
 
 func validateIdForName(name Name, id string) error {

test/integration/errors_test.go

@@ -28,7 +28,7 @@ func TestTooBigOrderError(t *testing.T) {
 	var prob acme.Problem
 	test.AssertErrorWraps(t, err, &prob)
 	test.AssertEquals(t, prob.Type, "urn:ietf:params:acme:error:malformed")
-	test.AssertEquals(t, prob.Detail, "Error creating new order :: Order cannot contain more than 100 DNS names")
+	test.AssertEquals(t, prob.Detail, "Order cannot contain more than 100 DNS names")
 }
 
 // TestAccountEmailError tests that registering a new account, or updating an

wfe2/wfe.go

@@ -29,6 +29,7 @@ import (
 	"github.com/letsencrypt/boulder/features"
 	"github.com/letsencrypt/boulder/goodkey"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
+	"github.com/letsencrypt/boulder/policy"
 	"github.com/letsencrypt/boulder/ratelimits"
 
 	// 'grpc/noncebalancer' is imported for its init function.
@@ -2044,6 +2045,11 @@ func (wfe *WebFrontEndImpl) orderToOrderJSON(request *http.Request, order *corep
 	return respObj
 }
 
+// newNewOrderLimitTransactions constructs a set of rate limit transactions to
+// evaluate for a new-order request.
+//
+// Precondition: names must be a list of DNS names that all pass
+// policy.WellFormedDomainNames.
 func (wfe *WebFrontEndImpl) newNewOrderLimitTransactions(regId int64, names []string) []ratelimits.Transaction {
 	if wfe.limiter == nil && wfe.txnBuilder == nil {
 		// Limiter is disabled.
@@ -2331,6 +2337,16 @@ func (wfe *WebFrontEndImpl) NewOrder(
 		names[i] = ident.Value
 	}
 
+	err = policy.WellFormedDomainNames(names)
+	if err != nil {
+		wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Invalid identifiers requested"), nil)
+		return
+	}
+	if len(names) > wfe.maxNames {
+		wfe.sendError(response, logEvent, probs.Malformed("Order cannot contain more than %d DNS names", wfe.maxNames), nil)
+		return
+	}
+
 	logEvent.DNSNames = names
 
 	var replaces string

wfe2/wfe_test.go

@@ -2539,6 +2539,11 @@ func TestNewOrder(t *testing.T) {
 			Request:      signAndPost(signer, targetPath, signedURL, `{"identifiers":[{"type":"dns","value":""}]}`),
 			ExpectedBody: `{"type":"` + probs.ErrorNS + `malformed","detail":"NewOrder request included empty domain name","status":400}`,
 		},
+		{
+			Name:         "POST, invalid domain name identifier",
+			Request:      signAndPost(signer, targetPath, signedURL, `{"identifiers":[{"type":"dns","value":"example.invalid"}]}`),
+			ExpectedBody: `{"type":"` + probs.ErrorNS + `rejectedIdentifier","detail":"Invalid identifiers requested :: Cannot issue for \"example.invalid\": Domain name does not end with a valid public suffix (TLD)","status":400}`,
+		},
 		{
 			Name:         "POST, no identifiers in payload",
 			Request:      signAndPost(signer, targetPath, signedURL, "{}"),