Skip to content

crypto/tls: Export TLS default cipher suites #21167

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

Closed
bdarnell opened this issue Jul 25, 2017 · 8 comments
Closed

crypto/tls: Export TLS default cipher suites #21167

bdarnell opened this issue Jul 25, 2017 · 8 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@bdarnell
Copy link

$ go version
go version go1.8.3 darwin/amd64

I would like to set tls.Config.CipherSuites to a non-default value (such as mozilla's "modern" compatibility recommendation), and prioritize either AES-GCM or ChaCha20 depending on whether hardware-accelerated AES is available (i.e. the same logic as in tls/common.go).

There is no way that I can see to inspect this default certificate list (it is only used in the non-exported Config.cipherSuites method), and the cipherhw package is internal-only. I could duplicate the cipherhw package and make the CPUID assembly call myself (and the analogous instructions for other CPUs), but that allows for version skew between my code and crypto/tls: what I want is not "does this CPU have AES support", but "will crypto/tls use an accelerated AES implementation on this CPU".

It would be nice if crypto/tls exposed either some way to inspect its default cipher suite list or to make the same performance-based cipher prioritization decisions as the default implementation.

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 25, 2017
@ALTree
Copy link
Member

ALTree commented Jul 25, 2017

cc @agl

@bradfitz
Copy link
Contributor

Possible hack: write a func to use crypto/tls to handshake with itself over an in-memory net.Pipe (not even localhost TCP) and see what it announces to itself with:

https://golang.org/pkg/net/#Pipe +
https://golang.org/pkg/crypto/tls/#Config.GetCertificate + InsecureSkipVerify +
https://golang.org/pkg/crypto/tls/#ClientHelloInfo.CipherSuites

shrug

@FiloSottile
Copy link
Contributor

I don't think we should commit to the default logic being a simple list of ciphersuites. A nil Config.CipherSuites just means "what the team thinks is best", which might involve custom logic.

For example #21144 proposed making version-based decisions. TLS 1.3 might bring logic along, too.

About what you are trying to do, crypto/tls is just another user of crypto/cipher, so I think replicating the logic is at least as good as inferring it from the defaults.

@bdarnell
Copy link
Author

bdarnell commented Jul 26, 2017

I don't think we should commit to the default logic being a simple list of ciphersuites.

That's not what I'm asking for. It's already not a simple list: the order depends on whether hardware-accelerated AES is available. Edit: I get what you're saying now. You want to retain the flexibility to make it even more dynamic in the future, so returning a list might not be able to capture the full logic.

The response to #21144 was "if you don't want 3DES, use your own list of cipher suites that doesn't include it". That's what I'm trying to do here, but in selecting my own cipher suites, I lose the automatic performance-based selection between AES and ChaCha20.

crypto/tls is just another user of crypto/cipher, so I think replicating the logic is at least as good as inferring it from the defaults.

The logic I'm trying to replicate here depends on crypto/internal/cipherhw, which is not accessible. Duplicating that whole package is an option, but a less-than-ideal one since my decisions about when AES acceleration is available may differ from whether Go can and will use that acceleration. Exposing the cipherhw package would be a good solution as far as I'm concerned.

@bradfitz
Copy link
Contributor

Exposing the cipherhw package would be a good solution as far as I'm concerned.

We really dislike exposing guts that we're then locked into maintaining for years and years.

If we did, it would be in x/crypto with no stability promises (our typical "this package may change at any time, please vendor it if you use it") and we'd then vendor it into std for releases.

@FiloSottile
Copy link
Contributor

@bdarnell Exactly :) (Shakes fist at GitHub for not emailing significant edits.)

While I agree that duplicating the cipherhw logic is sub-optimal, I think inferring from default cipher suites is as well, for different reasons: for example, our priorities might change, and we might not order them based on performance anymore, while your decision based on your needs and platform might stay the same.

FWIW, github.com/codahale/aesnicheck already offers you the functionality.

@bdarnell
Copy link
Author

bdarnell commented Aug 6, 2017

FWIW, github.com/codahale/aesnicheck already offers you the functionality.

Partially - aesnicheck only works on amd64, while the standard library also uses accelerated AES on s390x. And #18498 tracks the potential addition of support for similar instructions on arm64, so when/if that goes in we'd need to duplicate that logic as well (and use build tags to ensure that we only use it in go 1.10 or whichever version introduces it).

Anyway, while the duplication is annoying, it's manageable, and no good API suggests itself (it's not clear what a generic api would look like, and a very specific "is AES faster than ChaCha20 here" API is an awkward thing to commit to in a public interface). So I'm OK with closing this issue unless someone has a better idea.

@rsc
Copy link
Contributor

rsc commented Oct 23, 2017

OK, based on discussion above, closing.

@rsc rsc closed this as completed Oct 23, 2017
@golang golang locked and limited conversation to collaborators Oct 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants