From cf9df961ba16d70cd22cee36853936fea5363791 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 15 Apr 2022 12:14:00 -0700 Subject: [PATCH] Add feature flags for upcoming deprecations (#6043) This adds three features flags: SHA1CSRs, OldTLSOutbound, and OldTLSInbound. Each controls the behavior of an upcoming deprecation (except OldTLSInbound, which isn't yet scheduled for a deprecation but will be soon). Note that these feature flags take advantage of `features`' default values, so they can default to "true" (that is, each of these features is enabled by default), and we set them to "false" in the config JSON to turn them off when the time comes. The unittest for OldTLSOutbound requires that `example.com` resolves to 127.0.0.1. This is because there's logic in the VA that checks that redirected-to hosts end in an IANA TLD. The unittest relies on redirecting, and we can't use e.g. `localhost` in it because of that TLD check, so we use example.com. Fixes #6036 and #6037 --- csr/csr.go | 4 ++ csr/csr_test.go | 44 ++++++++++++++ docker-compose.yml | 4 ++ features/featureflag_string.go | 7 ++- features/features.go | 14 +++++ va/http.go | 7 +++ va/http_test.go | 102 +++++++++++++++++++++++++++++++++ web/send_error.go | 2 + wfe2/wfe.go | 7 +++ wfe2/wfe_test.go | 26 +++++++++ 10 files changed, 215 insertions(+), 2 deletions(-) diff --git a/csr/csr.go b/csr/csr.go index a960d17ab..da6037843 100644 --- a/csr/csr.go +++ b/csr/csr.go @@ -9,6 +9,7 @@ import ( "github.com/letsencrypt/boulder/core" berrors "github.com/letsencrypt/boulder/errors" + "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/goodkey" "github.com/letsencrypt/boulder/identifier" ) @@ -62,6 +63,9 @@ func VerifyCSR(ctx context.Context, csr *x509.CertificateRequest, maxNames int, if !goodSignatureAlgorithms[csr.SignatureAlgorithm] { return unsupportedSigAlg } + if !features.Enabled(features.SHA1CSRs) && csr.SignatureAlgorithm == x509.SHA1WithRSA { + return unsupportedSigAlg + } err = csr.CheckSignature() if err != nil { return invalidSig diff --git a/csr/csr_test.go b/csr/csr_test.go index d174cad41..36fed96ff 100644 --- a/csr/csr_test.go +++ b/csr/csr_test.go @@ -13,6 +13,7 @@ import ( "github.com/letsencrypt/boulder/core" berrors "github.com/letsencrypt/boulder/errors" + "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/goodkey" "github.com/letsencrypt/boulder/identifier" "github.com/letsencrypt/boulder/test" @@ -215,3 +216,46 @@ func TestNormalizeCSR(t *testing.T) { }) } } + +func TestSHA1Deprecation(t *testing.T) { + features.Reset() + + private, err := rsa.GenerateKey(rand.Reader, 2048) + test.AssertNotError(t, err, "error generating test key") + + makeAndVerifyCsr := func(alg x509.SignatureAlgorithm) error { + csrBytes, err := x509.CreateCertificateRequest(rand.Reader, + &x509.CertificateRequest{ + DNSNames: []string{"example.com"}, + SignatureAlgorithm: alg, + PublicKey: &private.PublicKey, + }, private) + if err != nil { + t.Fatal(err) + } + csr, err := x509.ParseCertificateRequest(csrBytes) + if err != nil { + t.Fatal(err) + } + return VerifyCSR(context.Background(), csr, 100, testingPolicy, &mockPA{}) + } + + err = makeAndVerifyCsr(x509.SHA256WithRSA) + if err != nil { + t.Fatalf("expected no error from VerifyCSR on a CSR signed with SHA256, got %s", err) + } + err = features.Set(map[string]bool{"SHA1CSRs": true}) + test.AssertNotError(t, err, "setting feature") + err = makeAndVerifyCsr(x509.SHA1WithRSA) + if err != nil { + t.Fatalf("(SHA1CSR == true) expected no error from VerifyCSR on a CSR signed with SHA1, got %s (maybe set GODEBUG=x509sha1=1)", err) + } + + err = features.Set(map[string]bool{"SHA1CSRs": false}) + test.AssertNotError(t, err, "setting feature") + t.Logf("enabled %t\n", features.Enabled(features.SHA1CSRs)) + err = makeAndVerifyCsr(x509.SHA1WithRSA) + if err == nil { + t.Fatalf("(SHA1CSR == false) expected error from VerifyCSR on a CSR signed with SHA1, got none") + } +} diff --git a/docker-compose.yml b/docker-compose.yml index 5892b1728..b290d358c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -27,6 +27,10 @@ services: ipv4_address: 10.88.88.88 redisnet: ipv4_address: 10.33.33.33 + extra_hosts: + # This is used by TestOldTLS in va/http_test.go + # TODO(#6011): Remove once TLS 1.0 and 1.1 support is gone. + - "example.com:127.0.0.1" # Use sd-test-srv as a backup to Docker's embedded DNS server # (https://docs.docker.com/config/containers/container-networking/#dns-services). # If there's a name Docker's DNS server doesn't know about, it will diff --git a/features/featureflag_string.go b/features/featureflag_string.go index b3b68b705..1716dcb69 100644 --- a/features/featureflag_string.go +++ b/features/featureflag_string.go @@ -31,11 +31,14 @@ func _() { _ = x[CheckFailedAuthorizationsFirst-20] _ = x[AllowReRevocation-21] _ = x[MozRevocationReasons-22] + _ = x[OldTLSOutbound-23] + _ = x[OldTLSInbound-24] + _ = x[SHA1CSRs-25] } -const _FeatureFlag_name = "unusedPrecertificateRevocationStripDefaultSchemePortNonCFSSLSignerStoreIssuerInfoStreamlineOrderAndAuthzsV1DisableNewValidationsCAAValidationMethodsCAAAccountURIEnforceMultiVAMultiVAFullResultsMandatoryPOSTAsGETAllowV1RegistrationStoreRevokerInfoRestrictRSAKeySizesFasterNewOrdersRateLimitECDSAForAllServeRenewalInfoGetAuthzReadOnlyGetAuthzUseIndexCheckFailedAuthorizationsFirstAllowReRevocationMozRevocationReasons" +const _FeatureFlag_name = "unusedPrecertificateRevocationStripDefaultSchemePortNonCFSSLSignerStoreIssuerInfoStreamlineOrderAndAuthzsV1DisableNewValidationsCAAValidationMethodsCAAAccountURIEnforceMultiVAMultiVAFullResultsMandatoryPOSTAsGETAllowV1RegistrationStoreRevokerInfoRestrictRSAKeySizesFasterNewOrdersRateLimitECDSAForAllServeRenewalInfoGetAuthzReadOnlyGetAuthzUseIndexCheckFailedAuthorizationsFirstAllowReRevocationMozRevocationReasonsOldTLSOutboundOldTLSInboundSHA1CSRs" -var _FeatureFlag_index = [...]uint16{0, 6, 30, 52, 66, 81, 105, 128, 148, 161, 175, 193, 211, 230, 246, 265, 289, 300, 316, 332, 348, 378, 395, 415} +var _FeatureFlag_index = [...]uint16{0, 6, 30, 52, 66, 81, 105, 128, 148, 161, 175, 193, 211, 230, 246, 265, 289, 300, 316, 332, 348, 378, 395, 415, 429, 442, 450} 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 4608d1d63..52fd67bfb 100644 --- a/features/features.go +++ b/features/features.go @@ -77,6 +77,17 @@ const ( // with the certificate's keypair, the cert will be revoked with reason // keyCompromise, regardless of what revocation reason they request. MozRevocationReasons + // OldTLSOutbound allows the VA to negotiate TLS 1.0 and TLS 1.1 during + // HTTPS redirects. When it is set to false, the VA will only connect to + // HTTPS servers that support TLS 1.2 or above. + OldTLSOutbound + // OldTLSInbound controls whether the WFE rejects inbound requests using + // TLS 1.0 and TLS 1.1. Because WFE does not terminate TLS in production, + // we rely on the TLS-Version header (set by our reverse proxy). + OldTLSInbound + // SHA1CSRs controls whether the /acme/finalize endpoint rejects CSRs that + // are self-signed using SHA1. + SHA1CSRs ) // List of features and their default value, protected by fMu @@ -104,6 +115,9 @@ var features = map[FeatureFlag]bool{ CheckFailedAuthorizationsFirst: false, AllowReRevocation: false, MozRevocationReasons: false, + OldTLSOutbound: true, + OldTLSInbound: true, + SHA1CSRs: true, } var fMu = new(sync.RWMutex) diff --git a/va/http.go b/va/http.go index aa3485cfd..e7629ef06 100644 --- a/va/http.go +++ b/va/http.go @@ -17,6 +17,7 @@ import ( "github.com/letsencrypt/boulder/core" berrors "github.com/letsencrypt/boulder/errors" + "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/iana" "github.com/letsencrypt/boulder/identifier" "github.com/letsencrypt/boulder/probs" @@ -634,6 +635,12 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( // TODO(#6011): Remove once TLS 1.0 and 1.1 support is gone. if httpResponse.TLS != nil && httpResponse.TLS.Version < tls.VersionTLS12 { oldTLS = true + if !features.Enabled(features.OldTLSOutbound) { + return nil, records, berrors.MalformedError( + "validation attempt was redirected to an HTTPS server that doesn't " + + "support TLSv1.2 or better. See " + + "https://community.letsencrypt.org/t/rejecting-sha-1-csrs-and-validation-using-tls-1-0-1-1-urls/175144") + } } if oldTLS { diff --git a/va/http_test.go b/va/http_test.go index e70facbef..dc09b27a4 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -3,9 +3,11 @@ package va import ( "bytes" "context" + "crypto/tls" "encoding/base64" "errors" "fmt" + "io/ioutil" mrand "math/rand" "net" "net/http" @@ -20,6 +22,7 @@ import ( "github.com/letsencrypt/boulder/bdns" "github.com/letsencrypt/boulder/core" berrors "github.com/letsencrypt/boulder/errors" + "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/identifier" "github.com/letsencrypt/boulder/probs" "github.com/letsencrypt/boulder/test" @@ -1545,3 +1548,102 @@ func TestLimitedReader(t *testing.T) { t.Errorf("Problem Detail contained an invalid UTF-8 string") } } + +// oldTLSRedirectSrv returns a pair of *httptest.Servers. The first one is +// plaintext and redirects all URLs to the second one. The second one is HTTPS +// and supports a max TLS version of 1.1. +// TODO(#6011): Remove once TLS 1.0 and 1.1 support is gone. +func oldTLSRedirectSrv(t *testing.T, token string) (*httptest.Server, *httptest.Server) { + var port int + m := http.NewServeMux() + addrs, err := net.LookupHost("example.com") + if err != nil { + t.Fatal(err) + } + if len(addrs) < 1 || addrs[0] != "127.0.0.1" { + t.Fatalf("this test requires example.com to resolve to 127.0.0.1 but it resolved to %s", addrs[0]) + } + m.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, fmt.Sprintf("https://example.com:%d%s", port, r.URL.Path), http.StatusMovedPermanently) + }) + + server := httptest.NewUnstartedServer(m) + server.Start() + + httpsMux := http.NewServeMux() + httpsMux.HandleFunc("/.well-known/acme-challenge/", func(w http.ResponseWriter, r *http.Request) { + pathComponents := strings.Split(r.URL.Path, "/") + if len(pathComponents) < 4 || pathComponents[3] != token { + http.NotFound(w, r) + return + } + + ch := core.Challenge{Token: token} + keyAuthz, _ := ch.ExpectedKeyAuthorization(accountKey) + fmt.Fprint(w, keyAuthz) + }) + httpsServer := httptest.NewUnstartedServer(httpsMux) + httpsServer.TLS = &tls.Config{ + MaxVersion: tls.VersionTLS11, + } + httpsServer.StartTLS() + + port = getPort(httpsServer) + + return server, httpsServer +} + +// TODO(#6011): Remove once TLS 1.0 and 1.1 support is gone. +func TestOldTLS(t *testing.T) { + features.Reset() + + chall := httpChallenge() + server, httpsServer := oldTLSRedirectSrv(t, chall.Token) + startURL := server.URL + + // Check that the HTTP servers are running as expected + c := http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + }, + }, + } + resp, err := c.Get(startURL + "/.well-known/acme-challenge/" + chall.Token) + if err != nil { + t.Fatal(err) + } + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(body), chall.Token) { + t.Fatal("response did not contain token") + } + if resp.TLS == nil { + t.Fatal("request did not redirect to HTTPS") + } + if resp.TLS.Version > tls.VersionTLS11 { + t.Fatalf("HTTPS request negotiated TLS version 0x%x (test expected 1.1). Try setting GODEBUG=tls10default=1", + resp.TLS.Version) + } + + // The real test + va, _ := setup(server, 0, "", nil) + va.httpsPort = getPort(httpsServer) + + _, prob := va.validateHTTP01(ctx, dnsi("localhost"), chall) + if prob != nil { + t.Errorf("(OldTLSOutbound == true) expected success, got %s", prob) + } + + err = features.Set(map[string]bool{"OldTLSOutbound": false}) + if err != nil { + t.Fatal(err) + } + + _, prob = va.validateHTTP01(ctx, dnsi("localhost"), chall) + if prob == nil { + t.Error("(OldTLSOutbound == false) expected fail, got success") + } +} diff --git a/web/send_error.go b/web/send_error.go index 87dcfb62f..53b146428 100644 --- a/web/send_error.go +++ b/web/send_error.go @@ -20,6 +20,8 @@ import ( // internal error. // - Prefixes the Type field of the ProblemDetails with a namespace. // - Sends an HTTP response containing the error and an error code to the user. +// The internal error (ierr) may be nil if no information beyond the +// ProblemDetails is needed for internal debugging. func SendError( log blog.Logger, namespace string, diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 0b7d64999..f05f73026 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -233,6 +233,13 @@ func (wfe *WebFrontEndImpl) HandleFunc(mux *http.ServeMux, pattern string, h web logEvent.Slug = request.URL.Path beeline.AddFieldToTrace(ctx, "slug", request.URL.Path) } + if !features.Enabled(features.OldTLSInbound) { + tls := request.Header.Get("TLS-Version") + if tls == "TLSv1" || tls == "TLSv1.1" { + wfe.sendError(response, logEvent, probs.Malformed("upgrade your ACME client to support TLSv1.2 or better"), nil) + return + } + } if request.Method != "GET" || pattern == newNoncePath { // Historically we did not return a error to the client // if we failed to get a new nonce. We preserve that diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 2df130893..7cc4c337d 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -3545,3 +3545,29 @@ func TestARI(t *testing.T) { test.AssertEquals(t, resp.Code, 404) test.AssertEquals(t, resp.Header().Get("Retry-After"), "") } + +// TODO(#6011): Remove once TLS 1.0 and 1.1 support is gone. +func TestOldTLSInbound(t *testing.T) { + features.Reset() + + wfe, _ := setupWFE(t) + req := &http.Request{ + URL: &url.URL{Path: "/directory"}, + Method: "GET", + Header: http.Header(map[string][]string{ + http.CanonicalHeaderKey("TLS-Version"): {"TLSv1"}, + }), + } + responseWriter := httptest.NewRecorder() + wfe.Handler(metrics.NoopRegisterer).ServeHTTP(responseWriter, req) + test.AssertEquals(t, responseWriter.Code, http.StatusOK) + + err := features.Set(map[string]bool{"OldTLSInbound": false}) + if err != nil { + t.Fatal(err) + } + + responseWriter = httptest.NewRecorder() + wfe.Handler(metrics.NoopRegisterer).ServeHTTP(responseWriter, req) + test.AssertEquals(t, responseWriter.Code, http.StatusBadRequest) +}