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.

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

nsqd: enable support for TLS1.3 #1385

Merged
merged 1 commit into from
Oct 21, 2021
Merged

nsqd: enable support for TLS1.3 #1385

merged 1 commit into from
Oct 21, 2021

Conversation

karalabe
Copy link
Contributor

Currently NSQD is capped at TLS1.2. TLS1.3 has been stabilized since Go 1.14, released almost 2 years ago. Might as well bump it.

@karalabe karalabe changed the title nsqd: enablw support for TLS1.3 nsqd: enable support for TLS1.3 Oct 20, 2021
nsqd/nsqd.go Outdated
@@ -717,7 +717,7 @@ func buildTLSConfig(opts *Options) (*tls.Config, error) {
Certificates: []tls.Certificate{cert},
ClientAuth: tlsClientAuthPolicy,
MinVersion: opts.TLSMinVersion,
MaxVersion: tls.VersionTLS12, // enable TLS_FALLBACK_SCSV prior to Go 1.5: https://go-review.googlesource.com/#/c/1776/
MaxVersion: tls.VersionTLS13, // enable TLS_FALLBACK_SCSV prior to Go 1.5: https://go-review.googlesource.com/#/c/1776/
Copy link
Member

Choose a reason for hiding this comment

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

I think this line can be removed, and the default zero value should work?

	// MinVersion contains the minimum SSL/TLS version that is acceptable.
	// If zero, then TLS 1.0 is taken as the minimum.
	MinVersion uint16

	// MaxVersion contains the maximum SSL/TLS version that is acceptable.
	// If zero, then the maximum version supported by this package is used,
	// which is currently TLS 1.3.
	MaxVersion uint16

(I checked latest go-1.17 and back to go-1.13 and it's consistent with that behavior.)

(This code hasn't been revisited in a very long time - I think the comment, and what the code tries to do, is out-of-date.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. PTAL

Currently NSQD is capped at TLS1.2. TLS1.3 has been stabilized since
Go 1.14, released almost 2 years ago. Might as well bump it.
@karalabe
Copy link
Contributor Author

Btw, I needed to make a small tweak to the protocol tests where the TLS handshake was tested.

In TLS1.2 and before, the handshake is a multi-RTT thing, where a lot of things are negotiated. If client certificates are used, those are sent over somewhere in the middle of the handshake and further things are negotiated afterwards. This means that a bad/missing client certificate will cause the handshake to fail.

In TLS1.3, the handshake was reduced to 3 messages: Client -> Server; Server -> Client; Client -> Server (see code). The client certificate is transmitted in the last Client -> Server message; at which point the TLS handshake completes. If the client certificate is bad, that rejection is not returned by the handshake, rather in the first application read. This meant that the code in the tests that relied on tlsConn.Handshake erroring stopped returning failures in TLS1.3. The only way to get the bad client cert error client side is to do an application read and have that fail. Hence my update to the tests.

@karalabe
Copy link
Contributor Author

karalabe commented Oct 21, 2021

Alternatively, I guess it would also be fine to try and read a single byte and have that fail; not to mix in the high level NSQ networking and risk some other error popping up. I can also do that if you want, just followed the test as close as possible in this iteration.

@ploxiln
Copy link
Member

ploxiln commented Oct 21, 2021

This looks good to me, thanks!

@ploxiln ploxiln merged commit 427d893 into nsqio:master Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants