Remove EnforceChallengeDisable (letsencrypt#3444)

Removes usage of the `EnforceChallengeDisable` feature, the feature itself is not removed as it is still configured in staging/production, once that is fixed I'll submit another PR removing the actual flag.

This keeps the behavior that when authorizations are retrieved from the SA they have their challenges populated, because that seems to make the most sense to me? It also retains TLS re-validation.

Fixes letsencrypt#3441.

ra/ra.go

@@ -552,7 +552,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.authzValidChallengeEnabled(&populatedAuthz) {
+			if 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)
@@ -1445,23 +1445,18 @@ func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, ba
 	// If TLSSNIRevalidation is enabled, find out whether this was a revalidation
 	// (previous certificate existed) or not. If it is a revalidation, we can
 	// proceed with validation even though the challenge type is currently
-	// disabled, regardless of the EnforceChallengeDisable setting.
-	var previousCertificateExists bool
-	if features.Enabled(features.TLSSNIRevalidation) {
+	// disabled.
+	if !ra.PA.ChallengeTypeEnabled(ch.Type, authz.RegistrationID) && features.Enabled(features.TLSSNIRevalidation) {
 		existsResp, err := ra.SA.PreviousCertificateExists(ctx, &sapb.PreviousCertificateExistsRequest{
 			Domain: &authz.Identifier.Value,
 			RegID:  &authz.RegistrationID,
 		})
 		if err != nil {
 			return core.Authorization{}, err
 		}
-		previousCertificateExists = *existsResp.Exists
-	}
-
-	if features.Enabled(features.EnforceChallengeDisable) &&
-		!previousCertificateExists &&
-		!ra.PA.ChallengeTypeEnabled(ch.Type, authz.RegistrationID) {
-		return core.Authorization{}, berrors.MalformedError("challenge type %q no longer allowed", ch.Type)
+		if !*existsResp.Exists {
+			return core.Authorization{}, berrors.MalformedError("challenge type %q no longer allowed", ch.Type)
+		}
 	}
 
 	// When configured with `reuseValidAuthz` we can expect some clients to try

ra/ra_test.go

@@ -674,22 +674,6 @@ func TestReuseValidAuthorization(t *testing.T) {
 	test.AssertNotError(t, err, "UpdateAuthorization on secondAuthz sni failed")
 	test.AssertEquals(t, finalAuthz.ID, secondAuthz.ID)
 	test.AssertEquals(t, secondAuthz.Status, core.StatusValid)
-
-	// Test that a valid authorization that used a challenge which has been disabled
-	// is not reused
-	_ = features.Set(map[string]bool{"EnforceChallengeDisable": true})
-	pa, err := policy.New(map[string]bool{
-		core.ChallengeTypeHTTP01:   false,
-		core.ChallengeTypeTLSSNI01: true,
-		core.ChallengeTypeDNS01:    true,
-	})
-	test.AssertNotError(t, err, "Couldn't create PA")
-	err = pa.SetHostnamePolicyFile("../test/hostname-policy.json")
-	test.AssertNotError(t, err, "Couldn't set hostname policy")
-	ra.PA = pa
-	newAuthz, err := ra.NewAuthorization(ctx, AuthzRequest, Registration.ID)
-	test.AssertNotError(t, err, "NewAuthorization for secondAuthz failed")
-	test.Assert(t, finalAuthz.ID != newAuthz.ID, "NewAuthorization reused a valid authz with a disabled challenge type")
 }
 
 func TestReusePendingAuthorization(t *testing.T) {
@@ -1172,8 +1156,6 @@ func TestNewOrderRateLimiting(t *testing.T) {
 	_, _, ra, fc, cleanUp := initAuthorities(t)
 	defer cleanUp()
 
-	_ = features.Set(map[string]bool{"EnforceChallengeDisable": true})
-
 	// Create a dummy rate limit config that sets a PendingOrdersPerAccount rate
 	// limit with a very low threshold
 	ra.rlPolicies = &dummyRateLimitConfig{
@@ -3221,8 +3203,7 @@ func TestNewAuthzTLSSNIRevalidation(t *testing.T) {
 		core.ChallengeTypeHTTP01: true,
 	}
 	_ = features.Set(map[string]bool{
-		"EnforceChallengeDisable": true,
-		"TLSSNIRevalidation":      true,
+		"TLSSNIRevalidation": true,
 	})
 	pa, err := policy.New(challenges)
 	test.AssertNotError(t, err, "Couldn't create PA")
@@ -3294,8 +3275,6 @@ func TestValidChallengeStillGood(t *testing.T) {
 	test.AssertNotError(t, err, "Couldn't create PA")
 	ra.PA = pa
 
-	_ = features.Set(map[string]bool{"EnforceChallengeDisable": true})
-
 	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")
@@ -3310,8 +3289,6 @@ func TestUpdateAuthorizationBadChallengeType(t *testing.T) {
 	test.AssertNotError(t, err, "Couldn't create PA")
 	ra.PA = pa
 
-	_ = features.Set(map[string]bool{"EnforceChallengeDisable": true})
-
 	exp := fc.Now().Add(10 * time.Hour)
 	_, err = ra.UpdateAuthorization(context.Background(), core.Authorization{Challenges: []core.Challenge{{Status: core.StatusValid, Type: core.ChallengeTypeTLSSNI01}}, Expires: &exp}, 0, core.Challenge{})
 	test.AssertError(t, err, "ra.UpdateAuthorization allowed a update to a authorization")

sa/sa.go

@@ -1861,12 +1861,10 @@ func (ssa *SQLStorageAuthority) GetValidOrderAuthorizations(
 		}
 		existing, present := byName[auth.Identifier.Value]
 		if !present || auth.Expires.After(*existing.Expires) {
-			if features.Enabled(features.EnforceChallengeDisable) {
-				// Retrieve challenges for the authz
-				auth.Challenges, err = ssa.getChallenges(auth.ID)
-				if err != nil {
-					return nil, err
-				}
+			// Retrieve challenges for the authz
+			auth.Challenges, err = ssa.getChallenges(auth.ID)
+			if err != nil {
+				return nil, err
 			}
 
 			byName[auth.Identifier.Value] = auth
@@ -1977,12 +1975,10 @@ func (ssa *SQLStorageAuthority) getAuthorizations(
 		}
 		existing, present := byName[auth.Identifier.Value]
 		if !present || auth.Expires.After(*existing.Expires) {
-			if features.Enabled(features.EnforceChallengeDisable) {
-				// Retrieve challenges for the authz
-				auth.Challenges, err = ssa.getChallenges(auth.ID)
-				if err != nil {
-					return nil, err
-				}
+			// Retrieve challenges for the authz
+			auth.Challenges, err = ssa.getChallenges(auth.ID)
+			if err != nil {
+				return nil, err
 			}
 
 			byName[auth.Identifier.Value] = auth

test/config-next/ra.json

@@ -46,8 +46,7 @@
       "TLSSNIRevalidation": true,
       "AllowTLS02Challenges": true,
       "CountCertificatesExact": true,
-      "ReusePendingAuthz": true,
-      "EnforceChallengeDisable": true
+      "ReusePendingAuthz": true
     },
     "CTLogGroups": [
 

test/config-next/sa.json

@@ -27,8 +27,7 @@
     },
     "features": {
       "WildcardDomains": true,
-      "AllowRenewalFirstRL": true,
-      "EnforceChallengeDisable": true
+      "AllowRenewalFirstRL": true
     }
   },
 

test/config/ra.json

@@ -42,8 +42,7 @@
     },
     "features": {
       "CountCertificatesExact": true,
-      "ReusePendingAuthz": false,
-      "EnforceChallengeDisable": true
+      "ReusePendingAuthz": false
     }
   },
 

test/config/sa.json

@@ -25,7 +25,6 @@
       ]
     },
     "features": {
-      "EnforceChallengeDisable": true
     }
   },