Add TLSv1.3 support. #15156

tsaarni · 2023-01-20T22:09:32Z

This PR adds optional TLS min/max protocol versions fields to TLSInfo struct, which is used in both server and client configs. Instead of hardcoding max version to TLSv1.2 it lets Go select the max version. This effectively enables TLSv1.3 support for etcd. The min/max values are also exposed via optional server flags and config file fields so that users can explicitly set the minimum and/or maximum TLS versions, if they want.

It seems that TLSv1.3 was previously supported (prior etcd 3.5) but it got disabled as a workaround to fix a test suite failure which appeared when Go 1.13 release enabled TLSv1.3 by default, see discussion #11110 (comment). In this PR, the failing test case is changed to set the max version to TLSv1.2. This approach allows testing ciphers with <TLSv1.3 without disabling TLSv1.3 altogether.

Related documentation change etcd-io/website#641.

Fixes #13506

Signed-off-by: Tero Saarni tero.saarni@est.tech

tsaarni · 2023-01-20T22:37:18Z

Hi @serathius, please could you check this PR. Thank You!

codecov-commenter · 2023-01-20T22:38:00Z

Codecov Report

Merging #15156 (3f3e622) into main (ee566c4) will decrease coverage by 0.31%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main   #15156      +/-   ##
==========================================
- Coverage   74.93%   74.62%   -0.31%     
==========================================
  Files         415      416       +1     
  Lines       34326    34351      +25     
==========================================
- Hits        25721    25634      -87     
- Misses       6986     7072      +86     
- Partials     1619     1645      +26

Flag	Coverage Δ
all	`74.62% <77.77%> (-0.31%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/embed/etcd.go	`77.36% <0.00%> (-0.61%)`	⬇️
server/embed/config.go	`75.24% <100.00%> (+0.75%)`	⬆️
server/etcdmain/config.go	`85.13% <100.00%> (+0.11%)`	⬆️
server/proxy/grpcproxy/register.go	`69.76% <0.00%> (-11.63%)`	⬇️
server/storage/mvcc/watchable_store.go	`85.30% <0.00%> (-8.97%)`	⬇️
client/pkg/v3/fileutil/purge.go	`68.85% <0.00%> (-6.56%)`	⬇️
client/v3/namespace/watch.go	`87.87% <0.00%> (-6.07%)`	⬇️
server/lease/lease.go	`94.87% <0.00%> (-5.13%)`	⬇️
client/pkg/v3/testutil/leak.go	`62.83% <0.00%> (-4.43%)`	⬇️
client/v3/leasing/cache.go	`87.77% <0.00%> (-3.89%)`	⬇️
... and 25 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

serathius

LGTM, is the min/max tls config exposed in etcd server binary flags?
cc @ahrtr @ptabor for second opinion

tsaarni · 2023-01-23T10:00:54Z

is the min/max tls config exposed in etcd server binary flags

Currently not, I targeted only to the minimum change. I can add server flags & config file options as well if requested.

serathius · 2023-01-23T10:13:29Z

is the min/max tls config exposed in etcd server binary flags

Currently not, I targeted only to the minimum change. I can add server flags & config file options as well if requested.

Sounds good

tsaarni · 2023-01-23T20:27:22Z

I've now added config options for etcd server binary and new tests. I see the docs are in website repo so a follow up PR is needed for the docs after confirming the proposed flags are OK.

tuminoid

Thanks @tsaarni for the PR! I added two comments, let me know what you think @serathius.

server/embed/config.go

client/pkg/tlsutil/versions.go

PR changed since

tsaarni · 2023-01-24T08:52:27Z

Thanks @tuminoid I agree with your review comments, so I went ahead and made those changes. Eagerly waiting for more reviews :-)

serathius · 2023-01-24T09:21:53Z

cc @ptabor

tsaarni · 2023-01-24T12:40:58Z

@serathius One test failed but could it be a flake https://github.com/etcd-io/etcd/actions/runs/3994448937/jobs/6852174459? Would it be possible to trigger a re-run?

server/embed/config.go

server/etcdmain/config.go

tests/integration/v3_tls_test.go

server/embed/config_test.go

ahrtr · 2023-01-24T20:36:08Z

Thank you @tsaarni

Overall looks good to me, with a couple of minor comments.

server/etcdmain/help.go

ahrtr

LGTM

Please consider to use github.com/stretchr/testify to simplify the test cases if you want.

tsaarni · 2023-01-25T10:48:20Z

Thank you @ahrtr, great feedback!

If it's OK, I will leave the tests as they are, but I also like github.com/stretchr/testify asserts better. When looking around the test files I touched, I thought standard testing package was preferred, so I used that mostly.

tsaarni · 2023-01-25T14:25:57Z

I've submitted also a proposal for the documentation change etcd-io/website#641.

client/pkg/tlsutil/versions.go

server/embed/config.go

server/embed/etcd.go

server/embed/config.go

serathius · 2023-01-26T09:35:10Z

Please add e2e tests for new etcdserver flags.

server/embed/config.go

tsaarni · 2023-01-27T15:57:53Z

Please add e2e tests for new etcdserver flags.

Thank you @serathius! I've made the requested changes, except for the refactoring of how TLSInfo is generated, which I commented previously above. Please, could you take another look and check if this would be acceptable. Thank you!

Please consider to use github.com/stretchr/testify to simplify the test cases if you want.

If it's OK, I will leave the tests as they are, but I also like github.com/stretchr/testify asserts better. When looking around the test files I touched, I thought standard testing package was preferred, so I used that mostly.

@ahrtr I've now changed to testify after all, since I anyways went through a bigger change in the previous commit.

serathius · 2023-01-30T11:18:10Z

server/embed/config.go

@@ -660,6 +666,12 @@ func updateCipherSuites(tls *transport.TLSInfo, ss []string) error {
 	return nil
 }

+func updateMinMaxVersions(info *transport.TLSInfo, min, max string) {
+	// Validate() has been called to check the user input, so it is safe to ignore errors here.


Nit: At that point we should not expect error, but in tests or during refactor someone might forget to call Validate and provide invalid input. It would make sense to just panic on errors here.

panic or cfg.logger.Fatal.

serathius · 2023-01-30T11:19:31Z

server/etcdmain/config.go

@@ -215,6 +216,8 @@ func newConfig() *config {
 	fs.StringVar(&cfg.ec.PeerTLSInfo.AllowedHostname, "peer-cert-allowed-hostname", "", "Allowed TLS hostname for inter peer authentication.")
 	fs.Var(flags.NewStringsValue(""), "cipher-suites", "Comma-separated list of supported TLS cipher suites between client/server and peers (empty will be auto-populated by Go).")
 	fs.BoolVar(&cfg.ec.PeerTLSInfo.SkipClientSANVerify, "experimental-peer-skip-client-san-verification", false, "Skip verification of SAN field in client certificate for peer connections.")
+	fs.StringVar(&cfg.ec.TlsMinVersion, "tls-min-version", string(tlsutil.TLSVersion12), "Minimum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3.")
+	fs.StringVar(&cfg.ec.TlsMaxVersion, "tls-max-version", "", "Maximum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3 (empty will be auto-populated by Go).")


Suggested change

fs.StringVar(&cfg.ec.TlsMaxVersion, "tls-max-version", "", "Maximum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3 (empty will be auto-populated by Go).")

fs.StringVar(&cfg.ec.TlsMaxVersion, "tls-max-version", string(tlsutil.TLSVersionDefault), "Maximum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3 (empty will be auto-populated by Go).")

serathius · 2023-01-30T11:20:37Z

server/etcdmain/config.go

@@ -215,6 +216,8 @@ func newConfig() *config {
 	fs.StringVar(&cfg.ec.PeerTLSInfo.AllowedHostname, "peer-cert-allowed-hostname", "", "Allowed TLS hostname for inter peer authentication.")
 	fs.Var(flags.NewStringsValue(""), "cipher-suites", "Comma-separated list of supported TLS cipher suites between client/server and peers (empty will be auto-populated by Go).")
 	fs.BoolVar(&cfg.ec.PeerTLSInfo.SkipClientSANVerify, "experimental-peer-skip-client-san-verification", false, "Skip verification of SAN field in client certificate for peer connections.")
+	fs.StringVar(&cfg.ec.TlsMinVersion, "tls-min-version", string(tlsutil.TLSVersion12), "Minimum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3.")
+	fs.StringVar(&cfg.ec.TlsMaxVersion, "tls-max-version", "", "Maximum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3 (empty will be auto-populated by Go).")


Suggested change

fs.StringVar(&cfg.ec.TlsMaxVersion, "tls-max-version", "", "Maximum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3 (empty will be auto-populated by Go).")

fs.StringVar(&cfg.ec.TlsMaxVersion, "tls-max-version", "", "Maximum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3 (empty defers to Go).")

Added optional TLS min/max protocol version and command line switches to set versions for the etcd server. If max version is not explicitly set by the user, let Go select the max version which is currently TLSv1.3. Previously max version was set to TLSv1.2. Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni · 2023-01-30T15:33:31Z

I have made the requested changes but tests failed, possibly flukes again?

This adds the min and max TLS version support from etcd-io#13506 and etcd-io#15156 to the grpc proxy. Fixes etcd-io#13506 Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>

This adds the min and max TLS version support from etcd-io#13506 and etcd-io#15156 to the grpc proxy. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>

serathius previously approved these changes Jan 23, 2023

View reviewed changes

tsaarni requested a review from serathius January 23, 2023 20:56

tuminoid reviewed Jan 24, 2023

View reviewed changes

server/embed/config.go Outdated Show resolved Hide resolved

client/pkg/tlsutil/versions.go Outdated Show resolved Hide resolved

ahrtr reviewed Jan 24, 2023

View reviewed changes

server/embed/config.go Outdated Show resolved Hide resolved

server/etcdmain/config.go Outdated Show resolved Hide resolved

tests/integration/v3_tls_test.go Show resolved Hide resolved

server/embed/config_test.go Outdated Show resolved Hide resolved

tsaarni force-pushed the add-minmax-allow-tls13 branch from 3f3e622 to cf76be0 Compare January 25, 2023 09:59

ahrtr reviewed Jan 25, 2023

View reviewed changes

server/etcdmain/help.go Outdated Show resolved Hide resolved

tsaarni force-pushed the add-minmax-allow-tls13 branch from cf76be0 to fc5ab82 Compare January 25, 2023 10:11

ahrtr approved these changes Jan 25, 2023

View reviewed changes

tsaarni mentioned this pull request Jan 25, 2023

Add TLS min/max version. etcd-io/website#641

Merged