Remove deprecated sapb.NewAuthzRequest fields (#7651)

Remove the id, identifierValue, status, and challenges fields from
sapb.NewAuthzRequest. These fields were left behind from the previous
corepb.Authorization request type, and are now being ignored by the SA.

Since the RA is no longer constructing full challenge objects to include
in the request, remove pa.ChallengesFor and replace it with the much
simpler pa.ChallengeTypesFor.

Part of #5913

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
 }
 

sa/model.go

@@ -686,20 +686,6 @@ func hasMultipleNonPendingChallenges(challenges []*corepb.Challenge) bool {
 // representation. It hardcodes the status to "pending" because it should be
 // impossible to create an authz in any other state.
 func newAuthzReqToModel(authz *sapb.NewAuthzRequest) (*authzModel, error) {
-	if authz.Token == "" && len(authz.ChallengeTypes) == 0 {
-		// This is actually a corepb.Authorization, sent to us by a not-yet-updated
-		// RA. Use the old code-path instead.
-		// TODO(#5913): Remove this fallback.
-		return authzPBToModel(&corepb.Authorization{
-			Id:             authz.Id,
-			DnsName:        authz.DnsName,
-			RegistrationID: authz.RegistrationID,
-			Status:         string(core.StatusPending),
-			Expires:        authz.Expires,
-			Challenges:     authz.Challenges,
-		})
-	}
-
 	am := &authzModel{
 		IdentifierType:  identifierTypeToUint[authz.Identifier.Type],
 		IdentifierValue: authz.Identifier.Value,