add checks for available cipher before setting h2 proto #16850
Conversation
If that's a case, please provide a test that confirms that and prevents invalid configuration in future. Best if that would be an e2e test. Please let me know if you need any help with writing it. |
pschou
force-pushed
the
http2-cipher-check
branch
from
a498fd8
to
21bb2c3
Compare
|
Example: ./bin/etcd --tls-max-version=TLS1.2 --cipher-suites='TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384' --cert-file=/etc/pki/server.pem --key-file=/etc/pki/server.pem --trusted-ca-file=/etc/pki/ca.pem --client-cert-auth=true --advertise-client-urls=https://localhost:32379 --listen-client-urls=https://localhost:32379 Without the update: With the update: |
There was a problem hiding this comment.
Hey @pschou - Thanks for adding the example. As suggested above, to make sure this behavior doesn't get broken again in future it would be ideal to have an e2e test covering this situation.
Please let us know if you need a hand adding this.
Thank you for the review and I do think I will need help. I have made an e2e attempt to see if this is a step in the right direction. Any advice or help would be greatly appreciated. |
There was a problem hiding this comment.
This aligns with http2 spec[1][2] so LGTM, thanks for refining this @pschou.
Just one suggestion about tracking a future improvement to how we implement this check.
1: https://httpwg.org/specs/rfc9113.html#tls12ciphers
2: https://httpwg.org/specs/rfc9113.html#BadCipherSuites
|
/ok-to-test |
|
cc @ahrtr |
|
Please squash commits. |
|
Sorry about that last-minute thought. I want to ensure that users can see an information line indicating what is happening so debugging can be more graceful. |
pschou
force-pushed
the
http2-cipher-check
branch
2 times, most recently
from
6e347a5
to
796ad50
Compare
Signed-off-by: schou <pschou@users.noreply.github.com>
pschou
force-pushed
the
http2-cipher-check
branch
from
796ad50
to
cca41a1
Compare
done |
|
/retest |
|
/retest |
| 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.") |
There was a problem hiding this comment.
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,
Line 668 in 6db5e00
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.
| // 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 | ||
| } |
There was a problem hiding this comment.
see https://github.com/etcd-io/etcd/pull/16850/files#r1405109505 .
etcd should have already failed before entering into this method.
| // 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 | |
| } |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions. |
|
@pschou: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Noting:
"When we implemented TLS 1.3 in Go 1.12, we didn't make TLS 1.3 cipher suites configurable, because they are a disjoint set from the TLS 1.0–1.2" - Golang
So, in effect... With crypto/tls: TLS 1.3 ciphers are not configurable
This setting of enforcing the TLS maximum at the command line is a must-- if the maximum is not set, golang will add all three TLSv1.3 ciphers, effectively making the configuration of which ciphers are allowed not configurable-- it's an all or nothing setup.
This PR addresses the issue when the maximum is set and a subset of the remaining ciphers are set, effectively enabling or disabling support for "h2" proto automatically based on ability.
Without this, etcd crashes on startup.