Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add checks for available cipher before setting h2 proto #16850

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add checks for available cipher before setting h2 proto #16850

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cca41a1
add checks for available cipher before setting h2 proto #16850
pschou Nov 24, 2023
f67aec5
Merge branch 'etcd-io:main' into http2-cipher-check
pschou May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions client/pkg/transport/listener.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
pschou marked this conversation as resolved.
Show resolved Hide resolved
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.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return an error directly to fail the etcd's startup in such case.

The only concern is we also support httpOnly mode, in which case we shouldn't return error because it doesn't have to necessarily be over http2,

etcd/server/embed/etcd.go

Line 668 in 6db5e00

sctx.httpOnly = true

It's OK to have one listener for gRPC and the other for http, but it does't make sense if an etcd member only supports serving http requests. Probably we should add a validation to prevent such extreme scenario.

}
} else {
// "h2" NextProtos is necessary for enabling HTTP2 for go's HTTP server
cfg.NextProtos = []string{"h2"}
}

return cfg, nil
}
Expand Down
14 changes: 14 additions & 0 deletions server/embed/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package embed

import (
"context"
"errors"
"fmt"
"io"
defaultLog "log"
Expand Down Expand Up @@ -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
}
Comment on lines +206 to +217
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see https://github.com/etcd-io/etcd/pull/16850/files#r1405109505 .

etcd should have already failed before entering into this method.

Suggested change
// 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)
Expand Down
58 changes: 58 additions & 0 deletions tests/e2e/etcd_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Loading