add checks for available cipher before setting h2 proto

[pschou]

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.

[serathius]

Without this, etcd crashes on startup.

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]

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: ... {"level":"fatal","ts":"2023-11-06T11:34:00.039138-0500","caller":"etcdmain/etcd.go:196","msg":"listener failed","error":"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)","stacktrace":"go.etcd.io/etcd/server/v3/etcdmain.startEtcdOrProxyV2\n\tgo.etcd.io/etcd/server/v3/etcdmain/etcd.go:196\ngo.etcd.io/etcd/server/v3/etcdmain.Main\n\tgo.etcd.io/etcd/server/v3/etcdmain/main.go:40\nmain.main\n\tgo.etcd.io/etcd/server/v3/main.go:31\nruntime.main\n\truntime/proc.go:250"} ...

With the update: ... {"level":"info","ts":"2023-11-06T13:38:32.33227-0500","caller":"embed/serve.go:251","msg":"serving client traffic securely","traffic":"http","address":"127.0.0.1:32379"} ...

[pschou]

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.

[jmhbnz]

/ok-to-test

[serathius]

cc @ahrtr

[serathius]

Please squash commits.

[pschou]

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]

Please squash commits.

done

[pschou]

/retest

[pschou]

/retest

[stale[bot]]

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.

[jmhbnz]

Discussed during sig-etcd triage meeting. Would be great to finish this off @pschou do you have time to rebase this and please address the two comments from @ahrtr? Thanks!

[k8s-ci-robot]

PR needs rebase.

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.

[stale[bot]]

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.

[k8s-ci-robot]

@pschou: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-verify	f67aec531a361e976ec24a064131ac33b455566a	link	true	/test pull-etcd-verify
ci-etcd-robustness-release36-amd64	f67aec531a361e976ec24a064131ac33b455566a	link	true	/test ci-etcd-robustness-release36-amd64
ci-etcd-robustness-release35-amd64	f67aec531a361e976ec24a064131ac33b455566a	link	true	/test ci-etcd-robustness-release35-amd64
ci-etcd-robustness-release34-amd64	f67aec531a361e976ec24a064131ac33b455566a	link	true	/test ci-etcd-robustness-release34-amd64
pull-etcd-govulncheck-main	f67aec531a361e976ec24a064131ac33b455566a	link	true	/test pull-etcd-govulncheck-main
pull-etcd-govulncheck	f67aec531a361e976ec24a064131ac33b455566a	link	true	/test pull-etcd-govulncheck

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.

[github-actions[bot]]

This pull request 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.