From 11434650b71e6b01d3070d7d79c9abdeb8747c61 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 9 Mar 2018 03:15:05 -0800 Subject: [PATCH] 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 ++-- features/features.go | 2 ++ ra/ra.go | 2 +- test/config-next/ra.json | 1 + test/config-next/va.json | 1 + va/caa_test.go | 5 ++++- va/gsb.go | 32 +++++++++++++++------------- va/va.go | 31 ++++++++++++++++++--------- va/va_test.go | 38 ++++++++++++++++++++++++++++++++++ 9 files changed, 88 insertions(+), 28 deletions(-) diff --git a/features/featureflag_string.go b/features/featureflag_string.go index 48749fdf6..d21e09000 100644 --- a/features/featureflag_string.go +++ b/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) { diff --git a/features/features.go b/features/features.go index c54cbf396..59663c04f 100644 --- a/features/features.go +++ b/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, } diff --git a/ra/ra.go b/ra/ra.go index 2bf25d740..6004f4fec 100644 --- a/ra/ra.go +++ b/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") diff --git a/test/config-next/ra.json b/test/config-next/ra.json index 3d3f25fb7..1d198a866 100644 --- a/test/config-next/ra.json +++ b/test/config-next/ra.json @@ -45,6 +45,7 @@ "WildcardDomains": true, "TLSSNIRevalidation": true, "ReusePendingAuthz": true, + "VAChecksGSB": true, "CancelCTSubmissions": false }, "CTLogGroups2": [ diff --git a/test/config-next/va.json b/test/config-next/va.json index 1ce391d8d..7e5f5f8a9 100644 --- a/test/config-next/va.json +++ b/test/config-next/va.json @@ -27,6 +27,7 @@ "ServerURL": "http://boulder:6000" }, "features": { + "VAChecksGSB": true, "IPv6First": true }, "remoteVAs": [ diff --git a/va/caa_test.go b/va/caa_test.go index 5f9fdd8a6..d6aa490b3 100644 --- a/va/caa_test.go +++ b/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) } diff --git a/va/gsb.go b/va/gsb.go index 18425abb8..b385e792f 100644 --- a/va/gsb.go +++ b/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 } diff --git a/va/va.go b/va/va.go index 44cbb323f..a8b2b82e9 100644 --- a/va/va.go +++ b/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) diff --git a/va/va_test.go b/va/va_test.go index 1919d119f..7622ceee4 100644 --- a/va/va_test.go +++ b/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{