diff --git a/ra/ra.go b/ra/ra.go index 390e52ac6..7c2a59b23 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -696,7 +696,10 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA( authzs map[string]*core.Authorization, regID int64, now time.Time) error { - var badNames, recheckNames []string + // badNames contains the names that were unauthorized + var badNames []string + // recheckNames is a list of names that must have their CAA records rechecked + var recheckNames []string // Per Baseline Requirements, CAA must be checked within 8 hours of issuance. // CAA is checked when an authorization is validated, so as long as that was // less than 8 hours ago, we're fine. If it was more than 8 hours ago @@ -715,6 +718,7 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA( } else if authz.Expires.Before(now) { badNames = append(badNames, name) } else if authz.Expires.Before(caaRecheckTime) { + // Ensure that CAA is rechecked for this name recheckNames = append(recheckNames, name) } } @@ -733,6 +737,10 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA( return nil } +// recheckCAA accepts a list of of names that need to have their CAA records +// rechecked because their associated authorizations are sufficiently old and +// performs the CAA checks required for each. If any of the rechecks fail an +// error is returned. func (ra *RegistrationAuthorityImpl) recheckCAA(ctx context.Context, names []string) error { ra.stats.Inc("recheck_caa", 1) ra.stats.Inc("recheck_caa_names", int64(len(names))) diff --git a/ra/ra_test.go b/ra/ra_test.go index 97ce65270..eb361d6d3 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -1948,10 +1948,33 @@ func (m *mockSAWithRecentAndOlder) GetValidAuthorizations( registrationID int64, names []string, now time.Time) (map[string]*core.Authorization, error) { + makeIdentifier := func(name string) core.AcmeIdentifier { + return core.AcmeIdentifier{ + Type: core.IdentifierDNS, + Value: name, + } + } return map[string]*core.Authorization{ - "recent.com": &core.Authorization{Expires: &m.recent}, - "older.com": &core.Authorization{Expires: &m.older}, - "older2.com": &core.Authorization{Expires: &m.older}, + "recent.com": &core.Authorization{ + Identifier: makeIdentifier("recent.com"), + Expires: &m.recent, + }, + "older.com": &core.Authorization{ + Identifier: makeIdentifier("older.com"), + Expires: &m.older, + }, + "older2.com": &core.Authorization{ + Identifier: makeIdentifier("older2.com"), + Expires: &m.older, + }, + "wildcard.com": &core.Authorization{ + Identifier: makeIdentifier("wildcard.com"), + Expires: &m.older, + }, + "*.wildcard.com": &core.Authorization{ + Identifier: makeIdentifier("*.wildcard.com"), + Expires: &m.older, + }, }, nil } @@ -1967,20 +1990,42 @@ func TestRecheckCAADates(t *testing.T) { recent: fc.Now().Add(15 * time.Hour), older: fc.Now().Add(5 * time.Hour), } - names := []string{"recent.com", "older.com", "older2.com"} + + // NOTE: The names provided here correspond to authorizations in the + // `mockSAWithRecentAndOlder` + names := []string{"recent.com", "older.com", "older2.com", "wildcard.com", "*.wildcard.com"} err := ra.checkAuthorizations(context.Background(), names, 999) + // We expect that there is no error rechecking authorizations for these names if err != nil { t.Errorf("expected nil err, got %s", err) } - if recorder.names["recent.com"] { + + // We expect that "recent.com" is not checked because its mock authorization + // isn't expired + if _, present := recorder.names["recent.com"]; present { t.Errorf("Rechecked CAA unnecessarily for recent.com") } - if !recorder.names["older.com"] { - t.Errorf("Failed to recheck CAA for older.com %#v", recorder.names) - } - if !recorder.names["older2.com"] { + + // We expect that "older.com" is checked + if _, present := recorder.names["older.com"]; !present { t.Errorf("Failed to recheck CAA for older.com") } + + // We expect that "older2.com" is checked + if _, present := recorder.names["older2.com"]; !present { + t.Errorf("Failed to recheck CAA for older2.com") + } + + // We expect that the "wildcard.com" domain (without the `*.` prefix) is checked. + if _, present := recorder.names["wildcard.com"]; !present { + t.Errorf("Failed to recheck CAA for wildcard.com") + } + + // We expect that "*.wildcard.com" is checked (with the `*.` prefix, because + // it is stripped at a lower layer than we are testing) + if _, present := recorder.names["*.wildcard.com"]; !present { + t.Errorf("Failed to recheck CAA for *.wildcard.com") + } } type caaFailer struct{} diff --git a/va/caa.go b/va/caa.go index 433adb22d..7e1a0c59e 100644 --- a/va/caa.go +++ b/va/caa.go @@ -35,11 +35,11 @@ func (va *ValidationAuthorityImpl) IsCAAValid( return &vapb.IsCAAValidResponse{}, nil } -// TODO(@cpu): `checkCAA` needs to be updated to accept an authorization instead -// of a challenge. Subsequently we should also update the function to check CAA -// IssueWild if the authorization's identifier's value has a `*.` prefix (See -// #3211) -func (va *ValidationAuthorityImpl) checkCAA(ctx context.Context, identifier core.AcmeIdentifier) *probs.ProblemDetails { +// checkCAA performs a CAA lookup & validation for the provided identifier. If +// the CAA lookup & validation fail a problem is returned. +func (va *ValidationAuthorityImpl) checkCAA( + ctx context.Context, + identifier core.AcmeIdentifier) *probs.ProblemDetails { present, valid, err := va.checkCAARecords(ctx, identifier) if err != nil { return probs.ConnectionFailure(err.Error()) @@ -155,17 +155,38 @@ func (va *ValidationAuthorityImpl) getCAASet(ctx context.Context, hostname strin return parseResults(results) } -func (va *ValidationAuthorityImpl) checkCAARecords(ctx context.Context, identifier core.AcmeIdentifier) (present, valid bool, err error) { +// checkCAARecords fetches the CAA records for the given identifier and then +// validates them. If the identifier argument's value has a wildcard prefix then +// the prefix is stripped and validation will be performed against the base +// domain, honouring any issueWild CAA records encountered as apppropriate. +// checkCAARecords returns three values: the first is a bool indicating whether +// CAA records were present. The second is a bool indicating whether issuance +// for the identifier is valid. Any errors encountered are returned as the third +// return value (or nil). +func (va *ValidationAuthorityImpl) checkCAARecords( + ctx context.Context, + identifier core.AcmeIdentifier) (present, valid bool, err error) { hostname := strings.ToLower(identifier.Value) + // If this is a wildcard name, remove the prefix + var wildcard bool + if strings.HasPrefix(hostname, `*.`) { + hostname = strings.TrimPrefix(identifier.Value, `*.`) + wildcard = true + } caaSet, err := va.getCAASet(ctx, hostname) if err != nil { return false, false, err } - present, valid = va.validateCAASet(caaSet) + present, valid = va.validateCAASet(caaSet, wildcard) return present, valid, nil } -func (va *ValidationAuthorityImpl) validateCAASet(caaSet *CAASet) (present, valid bool) { +// validateCAASet checks a provided *CAASet. When the wildcard argument is true +// this means the CAASet's issueWild records must be validated as well. This +// function returns two booleans: the first indicates whether the CAASet was +// empty, the second indicates whether the CAASet is valid for issuance to +// proceed. +func (va *ValidationAuthorityImpl) validateCAASet(caaSet *CAASet, wildcard bool) (present, valid bool) { if caaSet == nil { // No CAA records found, can issue va.stats.Inc("CAA.None", 1) @@ -187,7 +208,7 @@ func (va *ValidationAuthorityImpl) validateCAASet(caaSet *CAASet) (present, vali va.stats.Inc("CAA.WithUnknownNoncritical", 1) } - if len(caaSet.Issue) == 0 { + if len(caaSet.Issue) == 0 && !wildcard { // Although CAA records exist, none of them pertain to issuance in this case. // (e.g. there is only an issuewild directive, but we are checking for a // non-wildcard identifier, or there is only an iodef or non-critical unknown @@ -196,12 +217,22 @@ func (va *ValidationAuthorityImpl) validateCAASet(caaSet *CAASet) (present, vali return true, true } + // Per RFC 6844 Section 5.3 "issueWild properties MUST be ignored when + // processing a request for a domain that is not a wildcard domain" so we + // default to checking the `caaSet.Issue` records and only check + // `caaSet.Issuewild` when `wildcard` is true and there is >0 `Issuewild` + // records. + records := caaSet.Issue + if wildcard && len(caaSet.Issuewild) > 0 { + records = caaSet.Issuewild + } + // There are CAA records pertaining to issuance in our case. Note that this // includes the case of the unsatisfiable CAA record value ";", used to // prevent issuance by any CA under any circumstance. // // Our CAA identity must be found in the chosen checkSet. - for _, caa := range caaSet.Issue { + for _, caa := range records { if extractIssuerDomain(caa) == va.issuerDomain { va.stats.Inc("CAA.Authorized", 1) return true, true diff --git a/va/caa_test.go b/va/caa_test.go index 9eafacee5..5f9fdd8a6 100644 --- a/va/caa_test.go +++ b/va/caa_test.go @@ -89,6 +89,43 @@ func (mock caaMockDNS) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, record.Tag = "issue" record.Value = ";" results = append(results, &record) + case "unsatisfiable-wildcard.com": + // Forbidden issuance - issuewild doesn't contain LE + record.Tag = "issuewild" + record.Value = ";" + results = append(results, &record) + case "unsatisfiable-wildcard-override.com": + // Forbidden issuance - issue allows LE, issuewild overrides and does not + record.Tag = "issue" + record.Value = "letsencrypt.org" + results = append(results, &record) + secondRecord := record + secondRecord.Tag = "issuewild" + secondRecord.Value = "ca.com" + results = append(results, &secondRecord) + case "satisfiable-wildcard-override.com": + // Ok issuance - issue doesn't allow LE, issuewild overrides and does + record.Tag = "issue" + record.Value = "ca.com" + results = append(results, &record) + secondRecord := record + secondRecord.Tag = "issuewild" + secondRecord.Value = "letsencrypt.org" + results = append(results, &secondRecord) + case "satisfiable-multi-wildcard.com": + // Ok issuance - first issuewild doesn't permit LE but second does + record.Tag = "issuewild" + record.Value = "ca.com" + results = append(results, &record) + secondRecord := record + secondRecord.Tag = "issuewild" + secondRecord.Value = "letsencrypt.org" + results = append(results, &secondRecord) + case "satisfiable-wildcard.com": + // Ok issuance - issuewild allows LE + record.Tag = "issuewild" + record.Value = "letsencrypt.org" + results = append(results, &record) } return results, nil } @@ -107,49 +144,144 @@ func TestCAATimeout(t *testing.T) { } func TestCAAChecking(t *testing.T) { - type CAATest struct { + + testCases := []struct { + Name string Domain string Present bool Valid bool - } - tests := []CAATest{ - // Reserved - {"reserved.com", true, false}, - // Critical - {"critical.com", true, false}, - {"nx.critical.com", true, false}, - // Good (absent) - {"absent.com", false, true}, - {"example.co.uk", false, true}, - // Good (present) - {"present.com", true, true}, - {"present.servfail.com", true, true}, - // Good (multiple critical, one matching) - {"multi-crit-present.com", true, true}, - // Bad (unknown critical) - {"unknown-critical.com", true, false}, - {"unknown-critical2.com", true, false}, - // Good (unknown noncritical, no issue/issuewild records) - {"unknown-noncritical.com", true, true}, - // Good (issue record with unknown parameters) - {"present-with-parameter.com", true, true}, - // Bad (unsatisfiable issue record) - {"unsatisfiable.com", true, false}, + }{ + { + Name: "Bad (Reserved)", + Domain: "reserved.com", + Present: true, + Valid: false, + }, + { + Name: "Bad (Critical)", + Domain: "critical.com", + Present: true, + Valid: false, + }, + { + Name: "Bad (NX Critical)", + Domain: "nx.critical.com", + Present: true, + Valid: false, + }, + { + Name: "Good (absent)", + Domain: "absent.com", + Present: false, + Valid: true, + }, + { + Name: "Good (Example.co.uk, absent)", + Domain: "example.co.uk", + Present: false, + Valid: true, + }, + { + Name: "Good (present and valid)", + Domain: "present.com", + Present: true, + Valid: true, + }, + { + Name: "Good (Present w/ servfail exception?)", + Domain: "present.servfail.com", + Present: true, + Valid: true, + }, + { + Name: "Good (multiple critical, one matching)", + Domain: "multi-crit-present.com", + Present: true, + Valid: true, + }, + { + Name: "Bad (unknown critical)", + Domain: "unknown-critical.com", + Present: true, + Valid: false, + }, + { + Name: "Bad (unknown critical 2)", + Domain: "unknown-critical2.com", + Present: true, + Valid: false, + }, + { + Name: "Good (unknown non-critical, no issue/issuewild)", + Domain: "unknown-noncritical.com", + Present: true, + Valid: true, + }, + { + Name: "Good (issue rec with unknown params)", + Domain: "present-with-parameter.com", + Present: true, + Valid: true, + }, + { + Name: "Bad (unsatisfiable issue record)", + Domain: "unsatisfiable.com", + Present: true, + Valid: false, + }, + { + Name: "Bad (unsatisfiable issue, wildcard)", + Domain: "*.unsatisfiable.com", + Present: true, + Valid: false, + }, + { + Name: "Bad (unsatisfiable wildcard)", + Domain: "*.unsatisfiable-wildcard.com", + Present: true, + Valid: false, + }, + { + Name: "Bad (unsatisfiable wildcard override)", + Domain: "*.unsatisfiable-wildcard-override.com", + Present: true, + Valid: false, + }, + { + Name: "Good (satisfiable wildcard)", + Domain: "*.satisfiable-wildcard.com", + Present: true, + Valid: true, + }, + { + Name: "Good (multiple issuewild, one satisfiable)", + Domain: "*.satisfiable-multi-wildcard.com", + Present: true, + Valid: true, + }, + { + Name: "Good (satisfiable wildcard override)", + Domain: "*.satisfiable-wildcard-override.com", + Present: true, + Valid: true, + }, } va, _ := setup(nil, 0) va.dnsClient = caaMockDNS{} - for _, caaTest := range tests { - present, valid, err := va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: caaTest.Domain}) - if err != nil { - t.Errorf("checkCAARecords error for %s: %s", caaTest.Domain, err) - } - if present != caaTest.Present { - t.Errorf("checkCAARecords presence mismatch for %s: got %t expected %t", caaTest.Domain, present, caaTest.Present) - } - if valid != caaTest.Valid { - t.Errorf("checkCAARecords validity mismatch for %s: got %t expected %t", caaTest.Domain, valid, caaTest.Valid) - } + for _, caaTest := range testCases { + t.Run(caaTest.Name, func(t *testing.T) { + present, valid, err := va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: caaTest.Domain}) + if err != nil { + t.Errorf("checkCAARecords error for %s: %s", caaTest.Domain, err) + } + if present != caaTest.Present { + t.Errorf("checkCAARecords presence mismatch for %s: got %t expected %t", caaTest.Domain, present, caaTest.Present) + } + if valid != caaTest.Valid { + t.Errorf("checkCAARecords validity mismatch for %s: got %t expected %t", caaTest.Domain, valid, caaTest.Valid) + } + }) } present, valid, err := va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: "servfail.com"}) diff --git a/va/proto/va.pb.go b/va/proto/va.pb.go index 9100d80f4..44d2460f2 100644 --- a/va/proto/va.pb.go +++ b/va/proto/va.pb.go @@ -41,6 +41,7 @@ var _ = math.Inf const _ = proto1.ProtoPackageIsVersion2 // please upgrade the proto package type IsCAAValidRequest struct { + // NOTE: Domain may be a name with a wildcard prefix (e.g. `*.example.com`) Domain *string `protobuf:"bytes,1,opt,name=domain" json:"domain,omitempty"` XXX_unrecognized []byte `json:"-"` } diff --git a/va/proto/va.proto b/va/proto/va.proto index e016390e3..daf674ebc 100644 --- a/va/proto/va.proto +++ b/va/proto/va.proto @@ -15,6 +15,7 @@ service CAA { } message IsCAAValidRequest { + // NOTE: Domain may be a name with a wildcard prefix (e.g. `*.example.com`) optional string domain = 1; } diff --git a/va/va.go b/va/va.go index 190ccd24c..f30fa63d9 100644 --- a/va/va.go +++ b/va/va.go @@ -768,18 +768,34 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, identifier return nil, probs.Unauthorized("Correct value not found for DNS challenge") } -// TODO(@cpu): `validateChallengeAndCAA` needs to be updated to accept an -// authorization instead of a challenge. Subsequently we should also update the -// function to check CAA IssueWild if the authorization's identifier's value has -// a `*.` prefix (See #3211) -func (va *ValidationAuthorityImpl) validateChallengeAndCAA(ctx context.Context, identifier core.AcmeIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) { +// validateChallengeAndCAA performs a challenge validation and CAA validation +// for the provided identifier and a corresponding challenge. If the validation +// or CAA lookup fail a problem is returned along with the validation records +// created during the validation attempt. +func (va *ValidationAuthorityImpl) validateChallengeAndCAA( + ctx context.Context, + identifier core.AcmeIdentifier, + challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) { + + // If the identifier is a wildcard domain we need to validate the base + // domain by removing the "*." wildcard prefix. We create a separate + // `baseIdentifier` here before starting the `va.checkCAA` goroutine with the + // `identifier` to avoid a data race. + baseIdentifier := identifier + if strings.HasPrefix(identifier.Value, "*.") { + baseIdentifier.Value = strings.TrimPrefix(identifier.Value, "*.") + } + + // va.checkCAA accepts wildcard identifiers and handles them appropriately so + // we can dispatch `checkCAA` with the provided `identifier` instead of + // `baseIdentifier` ch := make(chan *probs.ProblemDetails, 1) go func() { ch <- va.checkCAA(ctx, identifier) }() // TODO(#1292): send into another goroutine - validationRecords, err := va.validateChallenge(ctx, identifier, challenge) + validationRecords, err := va.validateChallenge(ctx, baseIdentifier, challenge) if err != nil { return validationRecords, err } @@ -879,19 +895,16 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, domain } vStart := va.clk.Now() - // If the identifier is a wildcard domain we need to validate the base - // domain by removing the "*." wildcard prefix. - if strings.HasPrefix(domain, "*.") { - domain = strings.TrimPrefix(domain, "*.") - } - var remoteError chan *probs.ProblemDetails if len(va.remoteVAs) > 0 { remoteError = make(chan *probs.ProblemDetails, 1) go va.performRemoteValidation(ctx, domain, challenge, authz, remoteError) } - records, prob := va.validateChallengeAndCAA(ctx, core.AcmeIdentifier{Type: "dns", Value: domain}, challenge) + records, prob := va.validateChallengeAndCAA( + ctx, + core.AcmeIdentifier{Type: "dns", Value: domain}, + challenge) logEvent.ValidationRecords = records challenge.ValidationRecord = records