From de5fbbdb6727ae10a2a05e4e025063c8730d6868 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 13 Dec 2017 12:09:33 -0500 Subject: [PATCH] Implement CAA issueWild enforcement for wildcard names (#3266) This commit implements RFC 6844's description of the "CAA issuewild property" for CAA records. We check CAA in two places: at the time of validation, and at the time of issuance when an authorization is more than 8hours old. Both locations have been updated to properly enforce issuewild when checking CAA for a domain corresponding to a wildcard name in a certificate order. Resolves https://github.com/letsencrypt/boulder/issues/3211 --- ra/ra.go | 10 ++- ra/ra_test.go | 63 ++++++++++++-- va/caa.go | 51 +++++++++--- va/caa_test.go | 204 ++++++++++++++++++++++++++++++++++++++-------- va/proto/va.pb.go | 1 + va/proto/va.proto | 1 + va/va.go | 39 ++++++--- 7 files changed, 300 insertions(+), 69 deletions(-) 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