Merge branch main into better-check-finalize-cleanup

core/interfaces.go

@@ -8,7 +8,7 @@ import (
 // TODO(#5891): Move this interface to a more appropriate location.
 type PolicyAuthority interface {
 	WillingToIssue([]string) error
-	ChallengesFor(identifier.ACMEIdentifier) ([]Challenge, error)
+	ChallengeTypesFor(identifier.ACMEIdentifier) ([]AcmeChallenge, error)
 	ChallengeTypeEnabled(AcmeChallenge) bool
 	CheckAuthz(*Authorization) error
 }

csr/csr_test.go

@@ -22,8 +22,8 @@ import (
 
 type mockPA struct{}
 
-func (pa *mockPA) ChallengesFor(identifier identifier.ACMEIdentifier) (challenges []core.Challenge, err error) {
-	return
+func (pa *mockPA) ChallengeTypesFor(identifier identifier.ACMEIdentifier) ([]core.AcmeChallenge, error) {
+	return []core.AcmeChallenge{}, nil
 }
 
 func (pa *mockPA) WillingToIssue(domains []string) error {

policy/pa.go

@@ -5,7 +5,6 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
-	"math/rand/v2"
 	"net"
 	"net/mail"
 	"os"
@@ -35,21 +34,14 @@ type AuthorityImpl struct {
 	blocklistMu            sync.RWMutex
 
 	enabledChallenges map[core.AcmeChallenge]bool
-	pseudoRNG         *rand.Rand
-	rngMu             sync.Mutex
 }
 
 // New constructs a Policy Authority.
 func New(challengeTypes map[core.AcmeChallenge]bool, log blog.Logger) (*AuthorityImpl, error) {
-
-	pa := AuthorityImpl{
+	return &AuthorityImpl{
 		log:               log,
 		enabledChallenges: challengeTypes,
-		// We don't need real randomness for this.
-		pseudoRNG: rand.New(rand.NewPCG(rand.Uint64(), rand.Uint64())),
-	}
-
-	return &pa, nil
+	}, nil
 }
 
 // blockedNamesPolicy is a struct holding lists of blocked domain names. One for
@@ -524,11 +516,9 @@ func (pa *AuthorityImpl) checkHostLists(domain string) error {
 	return nil
 }
 
-// challengeTypesFor determines which challenge types are acceptable for the
+// ChallengeTypesFor determines which challenge types are acceptable for the
 // given identifier.
-func (pa *AuthorityImpl) challengeTypesFor(identifier identifier.ACMEIdentifier) ([]core.AcmeChallenge, error) {
-	var challenges []core.AcmeChallenge
-
+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, "*.") {
@@ -540,59 +530,25 @@ func (pa *AuthorityImpl) challengeTypesFor(identifier identifier.ACMEIdentifier)
 					"challenge type is not enabled")
 		}
 		// Only provide a DNS-01-Wildcard challenge
-		challenges = []core.AcmeChallenge{core.ChallengeTypeDNS01}
-	} else {
-		// Otherwise we collect up challenges based on what is enabled.
-		if pa.ChallengeTypeEnabled(core.ChallengeTypeHTTP01) {
-			challenges = append(challenges, core.ChallengeTypeHTTP01)
-		}
-
-		if pa.ChallengeTypeEnabled(core.ChallengeTypeTLSALPN01) {
-			challenges = append(challenges, core.ChallengeTypeTLSALPN01)
-		}
-
-		if pa.ChallengeTypeEnabled(core.ChallengeTypeDNS01) {
-			challenges = append(challenges, core.ChallengeTypeDNS01)
-		}
+		return []core.AcmeChallenge{core.ChallengeTypeDNS01}, nil
 	}
 
-	return challenges, nil
-}
+	// Otherwise we collect up challenges based on what is enabled.
+	var challenges []core.AcmeChallenge
 
-// ChallengesFor determines which challenge types are acceptable for the given
-// identifier, and constructs new challenge objects for those challenge types.
-// The resulting challenge objects all share a single challenge token and are
-// returned in a random order.
-func (pa *AuthorityImpl) ChallengesFor(identifier identifier.ACMEIdentifier) ([]core.Challenge, error) {
-	challTypes, err := pa.challengeTypesFor(identifier)
-	if err != nil {
-		return nil, err
+	if pa.ChallengeTypeEnabled(core.ChallengeTypeHTTP01) {
+		challenges = append(challenges, core.ChallengeTypeHTTP01)
 	}
 
-	challenges := make([]core.Challenge, len(challTypes))
-
-	token := core.NewToken()
-
-	for i, t := range challTypes {
-		c, err := core.NewChallenge(t, token)
-		if err != nil {
-			return nil, err
-		}
-
-		challenges[i] = c
+	if pa.ChallengeTypeEnabled(core.ChallengeTypeTLSALPN01) {
+		challenges = append(challenges, core.ChallengeTypeTLSALPN01)
 	}
 
-	// We shuffle the challenges to prevent ACME clients from relying on the
-	// specific order that boulder returns them in.
-	shuffled := make([]core.Challenge, len(challenges))
-
-	pa.rngMu.Lock()
-	defer pa.rngMu.Unlock()
-	for i, challIdx := range pa.pseudoRNG.Perm(len(challenges)) {
-		shuffled[i] = challenges[challIdx]
+	if pa.ChallengeTypeEnabled(core.ChallengeTypeDNS01) {
+		challenges = append(challenges, core.ChallengeTypeDNS01)
 	}
 
-	return shuffled, nil
+	return challenges, nil
 }
 
 // ChallengeTypeEnabled returns whether the specified challenge type is enabled
@@ -610,7 +566,7 @@ func (pa *AuthorityImpl) CheckAuthz(authz *core.Authorization) error {
 		return err
 	}
 
-	challTypes, err := pa.challengeTypesFor(authz.Identifier)
+	challTypes, err := pa.ChallengeTypesFor(authz.Identifier)
 	if err != nil {
 		return err
 	}

policy/pa_test.go

@@ -5,14 +5,15 @@ import (
 	"os"
 	"testing"
 
+	"gopkg.in/yaml.v3"
+
 	"github.com/letsencrypt/boulder/core"
 	berrors "github.com/letsencrypt/boulder/errors"
 	"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"
-	"gopkg.in/yaml.v3"
 )
 
 var enabledChallenges = map[core.AcmeChallenge]bool{
@@ -386,26 +387,25 @@ func TestWillingToIssue_SubErrors(t *testing.T) {
 		})
 }
 
-func TestChallengesFor(t *testing.T) {
+func TestChallengeTypesFor(t *testing.T) {
 	pa := paImpl(t)
 
-	challenges, err := pa.ChallengesFor(identifier.ACMEIdentifier{})
+	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.Type], "should not already have seen this type")
-		seenChalls[challenge.Type] = true
+		test.Assert(t, !seenChalls[challenge], "should not already have seen this type")
+		seenChalls[challenge] = true
 
-		test.Assert(t, enabledChallenges[challenge.Type], "Unsupported challenge returned")
+		test.Assert(t, enabledChallenges[challenge], "Unsupported challenge returned")
 	}
 	test.AssertEquals(t, len(seenChalls), len(enabledChallenges))
-
 }
 
-func TestChallengesForWildcard(t *testing.T) {
+func TestChallengeTypesForWildcard(t *testing.T) {
 	// wildcardIdent is an identifier for a wildcard domain name
 	wildcardIdent := identifier.ACMEIdentifier{
 		Type:  identifier.DNS,
@@ -419,7 +419,7 @@ func TestChallengesForWildcard(t *testing.T) {
 		core.ChallengeTypeDNS01:  false,
 	}
 	pa := must.Do(New(enabledChallenges, blog.NewMock()))
-	_, err := pa.ChallengesFor(wildcardIdent)
+	_, 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 "+
@@ -429,11 +429,11 @@ func TestChallengesForWildcard(t *testing.T) {
 	// should return only one DNS-01 type challenge
 	enabledChallenges[core.ChallengeTypeDNS01] = true
 	pa = must.Do(New(enabledChallenges, blog.NewMock()))
-	challenges, err := pa.ChallengesFor(wildcardIdent)
+	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].Type, core.ChallengeTypeDNS01)
+	test.AssertEquals(t, challenges[0], core.ChallengeTypeDNS01)
 }
 
 // TestMalformedExactBlocklist tests that loading a YAML policy file with an

ra/ra.go

@@ -2644,7 +2644,7 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 			return nil, err
 		}
 		newAuthzs = append(newAuthzs, pb)
-		ra.authzAges.WithLabelValues("NewOrder", pb.Status).Observe(0)
+		ra.authzAges.WithLabelValues("NewOrder", string(core.StatusPending)).Observe(0)
 	}
 
 	// Start with the order's own expiry as the minExpiry. We only care
@@ -2702,61 +2702,27 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 // necessary challenges for it and puts this and all of the relevant information
 // into a corepb.Authorization for transmission to the SA to be stored
 func (ra *RegistrationAuthorityImpl) createPendingAuthz(reg int64, identifier identifier.ACMEIdentifier) (*sapb.NewAuthzRequest, error) {
+	challTypes, err := ra.PA.ChallengeTypesFor(identifier)
+	if err != nil {
+		return nil, err
+	}
+
+	challStrs := make([]string, len(challTypes))
+	for i, t := range challTypes {
+		challStrs[i] = string(t)
+	}
+
 	authz := &sapb.NewAuthzRequest{
-		DnsName: identifier.Value,
 		Identifier: &sapb.Identifier{
 			Type:  string(identifier.Type),
 			Value: identifier.Value,
 		},
 		RegistrationID: reg,
-		Status:         string(core.StatusPending),
 		Expires:        timestamppb.New(ra.clk.Now().Add(ra.pendingAuthorizationLifetime).Truncate(time.Second)),
+		ChallengeTypes: challStrs,
+		Token:          core.NewToken(),
 	}
 
-	// Create challenges. The WFE will update them with URIs before sending them out.
-	challenges, err := ra.PA.ChallengesFor(identifier)
-	if err != nil {
-		// The only time ChallengesFor errors it is a fatal configuration error
-		// where challenges required by policy for an identifier are not enabled. We
-		// want to treat this as an internal server error.
-		return nil, berrors.InternalServerError(err.Error())
-	}
-	// Check each challenge for sanity.
-	var token string
-	var challTypes []string
-	for _, challenge := range challenges {
-		err := challenge.CheckPending()
-		if err != nil {
-			// berrors.InternalServerError because we generated these challenges, they should
-			// be OK.
-			err = berrors.InternalServerError("challenge didn't pass sanity check: %+v", challenge)
-			return nil, err
-		}
-
-		if token == "" {
-			token = challenge.Token
-		} else {
-			if challenge.Token != token {
-				return nil, berrors.InternalServerError("generated different tokens for challenges within the same authz")
-			}
-		}
-
-		if slices.Contains(challTypes, string(challenge.Type)) {
-			return nil, berrors.InternalServerError("generated multiple challenges of the same type within the same authz")
-		} else {
-			challTypes = append(challTypes, string(challenge.Type))
-		}
-
-		challPB, err := bgrpc.ChallengeToPB(challenge)
-		if err != nil {
-			return nil, err
-		}
-		authz.Challenges = append(authz.Challenges, challPB)
-	}
-
-	authz.Token = token
-	authz.ChallengeTypes = challTypes
-
 	return authz, nil
 }