More fixes

csr/csr_test.go

@@ -37,6 +37,7 @@ func (pa *mockPA) WillingToIssue(id core.AcmeIdentifier) error {
 func (pa *mockPA) WillingToIssueWildcard(id core.AcmeIdentifier) error {
 	return nil
 }
+
 func (pa *mockPA) ChallengeTypeEnabled(t string) bool {
 	return true
 }

ra/ra.go

@@ -532,7 +532,7 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, reque
 				ra.log.Warning(fmt.Sprintf("%s: %s", outErr.Error(), existingAuthz.ID))
 				return core.Authorization{}, outErr
 			}
-			if !features.Enabled(features.EnforceChallengeDisable) || ra.validChallengeStillGood(&populatedAuthz) {
+			if !features.Enabled(features.EnforceChallengeDisable) || ra.authzValidChallengeEnabled(&populatedAuthz) {
 				// The existing authorization must not expire within the next 24 hours for
 				// it to be OK for reuse
 				reuseCutOff := ra.clk.Now().Add(time.Hour * 24)
@@ -722,8 +722,8 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA(
 			// Ensure that CAA is rechecked for this name
 			recheckNames = append(recheckNames, name)
 		}
-		if authz != nil && features.Enabled(features.EnforceChallengeDisable) && !ra.validChallengeStillGood(authz) {
-			return berrors.UnauthorizedError("challenge used to validate authorization with ID %q no longer allowed", authz.ID)
+		if authz != nil && features.Enabled(features.EnforceChallengeDisable) && !ra.authzValidChallengeEnabled(authz) {
+			return berrors.UnauthorizedError("challenge used to validate authorization with ID %q forbidden by policy", authz.ID)
 		}
 	}
 
@@ -1757,9 +1757,9 @@ func (ra *RegistrationAuthorityImpl) createPendingAuthz(ctx context.Context, reg
 	return authz, nil
 }
 
-// validChallengeStillGood checks whether the valid challenge in an authorization uses a type
+// authzValidChallengeEnabled checks whether the valid challenge in an authorization uses a type
 // which is still enabled
-func (ra *RegistrationAuthorityImpl) validChallengeStillGood(authz *core.Authorization) bool {
+func (ra *RegistrationAuthorityImpl) authzValidChallengeEnabled(authz *core.Authorization) bool {
 	for _, chall := range authz.Challenges {
 		if chall.Status == core.StatusValid {
 			fmt.Println("TYPE", chall.Type)

ra/ra_test.go

@@ -2914,7 +2914,7 @@ func TestDisabledChallengeValidAuthz(t *testing.T) {
 		0,
 		fc.Now(),
 	)
-	test.AssertError(t, err, "RA didn't prevent use of an authorization which used an disabled challenge type")
+	test.AssertError(t, err, "RA didn't prevent use of an authorization which used a disabled challenge type")
 
 	err = ra.checkAuthorizationsCAA(
 		context.Background(),
@@ -2942,11 +2942,11 @@ func TestValidChallengeStillGood(t *testing.T) {
 
 	_ = features.Set(map[string]bool{"EnforceChallengeDisable": true})
 
-	test.Assert(t, !ra.validChallengeStillGood(&core.Authorization{}), "ra.validChallengeStillGood didn't fail with empty authorization")
-	test.Assert(t, !ra.validChallengeStillGood(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusPending}}}), "ra.validChallengeStillGood didn't fail with no valid challenges")
-	test.Assert(t, !ra.validChallengeStillGood(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusValid, Type: core.ChallengeTypeHTTP01}}}), "ra.validChallengeStillGood didn't fail with disabled challenge")
+	test.Assert(t, !ra.authzValidChallengeEnabled(&core.Authorization{}), "ra.authzValidChallengeEnabled didn't fail with empty authorization")
+	test.Assert(t, !ra.authzValidChallengeEnabled(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusPending}}}), "ra.authzValidChallengeEnabled didn't fail with no valid challenges")
+	test.Assert(t, !ra.authzValidChallengeEnabled(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusValid, Type: core.ChallengeTypeHTTP01}}}), "ra.authzValidChallengeEnabled didn't fail with disabled challenge")
 
-	test.Assert(t, ra.validChallengeStillGood(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusValid, Type: core.ChallengeTypeTLSSNI01}}}), "ra.validChallengeStillGood failed with enabled challenge")
+	test.Assert(t, ra.authzValidChallengeEnabled(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusValid, Type: core.ChallengeTypeTLSSNI01}}}), "ra.authzValidChallengeEnabled failed with enabled challenge")
 }
 
 func TestUpdateAuthorizationBadChallengeType(t *testing.T) {

sa/sa.go

@@ -222,24 +222,10 @@ func (ssa *SQLStorageAuthority) GetAuthorization(ctx context.Context, id string)
 		authz = pa.Authorization
 	}
 
-	var challObjs []challModel
-	_, err = tx.Select(
-		&challObjs,
-		getChallengesQuery,
-		map[string]interface{}{"authID": authz.ID},
-	)
+	authz.Challenges, err = ssa.getChallenges(authz.ID)
 	if err != nil {
-		return authz, Rollback(tx, err)
-	}
-	var challs []core.Challenge
-	for _, c := range challObjs {
-		chall, err := modelToChallenge(&c)
-		if err != nil {
-			return authz, Rollback(tx, err)
-		}
-		challs = append(challs, chall)
+		return authz, err
 	}
-	authz.Challenges = challs
 
 	return authz, tx.Commit()
 }
@@ -1545,26 +1531,11 @@ func (ssa *SQLStorageAuthority) GetOrderAuthorizations(
 		}
 		existing, present := byName[auth.Identifier.Value]
 		if !present || auth.Expires.After(*existing.Expires) {
-
-			// Retrieve challenges for the authzvar challObjs []challModel
-			var challObjs []challModel
-			_, err = ssa.dbMap.Select(
-				&challObjs,
-				getChallengesQuery,
-				map[string]interface{}{"authID": auth.ID},
-			)
+			// Retrieve challenges for the authz
+			auth.Challenges, err = ssa.getChallenges(auth.ID)
 			if err != nil {
 				return nil, err
 			}
-			var challs []core.Challenge
-			for _, c := range challObjs {
-				chall, err := modelToChallenge(&c)
-				if err != nil {
-					return nil, err
-				}
-				challs = append(challs, chall)
-			}
-			auth.Challenges = challs
 
 			byName[auth.Identifier.Value] = auth
 		}
@@ -1648,25 +1619,8 @@ func (ssa *SQLStorageAuthority) getAuthorizations(ctx context.Context, table str
 		}
 		existing, present := byName[auth.Identifier.Value]
 		if !present || auth.Expires.After(*existing.Expires) {
-			// Retrieve challenges for the authzvar challObjs []challModel
-			var challObjs []challModel
-			_, err = ssa.dbMap.Select(
-				&challObjs,
-				getChallengesQuery,
-				map[string]interface{}{"authID": auth.ID},
-			)
-			if err != nil {
-				return nil, err
-			}
-			var challs []core.Challenge
-			for _, c := range challObjs {
-				chall, err := modelToChallenge(&c)
-				if err != nil {
-					return nil, err
-				}
-				challs = append(challs, chall)
-			}
-			auth.Challenges = challs
+			// Retrieve challenges for the authz
+			auth.Challenges, err = ssa.getChallenges(auth.ID)
 
 			byName[auth.Identifier.Value] = auth
 		}
@@ -1734,23 +1688,7 @@ func (ssa *SQLStorageAuthority) GetAuthorizations(ctx context.Context, req *sapb
 	if features.Enabled(features.WildcardDomains) {
 		// Fetch each of the authorizations' associated challenges
 		for _, authz := range authzMap {
-			var challObjs []challModel
-			_, err = ssa.dbMap.Select(
-				&challObjs,
-				getChallengesQuery,
-				map[string]interface{}{"authID": authz.ID},
-			)
-			if err != nil {
-				return nil, err
-			}
-			authz.Challenges = make([]core.Challenge, len(challObjs))
-			for i, c := range challObjs {
-				chall, err := modelToChallenge(&c)
-				if err != nil {
-					return nil, err
-				}
-				authz.Challenges[i] = chall
-			}
+			authz.Challenges, err = ssa.getChallenges(authz.ID)
 		}
 	}
 	return authzMapToPB(authzMap)
@@ -1772,3 +1710,24 @@ func (ssa *SQLStorageAuthority) AddPendingAuthorizations(ctx context.Context, re
 	}
 	return &sapb.AuthorizationIDs{Ids: ids}, nil
 }
+
+func (ssa *SQLStorageAuthority) getChallenges(authID string) ([]core.Challenge, error) {
+	var challObjs []challModel
+	_, err := ssa.dbMap.Select(
+		&challObjs,
+		getChallengesQuery,
+		map[string]interface{}{"authID": authID},
+	)
+	if err != nil {
+		return nil, err
+	}
+	var challs []core.Challenge
+	for _, c := range challObjs {
+		chall, err := modelToChallenge(&c)
+		if err != nil {
+			return nil, err
+		}
+		challs = append(challs, chall)
+	}
+	return challs, nil
+}