From d58d09615a32fdb7f4b40bf2c666589e0c606e6f Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 29 Aug 2024 15:38:50 -0700 Subject: [PATCH] Improve how we disable challenge types (#7677) When creating an authorization, populate it with all challenges appropriate for that identifier, regardless of whether those challenge types are currently "enabled" in the config. This ensures that authorizations created during a incident for which we can temporarily disabled a single challenge type can still be validated via that challenge type after the incident is over. Also, when finalizing an order, check that the challenge type used to validation each authorization is not currently disabled. This ensures that, if we temporarily disable a single challenge due to an incident, we don't issue any more certificates using authorizations which were fulfilled using that disabled challenge. Note that standard rolling deployment of this change is not safe if any challenges are disabled at the same time, due to the possibility of an updated RA not filtering a challenge when writing it to the database, and then a non-updated RA not filtering it when reading from the database. But if all challenges are enabled then this change is safe for normal deploy. Fixes https://github.com/letsencrypt/boulder/issues/5913 --- core/interfaces.go | 2 +- core/objects.go | 4 +- core/objects_test.go | 4 +- csr/csr_test.go | 2 +- policy/pa.go | 60 ++++++++------- policy/pa_test.go | 172 +++++++++++++++++++++++++++++++------------ ra/ra.go | 2 +- ra/ra_test.go | 61 ++++++++++++++- 8 files changed, 220 insertions(+), 87 deletions(-) diff --git a/core/interfaces.go b/core/interfaces.go index d37247290da..35ebf38964a 100644 --- a/core/interfaces.go +++ b/core/interfaces.go @@ -10,5 +10,5 @@ type PolicyAuthority interface { WillingToIssue([]string) error ChallengeTypesFor(identifier.ACMEIdentifier) ([]AcmeChallenge, error) ChallengeTypeEnabled(AcmeChallenge) bool - CheckAuthz(*Authorization) error + CheckAuthzChallenges(*Authorization) error } diff --git a/core/objects.go b/core/objects.go index 9a8f25bb4a9..ad09b01611b 100644 --- a/core/objects.go +++ b/core/objects.go @@ -337,14 +337,14 @@ func (authz *Authorization) FindChallengeByStringID(id string) int { // challenge is valid. func (authz *Authorization) SolvedBy() (AcmeChallenge, error) { if len(authz.Challenges) == 0 { - return "", fmt.Errorf("Authorization has no challenges") + return "", fmt.Errorf("authorization has no challenges") } for _, chal := range authz.Challenges { if chal.Status == StatusValid { return chal.Type, nil } } - return "", fmt.Errorf("Authorization not solved by any challenge") + return "", fmt.Errorf("authorization not solved by any challenge") } // JSONBuffer fields get encoded and decoded JOSE-style, in base64url encoding diff --git a/core/objects_test.go b/core/objects_test.go index 8f9886a4bae..9b9d41f6508 100644 --- a/core/objects_test.go +++ b/core/objects_test.go @@ -100,7 +100,7 @@ func TestAuthorizationSolvedBy(t *testing.T) { { Name: "No challenges", Authz: Authorization{}, - ExpectedError: "Authorization has no challenges", + ExpectedError: "authorization has no challenges", }, // An authz with all non-valid challenges should return nil { @@ -108,7 +108,7 @@ func TestAuthorizationSolvedBy(t *testing.T) { Authz: Authorization{ Challenges: []Challenge{HTTPChallenge01(""), DNSChallenge01("")}, }, - ExpectedError: "Authorization not solved by any challenge", + ExpectedError: "authorization not solved by any challenge", }, // An authz with one valid HTTP01 challenge amongst other challenges should // return the HTTP01 challenge diff --git a/csr/csr_test.go b/csr/csr_test.go index 5dc2d6003f8..658a69bc6ad 100644 --- a/csr/csr_test.go +++ b/csr/csr_test.go @@ -39,7 +39,7 @@ func (pa *mockPA) ChallengeTypeEnabled(t core.AcmeChallenge) bool { return true } -func (pa *mockPA) CheckAuthz(a *core.Authorization) error { +func (pa *mockPA) CheckAuthzChallenges(a *core.Authorization) error { return nil } diff --git a/policy/pa.go b/policy/pa.go index 26edbdbdf37..67b1876f5fd 100644 --- a/policy/pa.go +++ b/policy/pa.go @@ -517,38 +517,31 @@ func (pa *AuthorityImpl) checkHostLists(domain string) error { } // ChallengeTypesFor determines which challenge types are acceptable for the -// given identifier. -func (pa *AuthorityImpl) ChallengeTypesFor(identifier identifier.ACMEIdentifier) ([]core.AcmeChallenge, error) { - // If the identifier is for a DNS wildcard name we only - // provide a DNS-01 challenge as a matter of CA policy. - if strings.HasPrefix(identifier.Value, "*.") { - // We must have the DNS-01 challenge type enabled to create challenges for - // a wildcard identifier per LE policy. - if !pa.ChallengeTypeEnabled(core.ChallengeTypeDNS01) { - return nil, fmt.Errorf( - "Challenges requested for wildcard identifier but DNS-01 " + - "challenge type is not enabled") - } - // Only provide a DNS-01-Wildcard challenge +// given identifier. This determination is made purely based on the identifier, +// and not based on which challenge types are enabled, so that challenge type +// filtering can happen dynamically at request rather than being set in stone +// at creation time. +func (pa *AuthorityImpl) ChallengeTypesFor(ident identifier.ACMEIdentifier) ([]core.AcmeChallenge, error) { + // If the identifier is for a DNS wildcard name we only provide a DNS-01 + // challenge, to comply with the BRs Sections 3.2.2.4.19 and 3.2.2.4.20 + // stating that ACME HTTP-01 and TLS-ALPN-01 are not suitable for validating + // Wildcard Domains. + if ident.Type == identifier.DNS && strings.HasPrefix(ident.Value, "*.") { return []core.AcmeChallenge{core.ChallengeTypeDNS01}, nil } - // Otherwise we collect up challenges based on what is enabled. - var challenges []core.AcmeChallenge - - if pa.ChallengeTypeEnabled(core.ChallengeTypeHTTP01) { - challenges = append(challenges, core.ChallengeTypeHTTP01) - } - - if pa.ChallengeTypeEnabled(core.ChallengeTypeTLSALPN01) { - challenges = append(challenges, core.ChallengeTypeTLSALPN01) + // Return all challenge types we support for non-wildcard DNS identifiers. + if ident.Type == identifier.DNS { + return []core.AcmeChallenge{ + core.ChallengeTypeHTTP01, + core.ChallengeTypeDNS01, + core.ChallengeTypeTLSALPN01, + }, nil } - if pa.ChallengeTypeEnabled(core.ChallengeTypeDNS01) { - challenges = append(challenges, core.ChallengeTypeDNS01) - } - - return challenges, nil + // Otherwise return an error because we don't support any challenges for this + // identifier type. + return nil, fmt.Errorf("unrecognized identifier type %q", ident.Type) } // ChallengeTypeEnabled returns whether the specified challenge type is enabled @@ -558,21 +551,26 @@ func (pa *AuthorityImpl) ChallengeTypeEnabled(t core.AcmeChallenge) bool { return pa.enabledChallenges[t] } -// CheckAuthz determines that an authorization was fulfilled by a challenge -// that was appropriate for the kind of identifier in the authorization. -func (pa *AuthorityImpl) CheckAuthz(authz *core.Authorization) error { +// CheckAuthzChallenges determines that an authorization was fulfilled by a +// challenge that is currently enabled and was appropriate for the kind of +// identifier in the authorization. +func (pa *AuthorityImpl) CheckAuthzChallenges(authz *core.Authorization) error { chall, err := authz.SolvedBy() if err != nil { return err } + if !pa.ChallengeTypeEnabled(chall) { + return errors.New("authorization fulfilled by disabled challenge type") + } + challTypes, err := pa.ChallengeTypesFor(authz.Identifier) if err != nil { return err } if !slices.Contains(challTypes, chall) { - return errors.New("authorization fulfilled by invalid challenge") + return errors.New("authorization fulfilled by inapplicable challenge type") } return nil diff --git a/policy/pa_test.go b/policy/pa_test.go index 88358d7aaf3..9398d8604d7 100644 --- a/policy/pa_test.go +++ b/policy/pa_test.go @@ -12,16 +12,16 @@ import ( "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/identifier" blog "github.com/letsencrypt/boulder/log" - "github.com/letsencrypt/boulder/must" "github.com/letsencrypt/boulder/test" ) -var enabledChallenges = map[core.AcmeChallenge]bool{ - core.ChallengeTypeHTTP01: true, - core.ChallengeTypeDNS01: true, -} - func paImpl(t *testing.T) *AuthorityImpl { + enabledChallenges := map[core.AcmeChallenge]bool{ + core.ChallengeTypeHTTP01: true, + core.ChallengeTypeDNS01: true, + core.ChallengeTypeTLSALPN01: true, + } + pa, err := New(enabledChallenges, blog.NewMock()) if err != nil { t.Fatalf("Couldn't create policy implementation: %s", err) @@ -388,52 +388,52 @@ func TestWillingToIssue_SubErrors(t *testing.T) { } func TestChallengeTypesFor(t *testing.T) { + t.Parallel() pa := paImpl(t) - challenges, err := pa.ChallengeTypesFor(identifier.ACMEIdentifier{}) - test.AssertNotError(t, err, "ChallengesFor failed") - - test.Assert(t, len(challenges) == len(enabledChallenges), "Wrong number of challenges returned") - - seenChalls := make(map[core.AcmeChallenge]bool) - for _, challenge := range challenges { - test.Assert(t, !seenChalls[challenge], "should not already have seen this type") - seenChalls[challenge] = true - - test.Assert(t, enabledChallenges[challenge], "Unsupported challenge returned") + testCases := []struct { + name string + ident identifier.ACMEIdentifier + wantChalls []core.AcmeChallenge + wantErr string + }{ + { + name: "dns", + ident: identifier.DNSIdentifier("example.com"), + wantChalls: []core.AcmeChallenge{ + core.ChallengeTypeHTTP01, core.ChallengeTypeDNS01, core.ChallengeTypeTLSALPN01, + }, + }, + { + name: "wildcard", + ident: identifier.DNSIdentifier("*.example.com"), + wantChalls: []core.AcmeChallenge{ + core.ChallengeTypeDNS01, + }, + }, + { + name: "other", + ident: identifier.ACMEIdentifier{Type: "ip", Value: "1.2.3.4"}, + wantErr: "unrecognized identifier type", + }, } - test.AssertEquals(t, len(seenChalls), len(enabledChallenges)) -} -func TestChallengeTypesForWildcard(t *testing.T) { - // wildcardIdent is an identifier for a wildcard domain name - wildcardIdent := identifier.ACMEIdentifier{ - Type: identifier.DNS, - Value: "*.zombo.com", - } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + challs, err := pa.ChallengeTypesFor(tc.ident) + + if len(tc.wantChalls) != 0 { + test.AssertNotError(t, err, "should have succeeded") + test.AssertDeepEquals(t, challs, tc.wantChalls) + } - // First try to get a challenge for the wildcard ident without the - // DNS-01 challenge type enabled. This should produce an error - var enabledChallenges = map[core.AcmeChallenge]bool{ - core.ChallengeTypeHTTP01: true, - core.ChallengeTypeDNS01: false, + if tc.wantErr != "" { + test.AssertError(t, err, "should have errored") + test.AssertContains(t, err.Error(), tc.wantErr) + } + }) } - pa := must.Do(New(enabledChallenges, blog.NewMock())) - _, err := pa.ChallengeTypesFor(wildcardIdent) - test.AssertError(t, err, "ChallengesFor did not error for a wildcard ident "+ - "when DNS-01 was disabled") - test.AssertEquals(t, err.Error(), "Challenges requested for wildcard "+ - "identifier but DNS-01 challenge type is not enabled") - - // Try again with DNS-01 enabled. It should not error and - // should return only one DNS-01 type challenge - enabledChallenges[core.ChallengeTypeDNS01] = true - pa = must.Do(New(enabledChallenges, blog.NewMock())) - challenges, err := pa.ChallengeTypesFor(wildcardIdent) - test.AssertNotError(t, err, "ChallengesFor errored for a wildcard ident "+ - "unexpectedly") - test.AssertEquals(t, len(challenges), 1) - test.AssertEquals(t, challenges[0], core.ChallengeTypeDNS01) } // TestMalformedExactBlocklist tests that loading a YAML policy file with an @@ -483,3 +483,83 @@ func TestValidEmailError(t *testing.T) { err = ValidEmail("example@-foobar.com") test.AssertEquals(t, err.Error(), "contact email \"example@-foobar.com\" has invalid domain : Domain name contains an invalid character") } + +func TestCheckAuthzChallenges(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + authz core.Authorization + enabled map[core.AcmeChallenge]bool + wantErr string + }{ + { + name: "unrecognized identifier", + authz: core.Authorization{ + Identifier: identifier.ACMEIdentifier{Type: "oops", Value: "example.com"}, + Challenges: []core.Challenge{{Type: core.ChallengeTypeDNS01, Status: core.StatusValid}}, + }, + wantErr: "unrecognized identifier type", + }, + { + name: "no challenges", + authz: core.Authorization{ + Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"}, + Challenges: []core.Challenge{}, + }, + wantErr: "has no challenges", + }, + { + name: "no valid challenges", + authz: core.Authorization{ + Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"}, + Challenges: []core.Challenge{{Type: core.ChallengeTypeDNS01, Status: core.StatusPending}}, + }, + wantErr: "not solved by any challenge", + }, + { + name: "solved by disabled challenge", + authz: core.Authorization{ + Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"}, + Challenges: []core.Challenge{{Type: core.ChallengeTypeDNS01, Status: core.StatusValid}}, + }, + enabled: map[core.AcmeChallenge]bool{core.ChallengeTypeHTTP01: true}, + wantErr: "disabled challenge type", + }, + { + name: "solved by wrong kind of challenge", + authz: core.Authorization{ + Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "*.example.com"}, + Challenges: []core.Challenge{{Type: core.ChallengeTypeHTTP01, Status: core.StatusValid}}, + }, + wantErr: "inapplicable challenge type", + }, + { + name: "valid authz", + authz: core.Authorization{ + Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"}, + Challenges: []core.Challenge{{Type: core.ChallengeTypeTLSALPN01, Status: core.StatusValid}}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + pa := paImpl(t) + + if tc.enabled != nil { + pa.enabledChallenges = tc.enabled + } + + err := pa.CheckAuthzChallenges(&tc.authz) + + if tc.wantErr == "" { + test.AssertNotError(t, err, "should have succeeded") + } else { + test.AssertError(t, err, "should have errored") + test.AssertContains(t, err.Error(), tc.wantErr) + } + }) + } +} diff --git a/ra/ra.go b/ra/ra.go index 513f2d4444f..cb199bb4aeb 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -826,7 +826,7 @@ func (ra *RegistrationAuthorityImpl) checkOrderAuthorizations( expired = append(expired, ident.Value) continue } - err = ra.PA.CheckAuthz(authz) + err = ra.PA.CheckAuthzChallenges(authz) if err != nil { invalid = append(invalid, ident.Value) continue diff --git a/ra/ra_test.go b/ra/ra_test.go index 461a2ad8cd2..ec76762b5c8 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -2516,7 +2516,7 @@ func TestNewOrderWildcard(t *testing.T) { test.AssertEquals(t, authz.Challenges[0].Type, core.ChallengeTypeDNS01) case "example.com": // If the authz is for example.com, we expect it has normal challenges - test.AssertEquals(t, len(authz.Challenges), 2) + test.AssertEquals(t, len(authz.Challenges), 3) default: t.Fatalf("Received an authorization for a name not requested: %q", name) } @@ -2556,7 +2556,7 @@ func TestNewOrderWildcard(t *testing.T) { case "zombo.com": // We expect that the base domain identifier auth has the normal number of // challenges - test.AssertEquals(t, len(authz.Challenges), 2) + test.AssertEquals(t, len(authz.Challenges), 3) case "*.zombo.com": // We expect that the wildcard identifier auth has only a pending // DNS-01 type challenge @@ -2590,7 +2590,7 @@ func TestNewOrderWildcard(t *testing.T) { // We expect the authz is for the identifier the correct domain test.AssertEquals(t, authz.Identifier.Value, "everything.is.possible.zombo.com") // We expect the authz has the normal # of challenges - test.AssertEquals(t, len(authz.Challenges), 2) + test.AssertEquals(t, len(authz.Challenges), 3) // Now submit an order request for a wildcard of the domain we just created an // order for. We should **NOT** reuse the authorization from the previous @@ -3153,6 +3153,61 @@ func TestFinalizeOrderWildcard(t *testing.T) { "wildcard order") } +func TestFinalizeOrderDisabledChallenge(t *testing.T) { + _, sa, ra, fc, cleanUp := initAuthorities(t) + defer cleanUp() + + // Create a random domain + var bytes [3]byte + _, err := rand.Read(bytes[:]) + test.AssertNotError(t, err, "creating test domain name") + domain := fmt.Sprintf("%x.example.com", bytes[:]) + + // Create a finalized authorization for that domain + authzID := createFinalizedAuthorization( + t, sa, domain, fc.Now().Add(24*time.Hour), core.ChallengeTypeHTTP01, fc.Now().Add(-1*time.Hour)) + + // Create an order that reuses that authorization + order, err := ra.NewOrder(context.Background(), &rapb.NewOrderRequest{ + RegistrationID: Registration.Id, + DnsNames: []string{domain}, + }) + test.AssertNotError(t, err, "creating test order") + test.AssertEquals(t, order.V2Authorizations[0], authzID) + + // Create a CSR for this order + testKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + test.AssertNotError(t, err, "generating test key") + csr, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{ + PublicKey: testKey.PublicKey, + DNSNames: []string{domain}, + }, testKey) + test.AssertNotError(t, err, "Error creating policy forbid CSR") + + // Replace the Policy Authority with one which has this challenge type disabled + pa, err := policy.New(map[core.AcmeChallenge]bool{ + core.ChallengeTypeDNS01: true, + core.ChallengeTypeTLSALPN01: true, + }, ra.log) + test.AssertNotError(t, err, "creating test PA") + err = pa.LoadHostnamePolicyFile("../test/hostname-policy.yaml") + test.AssertNotError(t, err, "loading test hostname policy") + ra.PA = pa + + // Now finalizing this order should fail + _, err = ra.FinalizeOrder(context.Background(), &rapb.FinalizeOrderRequest{ + Order: order, + Csr: csr, + }) + test.AssertError(t, err, "finalization should fail") + + // Unfortunately we can't test for the PA's "which is now disabled" error + // message directly, because the RA discards it and collects all invalid names + // into a single more generic error message. But it does at least distinguish + // between missing, expired, and invalid, so we can test for "invalid". + test.AssertContains(t, err.Error(), "authorizations for these identifiers not valid") +} + func TestIssueCertificateAuditLog(t *testing.T) { _, sa, ra, _, cleanUp := initAuthorities(t) defer cleanUp()