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

[JAORMX]

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

[ianlancetaylor]

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]

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

[FiloSottile]

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]

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?

[JAORMX]

@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]

CipherSuiteName or CipherSuitesNames?

CipherSuiteName (or NameForCipherSuite)

[JAORMX]

@FiloSottile @bcmills this is the current code proposal https://github.com/golang/go/pull/30326 . What do you think about the naming?

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

[FiloSottile]

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]

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]

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]

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]

I just pushed an update to https://github.com/golang/go/pull/30326 to make it a couple of functions that return maps. What do you think @FiloSottile @bcmills ?

[JAORMX]

I updated the topic to match the current proposal.

[bcmills]

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...)
}

[JAORMX]

@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]

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]

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

[gopherbot]

Change https://golang.org/cl/165957 mentions this issue: crypto/tls: Expose CipherSuite structure and add names

[noneymous]

@bcmills

Hm, this doesn't work for me. Because I still cannot iterate over available cipher suites, i can just query them knowing their name.

Why don't we simply make var cipherSuites = []*cipherSuite{ and var cipherSuitesTLS13 = []*cipherSuiteTLS13{ public? After thinking about it again, it should not break anything in existing projects, because it is just making former private stuff public.

Everybody can take this list and process or filter it as required. Everybody has full controll. You could put the "GetByName"-functions in your own package then.

[JAORMX]

@noneymous this was discussed in earlier comments. Folks weren't comfortable by exposing maps in such a way, since it's prone to race conditions. See https://github.com/golang/go/issues/30325#issuecomment-465737728 Anyway; one proposal I had was to create a function that would return an instance to the map. So every call to such a function would have a new instance; that way, we wouldn't have a race condition.

[noneymous]

Alright, missed that. Then we could simply add a getter function returning a copy (probably, like you suggested).

[JAORMX]

@noneymous yep, that's what #30326 does.

[noneymous]

And that would fit my requirements, so I'm okay with it. But it decides the use cases of other people. Why not just adding this:

func CipherSuites() []cipheSuite{ return append(cipherSuites[:0:0], cipherSuites...) }

-> Would return a copy, isolating the original list from manipulations
-> Is nothing hard-coded, so it doesn't need to be updated on future ciper changes

(Copy code based on https://github.com/go101/go101/wiki/How-to-perfectly-clone-a-slice%3F)

[JAORMX]

@noneymous thanks for the suggestion. I updated #30654 with that.

[jzelinskie]

This proposal only affects half of the implementation required to solve #21167.
@bdarnell and I have the same problem: we want to selectively drop certain suites from the defaults, but still let the standard library do whatever magic it would like to come up with that original list of defaults.

By exporting crypto.tls/defaultCipherSuites() along with this proposal being implemented, one can do something like this:

// Create a reverse lookup for cipher suites
reverseCipherSuites := make(map[uint16]string)
for name, id := range tls.CipherSuites() {
  reverseCipherSuites[id] = name
}

// Filter any cipher suites using 3DES
mySuite := make([]uint16, 0)
for _, id := range tls.DefaultCipherSuites() {
    if !strings.Contains(reverseCipherSuiteMap[id], "3DES") {
      mySuite = append(mySuite, id)
    }
}

// Create my TLS Config
cfg := &tls.Config{
  CipherSuites: mySuite,
  MinVersion: tls.VersionTLS12,
}

[noneymous]

Wouldn't the suiteDefaultOff flag contained in the list of ciphers in tls/cipher_suites.go, help you the same way?

[JAORMX]

@jzelinskie: With the patch https://github.com/golang/go/pull/30654 for this proposal, you wouldn't really need the reverse mapping, as tls.CipherSuites() would return a list with the structures that already contain a mapping from ID towards the struct which contains the name; so you could already filter like this:

// Filter any cipher suites using 3DES
mySuite := make([]uint16, 0)
for id, suite := range tls.CipherSuites() {
    if !strings.Contains(suite.Name, "3DES") {
      mySuite = append(mySuite, uint16(id))
    }
}

// Create my TLS Config
cfg := &tls.Config{
  CipherSuites: mySuite,
  MinVersion: tls.VersionTLS12,
}

the only thing that's missing is to know if the specific cipher is included in the list of default ciphers. So, @noneymous 's suggestion of exposing the flags would help. Is that a solution that would satisfy your requirements? I could add the flag exposure to https://github.com/golang/go/pull/30654 if needed.

[noneymous]

Hm, the "default cihpers" are actually all from the cipherSuites list which do not have the suiteDefaultOff flag. As I understand, defaultCipherSuites(), respectively its initiation function initDefaultCipherSuites(), does only set their preferred order.

So you could do the same:

for _, suite := range tls.CipherSuites() {
    if suite.flags&suiteDefaultOff != 0 {
        continue
    }
    // ...
}

[JAORMX]

@noneymous @jzelinskie alright, I pushed an update to https://github.com/golang/go/pull/30654 which makes the flags attribute public, as well as the relevant flags (which enables us to actually check the flags in each cipher suite). What do you think?

[noneymous]

@JAORMX @jzelinskie

I've added a pull request on my own, but I couldn't manage to pass the CLA -.- Does anyone of you want to copy and submit it?

Here is my suggestion: https://github.com/golang/go/pull/30739

• clean code
• code should be working
• should address all mentioned issues (dynamic retrieval, query by name/id, filtering default ciphers,...)
• minimum necessary changes, minimum invasive (added getter functions, exposed relevant flags and attributes)
• getters returning copies to avoid accidental manipulation of internal data
• i don't think we need to introduce a custom type CipherSuiteID, it is just bloating the code in my opintion, so i skipped it

best regards!

[jzelinskie]

FWIW, Kubernetes has an implementation of this mapping. Obviously it's not exactly the same, but it proves there is some value in maintaining a mapping.

https://github.com/kubernetes/kubernetes/pull/48859/commits/d7dbc96c70d480f0b81cd83ae3abd34b69c1e70d

I personally happy with or without this change because you can also solve #21167 by exposing the initialized defaults and just hardcoding which uint16s to filter.

[FiloSottile]

How about this 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
}

// CipherSuitesIncludingInsecure returns the list of cipher suites currently
// implemented by this package, including those disabled by default.
//
// Applications will usually want to ignore any CipherSuite for which
// CipherSuite.Insecure is true.
func CipherSuitesIncludingInsecure() []*CipherSuite

// CipherSuiteByID returns the CipherSuite with the given ID if it is
// currently implemented by this package. If the CipherSuite is found,
// the bool return value is true, and false otherwise.
func CipherSuiteByID(id uint16) (*CipherSuite, bool)

(BTW, it's easier to discuss API changes in the abstract first on GitHub, the implementation is not the important part, and reading PRs is harder. Anyway, the PR can't be merged until the proposal is accepted.)

Edited: changed CipherSuiteByID to return a bool, and added an Insecure field.

Edited: replaced MinVersion with SupportedVersions.

Edited: renamed CipherSuites to CipherSuitesIncludingInsecure.

[jzelinskie]

Looks pretty solid! Would a bool suffice instead of an error for the second return value in CipherSuiteByID? Not only is it more ergonomic, but it avoids having to export an error variable like ErrUnknownCipherSuite.

[FiloSottile]

Alternatively, we could use methods on a uint16 like this

type CipherSuite uint16
func (c CipherSuite) String() string
func CipherSuites() []CipherSuite

I love that it can provide a Stringer that supports unknown cipher suites, so you don't have to write your own fallback, and can just pass tls.CipherSuite(c) to Print.

However, it's not as extensible as a struct, and for example I already have no idea what the semantics of a MinVersion method should be for unknown cipher suites.

Unless someone can think of a way to make this work, let's go for https://github.com/golang/go/issues/30325#issuecomment-483863377.

[mholt]

We would probably use and benefit from a feature like this in Caddy.

Right now, we do this: https://github.com/mholt/caddy/blob/605787f6717602982f7f485353a7cb479b659802/caddytls/config.go#L457-L472

var SupportedCiphersMap = map[string]uint16{
    "ECDHE-ECDSA-AES256-GCM-SHA384":      tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
    "ECDHE-RSA-AES256-GCM-SHA384":        tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
    "ECDHE-ECDSA-AES128-GCM-SHA256":      tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
    "ECDHE-RSA-AES128-GCM-SHA256":        tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
    "ECDHE-ECDSA-WITH-CHACHA20-POLY1305": tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
    "ECDHE-RSA-WITH-CHACHA20-POLY1305":   tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
    "ECDHE-RSA-AES256-CBC-SHA":           tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
    "ECDHE-RSA-AES128-CBC-SHA":           tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
    "ECDHE-ECDSA-AES256-CBC-SHA":         tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
    "ECDHE-ECDSA-AES128-CBC-SHA":         tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
    "RSA-AES256-CBC-SHA":                 tls.TLS_RSA_WITH_AES_256_CBC_SHA,
    "RSA-AES128-CBC-SHA":                 tls.TLS_RSA_WITH_AES_128_CBC_SHA,
    "ECDHE-RSA-3DES-EDE-CBC-SHA":         tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
    "RSA-3DES-EDE-CBC-SHA":               tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,
}

I might love to see cipher suite names "standardized" (and frankly I wished we used underscores so they count as a single word when parsing and highlighting) beyond just a cryptic number.

However, there is something to be said for having only one true way of identifying cipher suite, being by their ID on the wire.

Then again, if being able to write out the full name of the cipher suite makes the configuration more readable and clearly shows which algorithms and key sizes are involved, perhaps that is safer in the long run.

I don't have much of an opinion on API at this point.

[colmmacc]

Cipher suite names are standardized, though they're not consistent. But the standard lives at ...

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

it would be neat to generate the map from this URL, or at least check that everything in IANA's list is accounted for in the map.

[davidben]

(Edit: Oops, I see @colmmacc and I crossed mid-air.)

Cipher suite names are standardized. There's an IANA registry and all. (We don't refer to them in RFCs by number. :-p Human-readable names are useful in prose.) The names of the Go constants with the underscores and _WITH_ are the standard ones.

Other than whether you include the TLS_ prefix, the only variation I've seen is just that OpenSSL has its own names, with the hyphens and weird inconsistently implicit RSA and CBC components. That's a non-standard OpenSSL-ism.

[FiloSottile]

@mholt I'm afraid we can't help you with the custom naming there. It makes the most sense to use the standard names, which match the constants in crypto/tls.

I took @jzelinskie's suggestion, and I also added an Insecure field, so even if you can't replicate the list of default cipher suites (which is too nuanced to capture), you can avoid bad cipher suites.

[mholt]

@FiloSottile

@mholt I'm afraid we can't help you with the custom naming there. It makes the most sense to use the standard names, which match the constants in crypto/tls.

Perfect, that's all I want; (I read my comment again) I was suggesting that we would want to _avoid_ custom naming.

[Lekensteyn]

Exposing the codepoints as ID and the IANA-registered names sounds fine to me. The name could for example be used in log messages.

Why expose "MinVersion" though, what is the intended use case? This seems like an internal detail that the user should not care about. If a cipher suite is specified in the configuration, but not supported by the protocol version or peer, it will be ignored. There is also ongoing work to mark TLS 1.0 and 1.1 as deprecated, leaving only TLS 1.2 and 1.3 as the future: https://datatracker.ietf.org/doc/draft-ietf-tls-oldversions-deprecate/
If you do decide to include a version, what about a SupportedVersions []uint16 field? That works better for TLS 1.3 even if it is not configurable.

I'm undecided about the Insecure field, how should applications process it? If it is only needed to check whether it works with HTTP/2, it probably does not belong here. What are the expected use cases?

[FiloSottile]

If you do decide to include a version, what about a SupportedVersions []uint16 field? That works better for TLS 1.3 even if it is not configurable.

Oh nice, I like that better, yeah.

Without knowing the supported versions, this can't be used to e.g. figure out if the peer supports ECDSA. All other information is in the name. You might also want to make sure you are not inadvertently disabling a protocol version by only listing incompatible suites.

I'm undecided about the Insecure field, how should applications process it? If it is only needed to check whether it works with HTTP/2, it probably does not belong here. What are the expected use cases?

I expect a common use of this will be to iterate over them, and to enable all of them except some (for whatever application concept of some). I don't want that to lead to people re-enabling RC4, and most importantly I want to retain a way to signal in the future that a suite is not ok anymore, without removing its implementation.

[Lekensteyn]

I expect a common use of this will be to iterate over them, and to enable all of them except some (for whatever application concept of some). I don't want that to lead to people re-enabling RC4, and most importantly I want to retain a way to signal in the future that a suite is not ok anymore, without removing its implementation.

I see, after exposing a list of cipher suites you are concerned about people enabling all possible cipher suites since they do not know better. If applications want to disable certain cipher suites or extend the defaults, an alternative would be providing a func RecommendedCipherSuites() []uint16 (or []*CipherSuite?). Recommended would remove the insecure cipher suites and maybe use a different order of priority while at it.

Do you plan to assign any significance to the order returned by CipherSuites()?

[davidben]

The Java SSLSocket API has a similar split (default vs supported), but applications still don't pay attention and enable all supported ones, which has caused some problems on Android.

Why is a function to return all cipher suites needed? It seems the main useful operations are:

1. Given a uint16, give me information about the cipher suite with that ID.
2. Given a string, give me information about the cipher suite with that name.
3. I want to use Go's default cipher suite preferences.

There already is a way to do (3). Just leave config.CipherSuites alone. Perhaps a caller wants to start with that and perturb it, so maybe add a function for returning that. But rather than returning everything implemented, it seems (1) and (2) could be served by their own functions. Sure, someone could still pass all 2^16 values into (1) and configure that, but that's unlikely to be done inadvertently, compared to just setting config.CipherSuites to tls.CipherSuites().

[FiloSottile]

I don't want to expose anything of the logic of default cipher suites, because I want to keep the ability to apply arbitrary logic to it, for example to change it based on Client Hello fingerprinting.

I do like the idea of dropping CipherSuites and adding CipherSuiteByName, but

1. people definitely want to do things like "configure the default set minus CBC" and I don't know how to enable that without yet another function,
2. I suspect an use might come up that we did not think about which requires the list.

A mitigating factor for people enabling them all is that it's a list you have to iterate over to extract the IDs from, and the docs will be screaming "Insecure" at you at the same time.

Here's an idea: CipherSuites(includeInsecure bool) []*CipherSuite. Unnamed bools are gross, but I am not sure I want to also add

const (
    withoutInsecure = 1
    includingInsecure = 2
)

[davidben]

Hrm. Isn't CipherSuites(false) more-or-less exposing the "default" list? Short of having crypto/tls support a filter language, it seems the minus-CBC use case just requires such a thing. It also needs the "default" order. "Default" in quotes because, as you note, the actual default may have extra behavior not embedded in the list. Though anyone doing this would lose that behavior anyway. (This awkwardness is part of why I dislike making such things configurable. It semi-constrains to behaviors expressible by that configuration.)

For CipherSuites(true), this may be a useful test: Suppose crypto/tls added some random bad cipher to support some niche thing. Should other callers' behavior change? CipherSuites(true) is only useful for use cases where the answer is yes and risks problems where the answer is no. The other ones either are unaffected or require one explicitly mention the new cipher in some way, by number or name.

[noneymous]

@FiloSottile https://github.com/golang/go/issues/30325#issuecomment-483863377 is good with me. It would be great to have such a useful API with so much information.

I suspect an use might come up that we did not think about which requires the list.

Exactly, like the one I'm here for ^^ Writing security tools I always want to enable all available ciphers. Or sometimes a certain kind of cipher in order to verify whether an endpoint is really accepting it.

@davidben if some people really want to fuck it up they can always go panic-mode and enable all ciphers. It is important that the default is secure and that they find some warnings along the way of changing the default.

Overall, I have no strong opinion about CipherSuites(includeInsecure bool) variant, but an additional idea: We could make CipherSuites() return the current secure list and add something like CipherSuitesInsecure() returning a complete list. People would by default use the original secure one and have an "insecurity indicator" in the new function's name. However, I understand, adding more public functions is not desirable either...

[davidben]

Testing tool use cases are complex.

The needs of production TLS stacks and testing tools, while not incompatible, pull in opposite directions. A production stack wants to limit itself to only good (or at least necessary) behaviors and minimize any complexity not needed by those behaviors. A testing tool wants maximum flexibility so that it can test questionable and even invalid behaviors.

My general feelings are that testing tools really need their own stacks (in BoringSSL, we run a bunch of tests using a fork of crypto/tls), but of course there is a wide range of testing behaviors, ranging from relatively cheap to very expensive and one can try to address some while still rejecting others. (Though I will note that CipherSuitesInsecure() is not necessary to build a testing tool. It just means you need to maintain your own list of which ciphers you are interested in testing. Indeed the set Go happens to implement is far from exhaustive, by design.)

I think that really boils down to what are the goals Go has for its standard library TLS implementation.

[noneymous]

It is less about writing tools testing SSL itself, but also layers above. Anything, where you don't primary care about the data confidentiality, where it is more important to be able to cover as many (or old) targets as possible. Web crawling, banner grabbing, HTTP security header checks, extraction of data via diverse protocols,... or any tool a penetration tester wants to build in order to automate something...

I see that golang currently is not as useful as Python in such cases (also because it only supports about 20 ciphers vs 60), but that doesn't mean we cannot improve in that direction.

[FiloSottile]

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]

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]

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

[dolmen]

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?

[FiloSottile]

@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]

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_ECDSA_WITH_CHACHA20_POLY1305_SHA256

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

[davidben]

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]

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

[FiloSottile]

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.

[mholt]

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]

(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.

[FiloSottile]

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

Leaving open a week for final comments.