diff --git a/policy/pa.go b/policy/pa.go index 7e632db51..ce7857a7d 100644 --- a/policy/pa.go +++ b/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 -// breaking queries. +// domain names. // -// We place several criteria on domains we are willing to issue for: -// - MUST contain only bytes in the DNS hostname character set +// It checks the criteria checked by `WellFormedDomainNames`, and additionally checks +// 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 -// the rejected domains will be returned. -func (pa *AuthorityImpl) WillingToIssue(domains []string) error { +// If multiple domains are invalid, the error will contain suberrors specific to +// each domain. +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 { - // 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) + err := ValidDomain(domain) if err != nil { - appendSubError(domain, err) - continue + subErrors = append(subErrors, subError(domain, err)) } } + 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.") } diff --git a/policy/pa_test.go b/policy/pa_test.go index f754c4358..e2f4fdc9d 100644 --- a/policy/pa_test.go +++ b/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{ - `co.uk`, - `foo.bd`, + // Test syntax errors + for _, tc := range testCases { + 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{ - "perfectly-fine.com", // fine - "letsdecrypt.org", // banned - "ok.*.this.is.a.*.weird.one.com", // malformed - "also-perfectly-fine.com", // fine - } + // Test multiple malformed domains and one banned domain; only the malformed ones will generate errors + err = pa.WillingToIssue([]string{ + "perfectly-fine.com", // fine + "letsdecrypt_org", // malformed + "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.AssertErrorWraps(t, err, &berr) - for _, err := range berr.SubErrors { - fmt.Println(err) - } - 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.)") - - subErrMap := make(map[string]berrors.SubBoulderError, len(berr.SubErrors)) - - for _, subErr := range berr.SubErrors { - subErrMap[subErr.Identifier.Value] = subErr - } - - subErrA, foundA := subErrMap["letsdecrypt.org"] - subErrB, foundB := subErrMap["ok.*.this.is.a.*.weird.one.com"] - test.AssertEquals(t, foundA, true) - test.AssertEquals(t, foundB, true) - - test.AssertEquals(t, subErrA.Type, berrors.RejectedIdentifier) - test.AssertEquals(t, subErrB.Type, berrors.Malformed) + test.AssertDeepEquals(t, err, + &berrors.BoulderError{ + Type: berrors.RejectedIdentifier, + 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{ + { + BoulderError: &berrors.BoulderError{ + Type: berrors.RejectedIdentifier, + Detail: "The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy", + }, + Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "letsdecrypt.org"}, + }, + { + BoulderError: &berrors.BoulderError{ + Type: berrors.RejectedIdentifier, + Detail: "The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy", + }, + Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"}, + }, + }, + }) // Test willing to issue with only *one* bad identifier. err = pa.WillingToIssue([]string{"letsdecrypt.org"}) - // It should error - test.AssertError(t, err, "Expected err from WillingToIssueWildcards") - - test.AssertErrorWraps(t, err, &berr) - // 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") + test.AssertDeepEquals(t, err, + &berrors.BoulderError{ + Type: berrors.RejectedIdentifier, + Detail: "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) { diff --git a/ratelimits/bucket.go b/ratelimits/bucket.go index 1be77b607..f4698770c 100644 --- a/ratelimits/bucket.go +++ b/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) diff --git a/ratelimits/names.go b/ratelimits/names.go index f7a712b1f..fdfd8e81e 100644 --- a/ratelimits/names.go +++ b/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 { - err := policy.ValidDomain(domain) - if err != nil { - return fmt.Errorf( - "invalid domain, %q must be formatted 'fqdnSet': %w", id, err) - } - } - return nil + return policy.WellFormedDomainNames(domains) } func validateIdForName(name Name, id string) error { diff --git a/test/integration/errors_test.go b/test/integration/errors_test.go index 1336c80b1..0c71bdb72 100644 --- a/test/integration/errors_test.go +++ b/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 diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 903017507..756cef2f2 100644 --- a/wfe2/wfe.go +++ b/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 diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 4634a772f..6c1253196 100644 --- a/wfe2/wfe_test.go +++ b/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, "{}"),