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

Add the ChaCha20 ciphers to the default cipher list. #415

Merged
merged 3 commits into from
Jan 5, 2017

Conversation

ColinSullivan1
Copy link
Member

No description provided.

@@ -39,6 +39,8 @@ func defaultCipherSuites() []uint16 {
tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
Copy link
Member

Choose a reason for hiding this comment

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

What is default for 1.8 if not configured at all? I think they put ChaCha first because of its performance gains, even over AES..

Copy link
Member Author

@ColinSullivan1 ColinSullivan1 Jan 5, 2017

Choose a reason for hiding this comment

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

Looks to be based on hardware: https://github.com/golang/go/blob/master/src/crypto/tls/common.go#L924
Shall we try to do the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not much luck there for us - looks like the hardware check belongs to an internal package.

Copy link
Member Author

Choose a reason for hiding this comment

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

After some research, and given NATS usage patterns, it does look like the preference should be for supporting for smaller clients (chipsets without the AES-NI instruction set) right now. If someone wants to reorder preferences for GCM, they certainly can in our config. I can reorder these to place chacha first then... sound good?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 89.153% when pulling 6269385 on tls-add-chacha-default into c6e8014 on master.

@derekcollison
Copy link
Member

Let's prefer ChaCha but allow them to override.

@@ -35,12 +35,12 @@ var cipherMap = map[string]uint16{

func defaultCipherSuites() []uint16 {
return []uint16{
tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
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 we should put ECDSA first..

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Will do...

@derekcollison derekcollison merged commit c275ec7 into master Jan 5, 2017
@derekcollison derekcollison deleted the tls-add-chacha-default branch January 5, 2017 23:27
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.

3 participants