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

Jump to bottom

crypto/tls: cipher suite configuration is ignored breaking existing code #31072

Closed
4a6f656c opened this issue Mar 27, 2019 · 4 comments
Closed

crypto/tls: cipher suite configuration is ignored breaking existing code #31072

4a6f656c opened this issue Mar 27, 2019 · 4 comments
Labels
FrozenDueToAge

Comments

@4a6f656c
Copy link
Contributor

4a6f656c commented Mar 27, 2019 •
edited

What version of Go are you using (go version)?

$ go version
go version devel +724a86fced Wed Mar 27 02:37:56 2019 +0000 openbsd/amd64

What did you do?

package main

import (
        "crypto/tls"
        "log"
)

func main() {
        tlsCfg := &tls.Config{
                CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
                //MaxVersion: tls.VersionTLS12,
                ServerName: "www.cloudflare.com",
        }

        conn, err := tls.Dial("tcp", "www.cloudflare.com:443", tlsCfg)
        if err != nil {
                log.Fatalf("Failed to dial: %v", err)
        }
        defer conn.Close()

        cs := conn.ConnectionState()
        log.Printf("Connected with TLS 0x%x using cipher suite 0x%x\n", cs.Version, cs.CipherSuite)
}

What did you expect to see?

Connected with TLS 0x303 using cipher suite 0xc02f

(which is what occurs if compiled with Go 1.12)

What did you see instead?

Connected with TLS 0x304 using cipher suite 0x1301

Based on the crypto/tls code and documentation for crypto/tls.Config, this is an intentional change, however it changes the behaviour of existing code - previously when compiled and run it would adhere to the configuration in CipherSuites, however it is now silently ignored and the behaviour changes to one that is not intended by the author.

This means that the connection is potentially being established without using one of the intended cipher suites - there are various cases where this is a bad thing. Obviously it can be worked around by pinning the maximum version (i.e. uncommenting the MaxVersion in the above code), however this still requires a change to existing code and is probably worse than having pinned cipher suites.

I'm also somewhat concerned by the fact that there is no way to configure the cipher suites for TLS 1.3.
@agnivade
Copy link
Contributor

@FiloSottile

@FiloSottile
Copy link
Contributor

FiloSottile commented Mar 27, 2019 •
edited

TLS 1.3 cipher suites are completely separate from 1.2 ones, so the only option to keep the same behavior would be not to enable TLS 1.3, which is what MaxVersion is for.

Most (all?) users set CipherSuites to stick to secure or faster ones, and thankfully all TLS 1.3 ciphersuites are secure, and we automatically prefer the faster ones.

Duplicate of #29349 (comment)

@4a6f656c
Copy link
Contributor Author

So you're saying that it is fine that this breaks code that works with Go 1.12 (and probably every Go 1 release), now that the previous behaviour/configuration is ignored?

Unfortunately, cipher suite selection is not just about security and performance - there are also contractual and compliance reasons. At the very least, it seems that if TLS 1.3 is going to be enabled and completely ignore CipherSuites, if the configuration has CipherSuites specified and a zero value is given for MaxVersion, the maximum version should be inferred as being TLS 1.2 (which would at least restore the previous behaviour without code change).

Your argument re TLS 1.3 cipher suites being completely separate from 1.2, also largely applies to previous versions of TLS - there are cipher suites that are TLS 1.2 only and cannot be used with TLS 1.0 or TLS 1.1, yet their configuration is via the same single mechanism. And when you say we automatically prefer the faster ones - the question is for who (there are situations where you may be willing to burn CPU cycles server-side to benefit some clients)?

@FiloSottile
Copy link
Contributor

if the configuration has CipherSuites specified and a zero value is given for MaxVersion, the maximum version should be inferred as being TLS 1.2

I agree that this is the only way to not change behavior at all, but in practice it would mean many applications which only set CipherSuites for performance or security reasons would find TLS 1.3 disabled by a subtle behavior. Also, it means that you must set MaxVersion to configure the TLS 1.2 cipher suites and still have TLS 1.3, which is confusing.

Instead, if one has contractual and compliance reasons to maintain the cipher suites values, I think it's reasonable to require them to also set MaxVersion.

PreferServerCipherSuites controls if the client or the server preference is prioritized.

@golang golang locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@FiloSottile @agnivade @4a6f656c @gopherbot