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

proposal: crypto/tls: Expose maps for cipher suite IDs/names #30325

Closed
JAORMX opened this issue Feb 20, 2019 · 60 comments
Closed

proposal: crypto/tls: Expose maps for cipher suite IDs/names #30325

JAORMX opened this issue Feb 20, 2019 · 60 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@JAORMX
Copy link

JAORMX commented Feb 20, 2019

Setting up a set of cipher suites for your HTTP(s) server requires a lot of boilerplate code. It would be nice to have an easy way to parse the cipher suites, say, from a configuration file.

This would make the TLS configuration friendlier (no need for more builder plate code) and easier to audit (given that some folks have opted not to configure this).

We could achieve this by having a mapping from a string value towards the cipher suite constant, which would be kept alongside the constants that are already in the source code.

Having this as part of the source code has the added benefit that, since the mapping would be besides the const values, it would be easier to maintain. As opposed to having this functionality as part of an external library.

This would help address requests like #21167

@gopherbot gopherbot added this to the Proposal milestone Feb 20, 2019
JAORMX added a commit to JAORMX/go that referenced this issue Feb 20, 2019
This is a utility function that allows you to parse a set of cipher
suites, and get a slice containing the const values for each cipher
suite.

The main purpose of this is to be able to easily parse such
configurations for HTTP(s) servers without much boiler plate code.

Fixes: golang#30325
JAORMX added a commit to JAORMX/go that referenced this issue Feb 20, 2019
This is a utility function that allows you to parse a set of cipher
suites, and get a slice containing the const values for each cipher
suite.

The main purpose of this is to be able to easily parse such
configurations for HTTP(s) servers without much boiler plate code.

Fixes: golang#30325
@ianlancetaylor ianlancetaylor changed the title proposal: Parse cipher suites from string proposal: crypto: parse cipher suites from string Feb 20, 2019
@ianlancetaylor
Copy link
Contributor

Sample docs from https://golang.org/cl/163119:

// CipherSuitesStringToList takes a string containing the wanted cipher suites
// in string form, and transforms them into a slice of uint16 values. The
// cipher suite names are added to the string and are separated by colons.
// For instance, an input of:
//
//    "TLS_RSA_WITH_AES_128_CBC_SHA:TLS_RSA_WITH_AES_256_CBC_SHA"
//
// Would yield as output:
//
//    []uint16{TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA}
//
// This is quite useful for configuring HTTP(s) servers from command line
// arguments or configuration files.

@gopherbot
Copy link

Change https://golang.org/cl/163119 mentions this issue: crypto: Create method for parsing string of cipher suites

@FiloSottile
Copy link
Contributor

Rather than defining a specific configuration format (what if it would make more sense for you to have a YAML list instead of a colon-separated string?) it might be time to add a map[string]uint16 and maybe a map[uint16]string, too. Looks like everyone replicated the latter anyway to print ciphersuite names.

A String method would be nicer, but alas ciphersuites are not typed.

@FiloSottile
Copy link
Contributor

I'm thinking we should just add this

// CipherSuitesNames maps cipher suite IDs to names like
// "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256". The presence of an ID in this map
// does not mean the cipher suite is currently supported, or enabled by default.
var CipherSuitesNames = map[uint16]string{}

and let parsers build the inverse map, as they are the rarer use case.

@golang/osp-team CipherSuiteName or CipherSuitesNames?

@FiloSottile FiloSottile added the Proposal-Crypto Proposal related to crypto packages or other security issues label Feb 20, 2019
@JAORMX
Copy link
Author

JAORMX commented Feb 20, 2019

@FiloSottile I have no strong opinion the format really. As long as we get something that works :D. The only reason I used colons is because it's similar to what OpenSSL uses.

Can you give an example on what you're thinking about with the yaml-like format?

@bcmills
Copy link
Member

bcmills commented Feb 20, 2019

CipherSuiteName or CipherSuitesNames?

CipherSuiteName (or NameForCipherSuite)

@JAORMX
Copy link
Author

JAORMX commented Feb 20, 2019

@FiloSottile @bcmills this is the current code proposal #30326 . What do you think about the naming?

I can definitely add the inverse mapping (map[uint16]string) if needed.

@FiloSottile
Copy link
Contributor

I'm thinking we should not prescribe any format at all, and instead expose only NameForCipherSuite.

A parser could then invert the map (which would be directly useful to anyone printing the name of a suite, which I've seen done many times)

var cipherSuites = make(map[string]uint16, len(tls.NameForCipherSuite))
for id, name := range tls.NameForCipherSuite {
    cipherSuites[name] = id
}

and use it to parse whatever format it wants. (JSON, colon separated, multiple flags, YAML, INI...)

@bcmills
Copy link
Member

bcmills commented Feb 20, 2019

FWIW, pretty much every time I've exported a map variable, it has turned out that I should have exported a function or two instead. Global maps are prone to data races (particularly if the program does anything interesting in an init function), and susceptible to monkey-patching (which works great until it doesn't, and then is very difficult to debug).

Given that we're talking about adding stable API surface to the standard library, the more I think about it the less I like the idea of a map.

@FiloSottile
Copy link
Contributor

I was about to backtrack on it being a map, but it satisfies three use cases, which otherwise would be three different exported symbols, unless you have a better idea:

  • map a name to an ID
  • map an ID to a name
  • list all available names/IDs

@JAORMX
Copy link
Author

JAORMX commented Feb 20, 2019

Well, it could be a couple of functions that return the two maps that @FiloSottile is talking about. That way we don't expose global instances, but ephemeral instances returned by those functions instead.

JAORMX added a commit to JAORMX/go that referenced this issue Feb 20, 2019
These are utility functions that allow you to get maps that translate
from Cipher Suite names towards IDs and vice versa.

The main purpose of this is to be able to easily parse such
configurations for HTTP(s) servers without much boiler plate code.

Fixes: golang#30325
@JAORMX
Copy link
Author

JAORMX commented Feb 20, 2019

I just pushed an update to #30326 to make it a couple of functions that return maps. What do you think @FiloSottile @bcmills ?

@JAORMX JAORMX changed the title proposal: crypto: parse cipher suites from string proposal: crypto: Expose maps for cipher suite IDs/names Feb 26, 2019
@JAORMX
Copy link
Author

JAORMX commented Feb 26, 2019

I updated the topic to match the current proposal.

@bcmills
Copy link
Member

bcmills commented Feb 28, 2019

What do you think […]?

func GetNamesForCipherSuites() map[string]uint16 seems a bit awkward, since the aliasing properties of the map aren't entirely clear.

Moreover, the use of uint16 instead of a named type is really unforunate, but I guess we're stuck with it for the constants. But we could at least introduce a named type for the mappings.

How is this as an alternative?

// […]
type CipherSuite uint16

// […]
func CipherSuiteByName(string) (CipherSuite, bool) {
	[…]
}

// […]
func (s CipherSuite) Name() string {
	[…]
}

var cipherSuites [...]CipherSuite = […]

// […]
func CipherSuites() []CipherSuite {
	return append([]CipherSuite(nil), cipherSuites...)
}

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 28, 2019
@JAORMX
Copy link
Author

JAORMX commented Mar 1, 2019

@bcmills I'm very much into the idea of creating a type for cipher suites. Perhaps with further iterations, we could switch the current usage of the uint16 const cipher suites to use this type too.

@noneymous
Copy link

noneymous commented Mar 6, 2019

Hi everyone!

I'm usually using Go to write security related checks and scanners. I often have issues with Go because some valuable information is either held private within a package, or in a format that cannot be accessed dynamically.

In this case, the valuable information is the available cipher suites. They are accessible, but I cannot query them. So I have to hard-code them in my package. Of course, this means, that I have to monitor this package for the infinite future for potential changes (e.g. if another cipher is added, or one gets disabled by default).

A simple use case in my situation is, to always enable as many ciphers as available (implemented). The more the better. Becasue, I don't care about confidentiality/integrity, I just want to make sure to be able to connect to as many (legacy) devices as possible in order to check them for something. So in my code I would like to read the slice of existing cipher suites and put them into the TLS config. Other people might have other criteria to chose their ciphers, e.g. any with certain attribtues,...

All the issue because of a single letter ;-) If
var cipherSuites = []*cipherSuite{ ... }
was just public, everything would be fine.

This would also be the most easy change, and every body could filter and apply the slice as needed. All information would be accessible from the outside. But I assume, this won't be the change implemented because of compatibility reasons.

Second best option would be to add kind of a getter function returning that slice (or a copy if you want to maintain the isolation)

Thrid best option would be a function returning something else, like a simple slice of cipher IDs or a map,... just as you were discussing above. But then we would be deciding other peoples use cases.

best regards,
Ben

JAORMX added a commit to JAORMX/go that referenced this issue Mar 7, 2019
This change exposes the CipherSuite structure, so folks can actually use
it in their code-bases. It also exposes the needed functions to get the
cipher suite both by name and by ID.

The "name" of the cipher suite was added to the struct.

Fixes: golang#30325
@JAORMX
Copy link
Author

JAORMX commented Mar 7, 2019

@bcmills @FiloSottile @noneymous I submitted an alternative proposal (though a little more invasive) in #30654 . What do you think?

JAORMX added a commit to JAORMX/go that referenced this issue Mar 7, 2019
This change exposes the CipherSuite structure, so folks can actually use
it in their code-bases. It also exposes the needed functions to get the
cipher suite both by name and by ID.

The "name" of the cipher suite was added to the struct.

Fixes: golang#30325
@FiloSottile
Copy link
Contributor

Alright, spent some time thinking about this, and crypto/tls serves applications, not testing tools, but if we think applications won't need a cipher suite we should not implement it in the first place. If we do implement it, I don't think we should require hardcoding it in order to use it.

I renamed CipherSuites to CipherSuitesIncludingInsecure, though, so it's clear to any implementer and reviewer that they should check the Insecure field if they care about a security baseline.

I expect most applications will be using CipherSuiteByID anyway.

@noneymous
Copy link

My totally valid application which just doesn't care about transport security takes this personally ^^ But other than that, I go conform with the current suggestion.

@gopherbot
Copy link

Change https://golang.org/cl/175517 mentions this issue: crypto/tls: add CipherSuites, InsecureCipherSuites and CipherSuiteName

@dolmen
Copy link
Contributor

dolmen commented Jun 20, 2019

Do we really need suites names in crypto/tls? This list will only grow with time with most of the suites becoming irrelevant.

What about exposing them instead in a non-stdlib package under golang.org/x/crypto?

@golang golang deleted a comment from dolmen Jun 20, 2019
@golang golang deleted a comment from dolmen Jun 20, 2019
@FiloSottile
Copy link
Contributor

@dolmen We already have exported constants for all cipher suite names, that's not changing. The new functions just return a slice, so they don't grow problematically.

@mholt
Copy link

mholt commented Jul 18, 2019

Incidentally, is there a particular reason that these two consts don't use the full IANA name, whereas all the others do?

TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305    uint16 = 0xcca8
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305  uint16 = 0xcca9

Why are they missing the _SHA256?

TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256

https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml

@davidben
Copy link
Contributor

davidben commented Jul 18, 2019

I don't remember the timeline of Go's implementation, but the suffix got added at the last minute during standardization, so a lot of things are missing it. :-/

https://tools.ietf.org/rfcdiff?url2=draft-ietf-tls-chacha20-poly1305-04.txt

@FiloSottile
Copy link
Contributor

This is #32061. Thanks @davidben, I was wondering how that happened.

@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 16, 2019
@FiloSottile
Copy link
Contributor

We discussed this with @rsc for Go 1.14, and came up with this API. It has a nice to use helper for the common case of turning an ID into a name, and relegates the insecure entries to their own API.

// CipherSuite is a TLS cipher suite. Note that most functions in this package
// accept and expose cipher suite IDs instead of this type.
type CipherSuite struct {
	ID   uint16
	Name string

	// Supported versions is the list of TLS protocol versions that can
	// negotiate this cipher suite.
	SupportedVersions []uint16

	// Insecure is true if the cipher suite has known security issues
	// due to its primitives, design, or implementation.
	Insecure bool
}

// CipherSuites returns a list of cipher suites supported by this package,
// ordered by ID. This list might not reflect the list of cipher suites enabled
// by default depending on a number of factors including the platform and peer.
// This list will change across Go versions.
func CipherSuites() []*CipherSuite

// CipherSuites returns a list of cipher suites that are disabled by default,
// but still supported by this package. These cipher suites are not included
// in the list returned by CipherSuites.
func InsecureCipherSuites() []*CipherSuite

// CipherSuiteName returns the standard name of the cipher suite,
// if it is known to this package. Otherwise, it returns its
// %04X fmt representation.
func CipherSuiteName(id uint16) string

I think this should still work for all the use cases we discussed, but please do provide your feedback.

@FiloSottile FiloSottile modified the milestones: Proposal, Go1.14 Oct 1, 2019
@mholt
Copy link

mholt commented Oct 1, 2019

Nice! That will work for our use case in Caddy.

(is there any particular reason to use*CipherSuite over CipherSuite type? i'm worried about the potential to accidentally-or-whatever change pointed values...)

@FiloSottile
Copy link
Contributor

(is there any particular reason to use*CipherSuite over CipherSuite type? i'm worried about the potential to accidentally-or-whatever change pointed values...)

I always use pointers to structs that are not comparable in nature to a native value (like say, time.Time). This one even has a slice, so it has some indirection anyway. If we are worried about accidental changes, we can make deep copies, but not sure it's worth it.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.14 Oct 23, 2019
@FiloSottile
Copy link
Contributor

Adopting the new proposal tracking process, this seems like a likely accept.

Leaving open a week for final comments.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet