Add CAAAfterValidation feature flag (#7082)

Add a new feature flag "CAAAfterValidation" which, when set to true in
the VA, causes the VA to only begin CAA checks after basic domain
control validation has completed successfully. This will make successful
validations take longer, since the DCV and CAA checks are performed
serially instead of in parallel. However, it will also reduce the number
of CAA checks we perform by up to 80%, since such a high percentage of
validations also fail.

IN-9575 tracks enabling this feature flag in staging and prod
Fixes #7058

features/features.go

@@ -74,6 +74,12 @@ const (
 	// According to the BRs Section 7.1.4.2.2(a), the commonName field is
 	// Deprecated, and its inclusion is discouraged but not (yet) prohibited.
 	RequireCommonName
+
+	// CAAAfterValidation causes the VA to only kick off CAA checks after the base
+	// domain control validation has completed and succeeded. This makes
+	// successful validations slower by serializing the DCV and CAA work, but
+	// makes unsuccessful validations easier by not doing CAA work at all.
+	CAAAfterValidation
 )
 
 // List of features and their default value, protected by fMu
@@ -96,6 +102,7 @@ var features = map[FeatureFlag]bool{
 	AsyncFinalize:                     false,
 	RequireCommonName:                 true,
 	LeaseCRLShards:                    false,
+	CAAAfterValidation:                false,
 
 	StoreLintingCertificateInsteadOfPrecertificate: false,
 }

test/config-next/va-remote-a.json

@@ -34,7 +34,9 @@
 				}
 			}
 		},
-		"features": {},
+		"features": {
+			"CAAAfterValidation": true
+		},
 		"accountURIPrefixes": [
 			"http://boulder.service.consul:4000/acme/reg/",
 			"http://boulder.service.consul:4001/acme/acct/"

test/config-next/va-remote-b.json

@@ -34,7 +34,9 @@
 				}
 			}
 		},
-		"features": {},
+		"features": {
+			"CAAAfterValidation": true
+		},
 		"accountURIPrefixes": [
 			"http://boulder.service.consul:4000/acme/reg/",
 			"http://boulder.service.consul:4001/acme/acct/"

test/config-next/va.json

@@ -41,7 +41,8 @@
 		},
 		"features": {
 			"EnforceMultiVA": true,
-			"MultiVAFullResults": true
+			"MultiVAFullResults": true,
+			"CAAAfterValidation": true
 		},
 		"remoteVAs": [
 			{

va/va.go

@@ -378,17 +378,21 @@ func (va *ValidationAuthorityImpl) validate(
 		baseIdentifier.Value = strings.TrimPrefix(identifier.Value, "*.")
 	}
 
-	// va.checkCAA accepts wildcard identifiers and handles them appropriately so
-	// we can dispatch `checkCAA` with the provided `identifier` instead of
-	// `baseIdentifier`
+	// Create this channel outside of the feature-conditional block so that it is
+	// also in scope for the matching block below.
 	ch := make(chan *probs.ProblemDetails, 1)
-	go func() {
-		params := &caaParams{
-			accountURIID:     regid,
-			validationMethod: challenge.Type,
-		}
-		ch <- va.checkCAA(ctx, identifier, params)
-	}()
+	if !features.Enabled(features.CAAAfterValidation) {
+		// va.checkCAA accepts wildcard identifiers and handles them appropriately so
+		// we can dispatch `checkCAA` with the provided `identifier` instead of
+		// `baseIdentifier`
+		go func() {
+			params := &caaParams{
+				accountURIID:     regid,
+				validationMethod: challenge.Type,
+			}
+			ch <- va.checkCAA(ctx, identifier, params)
+		}()
+	}
 
 	// TODO(#1292): send into another goroutine
 	validationRecords, prob := va.validateChallenge(ctx, baseIdentifier, challenge)
@@ -401,11 +405,23 @@ func (va *ValidationAuthorityImpl) validate(
 		return validationRecords, prob
 	}
 
-	for i := 0; i < cap(ch); i++ {
-		if extraProblem := <-ch; extraProblem != nil {
-			return validationRecords, extraProblem
+	if !features.Enabled(features.CAAAfterValidation) {
+		for i := 0; i < cap(ch); i++ {
+			if extraProblem := <-ch; extraProblem != nil {
+				return validationRecords, extraProblem
+			}
+		}
+	} else {
+		params := &caaParams{
+			accountURIID:     regid,
+			validationMethod: challenge.Type,
+		}
+		prob := va.checkCAA(ctx, identifier, params)
+		if prob != nil {
+			return validationRecords, prob
 		}
 	}
+
 	return validationRecords, nil
 }
 

va/va_test.go

@@ -310,6 +310,42 @@ func TestPerformValidationWildcard(t *testing.T) {
 	}
 }
 
+func TestDCVAndCAASequencing(t *testing.T) {
+	va, mockLog := setup(nil, 0, "", nil)
+
+	// When performing validation without the CAAAfterValidation flag, CAA should
+	// be checked.
+	req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01)
+	res, err := va.PerformValidation(context.Background(), req)
+	test.AssertNotError(t, err, "performing validation")
+	test.Assert(t, res.Problems == nil, fmt.Sprintf("validation failed: %#v", res.Problems))
+	caaLog := mockLog.GetAllMatching(`Checked CAA records for`)
+	test.AssertEquals(t, len(caaLog), 1)
+
+	_ = features.Set(map[string]bool{features.CAAAfterValidation.String(): true})
+	defer features.Reset()
+
+	// When performing successful validation with the CAAAfterValidation flag,
+	// CAA should be checked.
+	mockLog.Clear()
+	req = createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01)
+	res, err = va.PerformValidation(context.Background(), req)
+	test.AssertNotError(t, err, "performing validation")
+	test.Assert(t, res.Problems == nil, fmt.Sprintf("validation failed: %#v", res.Problems))
+	caaLog = mockLog.GetAllMatching(`Checked CAA records for`)
+	test.AssertEquals(t, len(caaLog), 1)
+
+	// When performing failed validation with the CAAAfterValidation flag,
+	// CAA should be skipped
+	mockLog.Clear()
+	req = createValidationRequest("bad-dns01.com", core.ChallengeTypeDNS01)
+	res, err = va.PerformValidation(context.Background(), req)
+	test.AssertNotError(t, err, "performing validation")
+	test.Assert(t, res.Problems != nil, "validation succeeded")
+	caaLog = mockLog.GetAllMatching(`Checked CAA records for`)
+	test.AssertEquals(t, len(caaLog), 0)
+}
+
 func TestMultiVA(t *testing.T) {
 	// Create a new challenge to use for the httpSrv
 	req := createValidationRequest("localhost", core.ChallengeTypeHTTP01)