Check safe browsing at validation time (#3539)

Right now we check safe browsing at new-authz time, which introduces a possible
external dependency when calling new-authz. This is usually fine, since most safe
browsing checks can be satisfied locally, but when requests have to go external,
it can create variance in new-authz timing.

Fixes #3491.

features/featureflag_string.go

@@ -4,9 +4,9 @@ package features
 
 import "strconv"
 
-const _FeatureFlag_name = "unusedUseAIAIssuerURLReusePendingAuthzCountCertificatesExactIPv6FirstAllowRenewalFirstRLWildcardDomainsEnforceChallengeDisableTLSSNIRevalidationCancelCTSubmissionsEnforceV2ContentType"
+const _FeatureFlag_name = "unusedUseAIAIssuerURLReusePendingAuthzCountCertificatesExactIPv6FirstAllowRenewalFirstRLWildcardDomainsEnforceChallengeDisableTLSSNIRevalidationCancelCTSubmissionsVAChecksGSBEnforceV2ContentType"
 
-var _FeatureFlag_index = [...]uint8{0, 6, 21, 38, 60, 69, 88, 103, 126, 144, 163, 183}
+var _FeatureFlag_index = [...]uint8{0, 6, 21, 38, 60, 69, 88, 103, 126, 144, 163, 174, 194}
 
 func (i FeatureFlag) String() string {
 	if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) {

features/features.go

@@ -25,6 +25,7 @@ const (
 	// Allow TLS-SNI in new-authz that are revalidating for previous issuance
 	TLSSNIRevalidation
 	CancelCTSubmissions
+	VAChecksGSB
 	// Return errors to ACMEv2 clients that do not send the correct JWS
 	// Content-Type header
 	EnforceV2ContentType
@@ -42,6 +43,7 @@ var features = map[FeatureFlag]bool{
 	EnforceChallengeDisable: false, // deprecated
 	TLSSNIRevalidation:      false,
 	CancelCTSubmissions:     true,
+	VAChecksGSB:             false,
 	EnforceV2ContentType:    false,
 }
 

ra/ra.go

@@ -1840,7 +1840,7 @@ func (ra *RegistrationAuthorityImpl) createPendingAuthz(ctx context.Context, reg
 		Expires:        &expires,
 	}
 
-	if identifier.Type == core.IdentifierDNS {
+	if identifier.Type == core.IdentifierDNS && !features.Enabled(features.VAChecksGSB) {
 		isSafeResp, err := ra.VA.IsSafeDomain(ctx, &vaPB.IsSafeDomainRequest{Domain: &identifier.Value})
 		if err != nil {
 			outErr := berrors.InternalServerError("unable to determine if domain was safe")

test/config-next/ra.json

@@ -45,6 +45,7 @@
       "WildcardDomains": true,
       "TLSSNIRevalidation": true,
       "ReusePendingAuthz": true,
+      "VAChecksGSB": true,
       "CancelCTSubmissions": false
     },
     "CTLogGroups2": [

test/config-next/va.json

@@ -27,6 +27,7 @@
       "ServerURL": "http://boulder:6000"
     },
     "features": {
+      "VAChecksGSB": true,
       "IPv6First": true
     },
     "remoteVAs": [

va/caa_test.go

@@ -336,7 +336,10 @@ func TestCAAFailure(t *testing.T) {
 	va, _ := setup(hs, 0)
 	va.dnsClient = caaMockDNS{}
 
-	_, prob := va.validateChallengeAndCAA(ctx, dnsi("reserved.com"), chall)
+	_, prob := va.validateChallengeAndIdentifier(ctx, dnsi("reserved.com"), chall)
+	if prob == nil {
+		t.Fatalf("Expected CAA rejection for reserved.com, got success")
+	}
 	test.AssertEquals(t, prob.Type, probs.CAAProblem)
 }
 

va/gsb.go

@@ -32,31 +32,35 @@ func (va *ValidationAuthorityImpl) IsSafeDomain(ctx context.Context, req *vaPB.I
 	if req == nil || req.Domain == nil {
 		return nil, bgrpc.ErrMissingParameters
 	}
+	status := va.isSafeDomain(ctx, *req.Domain)
+	return &vaPB.IsDomainSafe{IsSafe: &status}, nil
+}
+
+// isSafeDomain returns true if the VA considers the given domain safe. If the
+// backend errors, we consider the domain safe, so this function never returns
+// error.
+func (va *ValidationAuthorityImpl) isSafeDomain(ctx context.Context, domain string) bool {
 	stats := va.stats.NewScope("IsSafeDomain")
 	stats.Inc("IsSafeDomain.Requests", 1)
 	if va.safeBrowsing == nil {
 		stats.Inc("IsSafeDomain.Skips", 1)
-		status := true
-		return &vaPB.IsDomainSafe{IsSafe: &status}, nil
+		return true
 	}
 
-	var status bool
-	list, err := va.safeBrowsing.IsListed(ctx, *req.Domain)
+	list, err := va.safeBrowsing.IsListed(ctx, domain)
 	if err != nil {
 		stats.Inc("IsSafeDomain.Errors", 1)
 		// In the event of an error checking the GSB status we allow the domain in
 		// question to be treated as safe to avoid coupling the availability of the
 		// VA to the GSB API. This is acceptable for Let's Encrypt because we do not
 		// have a hard commitment to GSB filtering in our CP/CPS.
-		status = true
-	} else {
-		stats.Inc("IsSafeDomain.Successes", 1)
-		status = (list == "")
-		if status {
-			stats.Inc("IsSafeDomain.Status.Good", 1)
-		} else {
-			stats.Inc("IsSafeDomain.Status.Bad", 1)
-		}
+		return true
 	}
-	return &vaPB.IsDomainSafe{IsSafe: &status}, nil
+	stats.Inc("IsSafeDomain.Successes", 1)
+	if list == "" {
+		stats.Inc("IsSafeDomain.Status.Good", 1)
+		return true
+	}
+	stats.Inc("IsSafeDomain.Status.Bad", 1)
+	return false
 }

va/va.go

@@ -717,11 +717,11 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, identifier
 		invalidRecord, andMore, challengeSubdomain))
 }
 
-// validateChallengeAndCAA performs a challenge validation and CAA validation
-// for the provided identifier and a corresponding challenge. If the validation
-// or CAA lookup fail a problem is returned along with the validation records
-// created during the validation attempt.
-func (va *ValidationAuthorityImpl) validateChallengeAndCAA(
+// validateChallengeAndIdentifier performs a challenge validation and, in parallel,
+// checks CAA and GSB for the identifier. If any of those steps fails, it
+// returns a ProblemDetails plus the validation records created during the
+// validation attempt.
+func (va *ValidationAuthorityImpl) validateChallengeAndIdentifier(
 	ctx context.Context,
 	identifier core.AcmeIdentifier,
 	challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) {
@@ -738,10 +738,19 @@ func (va *ValidationAuthorityImpl) validateChallengeAndCAA(
 	// va.checkCAA accepts wildcard identifiers and handles them appropriately so
 	// we can dispatch `checkCAA` with the provided `identifier` instead of
 	// `baseIdentifier`
-	ch := make(chan *probs.ProblemDetails, 1)
+	ch := make(chan *probs.ProblemDetails, 2)
 	go func() {
 		ch <- va.checkCAA(ctx, identifier)
 	}()
+	go func() {
+		if features.Enabled(features.VAChecksGSB) && !va.isSafeDomain(ctx, baseIdentifier.Value) {
+			ch <- probs.Unauthorized(fmt.Sprintf(
+				"%q was considered an unsafe domain by a third-party API",
+				baseIdentifier.Value))
+		} else {
+			ch <- nil
+		}
+	}()
 
 	// TODO(#1292): send into another goroutine
 	validationRecords, err := va.validateChallenge(ctx, baseIdentifier, challenge)
@@ -749,9 +758,11 @@ func (va *ValidationAuthorityImpl) validateChallengeAndCAA(
 		return validationRecords, err
 	}
 
-	caaProblem := <-ch
-	if caaProblem != nil {
-		return validationRecords, caaProblem
+	for i := 0; i < cap(ch); i++ {
+		extraProblem := <-ch
+		if extraProblem != nil {
+			return validationRecords, extraProblem
+		}
 	}
 	return validationRecords, nil
 }
@@ -848,7 +859,7 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, domain
 		go va.performRemoteValidation(ctx, domain, challenge, authz, remoteError)
 	}
 
-	records, prob := va.validateChallengeAndCAA(
+	records, prob := va.validateChallengeAndIdentifier(
 		ctx,
 		core.AcmeIdentifier{Type: "dns", Value: domain},
 		challenge)

va/va_test.go

@@ -682,6 +682,44 @@ func TestValidateHTTP(t *testing.T) {
 	test.Assert(t, prob == nil, "validation failed")
 }
 
+func TestGSBAtValidation(t *testing.T) {
+	chall := core.HTTPChallenge01()
+	setChallengeToken(&chall, core.NewToken())
+
+	hs := httpSrv(t, chall.Token)
+	defer hs.Close()
+
+	va, _ := setup(hs, 0)
+
+	_ = features.Set(map[string]bool{"VAChecksGSB": true})
+	defer features.Reset()
+
+	ctrl := gomock.NewController(t)
+	sbc := NewMockSafeBrowsing(ctrl)
+	sbc.EXPECT().IsListed(gomock.Any(), "good.com").Return("", nil)
+	sbc.EXPECT().IsListed(gomock.Any(), "bad.com").Return("bad", nil)
+	sbc.EXPECT().IsListed(gomock.Any(), "errorful.com").Return("", fmt.Errorf("welp"))
+	va.safeBrowsing = sbc
+
+	_, prob := va.validateChallengeAndIdentifier(ctx, dnsi("bad.com"), chall)
+	if prob == nil {
+		t.Fatalf("Expected rejection for bad.com, got success")
+	}
+	if !strings.Contains(prob.Error(), "unsafe domain") {
+		t.Errorf("Got error %q, expected an unsafe domain error.", prob.Error())
+	}
+
+	_, prob = va.validateChallengeAndIdentifier(ctx, dnsi("errorful.com"), chall)
+	if prob != nil {
+		t.Fatalf("Expected success for errorful.com, got error")
+	}
+
+	_, prob = va.validateChallengeAndIdentifier(ctx, dnsi("good.com"), chall)
+	if prob != nil {
+		t.Fatalf("Expected success for good.com, got %s", prob)
+	}
+}
+
 // challengeType == "tls-sni-00" or "dns-00", since they're the same
 func createChallenge(challengeType string) core.Challenge {
 	chall := core.Challenge{