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

security: update the TLS cipher suite list #80476

Merged
merged 1 commit into from
May 3, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Apr 25, 2022

Fixes #80483

This does not really change the list, it merely explains more clearly
how it was built.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz marked this pull request as ready for review April 25, 2022 15:33
@knz knz requested review from a team as code owners April 25, 2022 15:33
@knz knz requested a review from bdarnell April 25, 2022 15:36
Copy link
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @knz)


-- commits line 8 at r1:
Do not say that TLS 1.2 is deprecated; we're still a long way from starting that process (and it's not really our call. TLS 1.1 was deprecated by RFC 8996, and I expect we'll follow a similar IETF-led process when it's time to drop 1.2).


pkg/security/tls.go line 162 at r1 (raw file):

		//   include at least one cipher higher in the priority list, but
		//   there's also no reason to keep it around)
		// - AES is always prioritized over ChaCha20. Go makes this decision

While you're here, this point can be removed. In current versions of go, the order of this list is ignored and the implementation will reorder AES vs ChaCha20 automatically.


pkg/security/tls.go line 184 at r1 (raw file):

			tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
			tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
			tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,

These constants with the SHA256 suffixes are equal to the preceding ones without SHA256; there's no reason to have both in the list (see this commit).

The SHA256 names are the official ones, so it's good to change from the deprecated aliases to the proper names, but we're not adding any new ciphers to the list and therefore we don't need to document anything in release notes.


pkg/security/tls.go line 194 at r1 (raw file):

			tls.TLS_RSA_WITH_AES_128_CBC_SHA,
			tls.TLS_RSA_WITH_AES_256_CBC_SHA,
			// NB: no need to add TLS 1.3 ciphers here. As per the

Thanks for this comment; it's a common point of confusion.

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @knz)


-- commits line 8 at r1:

Previously, bdarnell (Ben Darnell) wrote…

Do not say that TLS 1.2 is deprecated; we're still a long way from starting that process (and it's not really our call. TLS 1.1 was deprecated by RFC 8996, and I expect we'll follow a similar IETF-led process when it's time to drop 1.2).

Done.


pkg/security/tls.go line 162 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

While you're here, this point can be removed. In current versions of go, the order of this list is ignored and the implementation will reorder AES vs ChaCha20 automatically.

I went to the source code and I don't see what you say. I still see the code that pulls AES up if supported in hardware, but it's not unconditional?


pkg/security/tls.go line 184 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

These constants with the SHA256 suffixes are equal to the preceding ones without SHA256; there's no reason to have both in the list (see this commit).

The SHA256 names are the official ones, so it's good to change from the deprecated aliases to the proper names, but we're not adding any new ciphers to the list and therefore we don't need to document anything in release notes.

Done.

Copy link
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @knz)


pkg/security/tls.go line 162 at r1 (raw file):

Previously, knz (kena) wrote…

I went to the source code and I don't see what you say. I still see the code that pulls AES up if supported in hardware, but it's not unconditional?

In older versions of Go, Config.CipherSuites is treated as a static ordered list (i.e. it uses the input order to prioritize ciphers), but if Config.CipherSuites the defaults would be computed dynamically to prioritize AES or ChaCha based on hardware capabilities. This comment was referring to the fact that the dynamic prioritization was lost if any CipherSuites were specified.

In newer versions of Go, Config.CipherSuites is treated as an unordered set and the crypto/tls library will apply the dynamic prioritization order to that set, so we no longer lose anything by passing in ciphers.

golang/go@9d0819b

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

TFYR

bors r=bdarnell

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @bdarnell and @knz)


pkg/security/tls.go line 162 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

In older versions of Go, Config.CipherSuites is treated as a static ordered list (i.e. it uses the input order to prioritize ciphers), but if Config.CipherSuites the defaults would be computed dynamically to prioritize AES or ChaCha based on hardware capabilities. This comment was referring to the fact that the dynamic prioritization was lost if any CipherSuites were specified.

In newer versions of Go, Config.CipherSuites is treated as an unordered set and the crypto/tls library will apply the dynamic prioritization order to that set, so we no longer lose anything by passing in ciphers.

golang/go@9d0819b

Done.

This does not really change the list, it merely explains more clearly
how it was built.

Release note: None
@craig
Copy link
Contributor

craig bot commented May 3, 2022

Build succeeded:

@craig craig bot merged commit d912b66 into cockroachdb:master May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CHACHA ciphers for TLS 1.2
3 participants