From cca41a133360187f64742c5d92bbc2fbd6ac3a13 Mon Sep 17 00:00:00 2001 From: schou Date: Fri, 24 Nov 2023 14:40:29 -0500 Subject: [PATCH] add checks for available cipher before setting h2 proto #16850 Signed-off-by: schou --- client/pkg/transport/listener.go | 31 +++++++++++++++-- server/embed/serve.go | 14 ++++++++ tests/e2e/etcd_config_test.go | 58 ++++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 2 deletions(-) diff --git a/client/pkg/transport/listener.go b/client/pkg/transport/listener.go index ec69f4e2a..20f9a5db2 100644 --- a/client/pkg/transport/listener.go +++ b/client/pkg/transport/listener.go @@ -527,8 +527,35 @@ func (info TLSInfo) ServerConfig() (*tls.Config, error) { cfg.ClientCAs = cp } - // "h2" NextProtos is necessary for enabling HTTP2 for go's HTTP server - cfg.NextProtos = []string{"h2"} + // If manually defined ciphers are provided and TLS1.3 is not allowed, the + // list of ciphers may not allow HTTP2. Test for this and disable if this is the case. + // + // TODO: There is a longstanding upstream issue of testing for bad ciphers. + // This test could be simplified in the future once this is resolved: + // https://github.com/golang/go/issues/41068#issuecomment-722549561 + if len(cfg.CipherSuites) > 0 && cfg.MaxVersion > 0 && cfg.MaxVersion < tls.VersionTLS13 { + + // Test for the cases where TLS1.3 is not allowed, and thus http2 may not + // be possible when select cipherSuites are defined. + var hasHTTP2cipher bool + for _, c := range cfg.CipherSuites { + switch c { + // If one of these cipher suites are defined, then the h2 protocol is possible. + case tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256: + hasHTTP2cipher = true + break + } + } + if hasHTTP2cipher { + cfg.NextProtos = []string{"h2"} + } else { + // Display a warning that none of the h2 ciphers are available and likewise GRBC cannot be started. + info.Logger.Info("CipherSuites is missing an HTTP2 capable cipher (TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 or TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256). Disabling HTTP2 protocol.") + } + } else { + // "h2" NextProtos is necessary for enabling HTTP2 for go's HTTP server + cfg.NextProtos = []string{"h2"} + } return cfg, nil } diff --git a/server/embed/serve.go b/server/embed/serve.go index 8b64cd77e..199b6c1f4 100644 --- a/server/embed/serve.go +++ b/server/embed/serve.go @@ -16,6 +16,7 @@ package embed import ( "context" + "errors" "fmt" "io" defaultLog "log" @@ -202,6 +203,19 @@ func (sctx *serveCtx) serve( } if grpcEnabled { + // Verify that HTTP2 is available, and if not terminate early + var hasH2 bool + for _, p := range tlscfg.NextProtos { + if p == "h2" { + hasH2 = true + } + } + if !hasH2 { + grpcStartError := errors.New("GRPC cannot be started without HTTP2 protocol. Please add at least one of the HTTP2 capable ciphers or set tls-max-version to TLS1.3.") + sctx.lg.Error("Configure https server failed", zap.Error(grpcStartError)) + return grpcStartError + } + gs = v3rpc.Server(s, tlscfg, nil, gopts...) v3electionpb.RegisterElectionServer(gs, servElection) v3lockpb.RegisterLockServer(gs, servLock) diff --git a/tests/e2e/etcd_config_test.go b/tests/e2e/etcd_config_test.go index b9b1a51bd..45260eaef 100644 --- a/tests/e2e/etcd_config_test.go +++ b/tests/e2e/etcd_config_test.go @@ -452,3 +452,61 @@ func TestEtcdTLSVersion(t *testing.T) { proc.Wait() // ensure the port has been released proc.Close() } + +func TestEtcdNonH2CiphersWithGRPC(t *testing.T) { + e2e.BeforeTest(t) + + d := t.TempDir() + argsWithGRPCandH2andWithoutH2Ciphers := []string{ + e2e.BinPath.Etcd, + "--data-dir", d, + "--name", "e1", + "--enable-grpc-gateway", + "--listen-client-urls", "https://0.0.0.0:0", + "--advertise-client-urls", "https://0.0.0.0:0", + "--listen-peer-urls", fmt.Sprintf("https://127.0.0.1:%d", e2e.EtcdProcessBasePort), + "--initial-advertise-peer-urls", fmt.Sprintf("https://127.0.0.1:%d", e2e.EtcdProcessBasePort), + "--initial-cluster", fmt.Sprintf("e1=https://127.0.0.1:%d", e2e.EtcdProcessBasePort), + "--peer-cert-file", e2e.CertPath, + "--peer-key-file", e2e.PrivateKeyPath, + "--cert-file", e2e.CertPath2, + "--key-file", e2e.PrivateKeyPath2, + "--cipher-suites", "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256", + + "--tls-min-version", "TLS1.2", + "--tls-max-version", "TLS1.2", + } + err := e2e.SpawnWithExpect(argsWithGRPCandH2andWithoutH2Ciphers, expect.ExpectedResponse{Value: "http2: TLSConfig.CipherSuites is missing an HTTP/2-required AES_128_GCM_SHA256 cipher (need at least one of TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 or TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256)"}) + require.ErrorContains(t, err, "GRPC cannot be started without HTTP2 protocol.") +} + +func TestEtcdNonH2CiphersWithoutGRPC(t *testing.T) { + e2e.BeforeTest(t) + + d := t.TempDir() + proc, err := e2e.SpawnCmd( + []string{ + e2e.BinPath.Etcd, + "--data-dir", d, + "--name", "e1", + "--listen-client-http-urls", "https://0.0.0.0:0", + "--listen-peer-urls", fmt.Sprintf("https://127.0.0.1:%d", e2e.EtcdProcessBasePort), + "--initial-advertise-peer-urls", fmt.Sprintf("https://127.0.0.1:%d", e2e.EtcdProcessBasePort), + "--initial-cluster", fmt.Sprintf("e1=https://127.0.0.1:%d", e2e.EtcdProcessBasePort), + "--peer-cert-file", e2e.CertPath, + "--peer-key-file", e2e.PrivateKeyPath, + "--cert-file", e2e.CertPath2, + "--key-file", e2e.PrivateKeyPath2, + "--cipher-suites", "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256", + + "--tls-min-version", "TLS1.2", + "--tls-max-version", "TLS1.2", + }, nil, + ) + assert.NoError(t, err) + assert.NoError(t, e2e.WaitReadyExpectProc(context.TODO(), proc, e2e.EtcdServerReadyLines), "did not receive expected output from etcd process") + assert.NoError(t, proc.Stop()) + + proc.Wait() // ensure the port has been released + proc.Close() +}