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 #5913

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
 }

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

core/objects_test.go

@@ -100,15 +100,15 @@ 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
 		{
 			Name: "All non-valid challenges",
 			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

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
 }
 

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

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("[email protected]")
 	test.AssertEquals(t, err.Error(), "contact email \"[email protected]\" 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)
+			}
+		})
+	}
+}

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