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

crypto/tls: make TLS 1.3 opt-out for Go 1.13 #30055

Closed
rsc opened this issue Feb 1, 2019 · 18 comments
Closed

crypto/tls: make TLS 1.3 opt-out for Go 1.13 #30055

rsc opened this issue Feb 1, 2019 · 18 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Feb 1, 2019

It seems clear from the history of filed issues that some users will experience minor disruptions when switching to TLS 1.3. For disruptive changes like that, we typically do one or more releases in which the new feature is opt-in, with a prominent announcement about when it will turn on by default. Then we do at least one release with the feature opt-out. (Past examples include HTTP/2, vendor directories, and modules.)

Especially given how central TLS is to so many Go users (any HTTPS client or server!), it seems like we should do that for TLS 1.3 too:

  • Make TLS 1.3 available in Go 1.12 but opt-in (off by default).
  • Make clear in the release notes and package docs how to turn on TLS 1.3.
  • Make clear in the release notes that TLS 1.3 will turn on by default in Go 1.13.
  • Make TLS 1.3 in Go 1.13 opt-out (on by default).

Following the HTTP/2 example my suggestion would be to use GODEBUG=tls13=1 for opt-in now, and GODEBUG=tls13=0 for opt-out later.

/cc @FiloSottile

@rsc rsc added this to the Go1.12 milestone Feb 1, 2019
@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

Following the HTTP/2 example my suggestion would be to use GODEBUG=tls13=1 for opt-in now

That's fine as long as there's also a way to enable it in code. Via tls.Config somehow? (MaxVersion and MinVersion both seem wrong.) Or at least with os.Setenv at runtime, but that means crypto/tls would need to keep calling Getenv. New bool on tls.Config would be kinda unfortunate, but maybe?

@neild
Copy link
Contributor

neild commented Feb 1, 2019

Should this apply to RSA-PSS as well?

@mvdan
Copy link
Member

mvdan commented Feb 1, 2019

New bool on tls.Config would be kinda unfortunate, but maybe?

A perhaps less specific version could be Debug []string, e.g. tlsConfig.Debug = []string{"tls13=1"}. Then it can be reused for more on/off switches in future Go releases, and its behavior is consistent with GODEBUG.

@FiloSottile
Copy link
Contributor

FiloSottile commented Feb 1, 2019

MaxVersion feels wrong semantically, and in practice it would lock applications to TLS 1.3 when TLS 1.4 ever comes out. I'm worried that tlsConfig.Debug would be a slippery slope, we get requests all the time to add all kinds of knobs which are inconsistent with the simplicity goal of crypto/tls, and that would provide an extra avenue for them.

How about GODEBUG=tls13=1, read once per tls.Config? It would allow os.Setenv in code, it would probably have acceptable performance, and still let applications test canaries without modifying code. It would probably have to be inherited via GetConfigForClient if one os.Getenv per connection is unacceptable.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

MaxVersion = 0xFFFF would be my vote. Or some other unused constant to mean MaxBeta.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

But users likely wouldn't be able to reach all such configs so the environment way probably best if Setenv works.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/160998 mentions this issue: crypto/tls: disable RSA-PSS in TLS 1.2

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/160997 mentions this issue: crypto/tls: make TLS 1.3 opt-in

@FiloSottile
Copy link
Contributor

CL 160997 just uses os.Getenv at every connection. On Unix it takes a brief lock but uses an internal view of the environment, so there is no syscall. On Windows it makes a syscall. We can test the performance before adding complexity the day before the RC.

gopherbot pushed a commit that referenced this issue Feb 7, 2019
Updates #30055

Change-Id: If68615c8e9daa4226125dcc6a6866f29f3cfeef1
Reviewed-on: https://go-review.googlesource.com/c/160997
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Russ Cox <[email protected]>
gopherbot pushed a commit that referenced this issue Feb 7, 2019
Most of the issues that led to the decision on #30055 were related to
incompatibility with or faulty support for RSA-PSS (#29831, #29779,
v1.5 signatures). RSA-PSS is required by TLS 1.3, but is also available
to be negotiated in TLS 1.2.

Altering TLS 1.2 behavior based on GODEBUG=tls13=1 feels surprising, so
just disable RSA-PSS entirely in TLS 1.2 until TLS 1.3 is on by default,
so breakage happens all at once.

Updates #30055

Change-Id: Iee90454a20ded8895e5302e8bcbcd32e4e3031c2
Reviewed-on: https://go-review.googlesource.com/c/160998
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
@FiloSottile FiloSottile modified the milestones: Go1.12, Go1.13 Feb 7, 2019
@FiloSottile
Copy link
Contributor

Done for Go 1.12, milestoned to Go 1.13 to switch to opt-out.

@crvv
Copy link
Contributor

crvv commented Feb 13, 2019

I think it is worth to document how to use os.Setenv to set this environment variable.

I guess this might work

os.Setenv("GODEBUG", os.Getenv("GODEBUG")+",tls13=1")

But os.Setenv("GODEBUG", "tls13=1") will overwrite other GODEBUG like GODEBUG=http2server=0.

If some other goroutines are also setting GODEBUG, there can be a race condition.

Alternately, maybe using another env is better. And os.Setenv("GOTLS13", "1") is mostly OK.

@crvv
Copy link
Contributor

crvv commented Feb 13, 2019

According to #13611
GODEBUG usage should also be documented in https://tip.golang.org/pkg/runtime/

@bradfitz
Copy link
Contributor

@crvv, I sent https://go-review.googlesource.com/c/go/+/162360

nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 18, 2019
Updates golang#30055

Change-Id: If68615c8e9daa4226125dcc6a6866f29f3cfeef1
Reviewed-on: https://go-review.googlesource.com/c/160997
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Russ Cox <[email protected]>
nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 18, 2019
Most of the issues that led to the decision on golang#30055 were related to
incompatibility with or faulty support for RSA-PSS (golang#29831, golang#29779,
v1.5 signatures). RSA-PSS is required by TLS 1.3, but is also available
to be negotiated in TLS 1.2.

Altering TLS 1.2 behavior based on GODEBUG=tls13=1 feels surprising, so
just disable RSA-PSS entirely in TLS 1.2 until TLS 1.3 is on by default,
so breakage happens all at once.

Updates golang#30055

Change-Id: Iee90454a20ded8895e5302e8bcbcd32e4e3031c2
Reviewed-on: https://go-review.googlesource.com/c/160998
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/163039 mentions this issue: [release-branch.go1.12] crypto/tls: don't select RSA-PSS for client certificates in TLS 1.2

nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 20, 2019
Updates golang#30055

Change-Id: If68615c8e9daa4226125dcc6a6866f29f3cfeef1
Reviewed-on: https://go-review.googlesource.com/c/160997
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Russ Cox <[email protected]>
nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 20, 2019
Most of the issues that led to the decision on golang#30055 were related to
incompatibility with or faulty support for RSA-PSS (golang#29831, golang#29779,
v1.5 signatures). RSA-PSS is required by TLS 1.3, but is also available
to be negotiated in TLS 1.2.

Altering TLS 1.2 behavior based on GODEBUG=tls13=1 feels surprising, so
just disable RSA-PSS entirely in TLS 1.2 until TLS 1.3 is on by default,
so breakage happens all at once.

Updates golang#30055

Change-Id: Iee90454a20ded8895e5302e8bcbcd32e4e3031c2
Reviewed-on: https://go-review.googlesource.com/c/160998
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/163080 mentions this issue: Revert "crypto/tls: disable RSA-PSS in TLS 1.2"

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/163081 mentions this issue: crypto/tls: enable TLS 1.3 by default

gopherbot pushed a commit that referenced this issue Feb 21, 2019
…ertificates in TLS 1.2

In https://golang.org/cl/160998, RSA-PSS was disabled for
(most of) TLS 1.2. One place where we can't disable it is in a Client
Hello which offers both TLS 1.2 and 1.3: RSA-PSS is required by TLS 1.3,
so to offer TLS 1.3 we need to offer RSA-PSS, even if the server might
select TLS 1.2.

The good news is that we want to disable RSA-PSS mostly when we are the
signing side, as that's where broken crypto.Signer implementations will
bite us. So we can announce RSA-PSS in the Client Hello, tolerate the
server picking TLS 1.2 and RSA-PSS for their signatures, but still not
do RSA-PSS on our side if asked to provide a client certificate.

Client-TLSv12-ClientCert-RSA-PSS-Disabled changed because it was indeed
actually using RSA-PSS.

Updates #30055

Change-Id: I5ecade744b666433b37847abf55e1f08089b21d4
Reviewed-on: https://go-review.googlesource.com/c/163039
Reviewed-by: Andrew Bonventre <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
gopherbot pushed a commit that referenced this issue Feb 27, 2019
In Go 1.13 we will enable RSA-PSS in TLS 1.2 at the same time as we make
TLS 1.3 enabled by default.

This reverts commit 7ccd358.

Updates #30055

Change-Id: I6f2ddf7652d1172a6b29f4e335ff3a71a89974bc
Reviewed-on: https://go-review.googlesource.com/c/163080
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
gopherbot pushed a commit that referenced this issue Feb 27, 2019
Updates #30055

Change-Id: I3e79dd7592673c5d76568b0bcded6c391c3be6b3
Reviewed-on: https://go-review.googlesource.com/c/163081
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
@bcmills bcmills changed the title crypto/tls: make TLS 1.3 opt-in for Go 1.12 crypto/tls: make TLS 1.3 opt-out for Go 1.13 Mar 1, 2019
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 1, 2019
jonathonlacher pushed a commit to jonathonlacher/go that referenced this issue Mar 8, 2019
…ertificates in TLS 1.2

In https://golang.org/cl/160998, RSA-PSS was disabled for
(most of) TLS 1.2. One place where we can't disable it is in a Client
Hello which offers both TLS 1.2 and 1.3: RSA-PSS is required by TLS 1.3,
so to offer TLS 1.3 we need to offer RSA-PSS, even if the server might
select TLS 1.2.

The good news is that we want to disable RSA-PSS mostly when we are the
signing side, as that's where broken crypto.Signer implementations will
bite us. So we can announce RSA-PSS in the Client Hello, tolerate the
server picking TLS 1.2 and RSA-PSS for their signatures, but still not
do RSA-PSS on our side if asked to provide a client certificate.

Client-TLSv12-ClientCert-RSA-PSS-Disabled changed because it was indeed
actually using RSA-PSS.

Updates golang#30055

Change-Id: I5ecade744b666433b37847abf55e1f08089b21d4
Reviewed-on: https://go-review.googlesource.com/c/163039
Reviewed-by: Andrew Bonventre <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
@FiloSottile FiloSottile modified the milestones: Go1.13, Go1.14 Apr 29, 2019
@FiloSottile
Copy link
Contributor

Milestoned to 1.14 to remove the opt-out.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/191999 mentions this issue: crypto/tls: remove TLS 1.3 opt-out

tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
Fixes golang#30055

Change-Id: If757c43b52fc7bf62b0afb1c720615329fb5569d
Reviewed-on: https://go-review.googlesource.com/c/go/+/191999
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
Fixes golang#30055

Change-Id: If757c43b52fc7bf62b0afb1c720615329fb5569d
Reviewed-on: https://go-review.googlesource.com/c/go/+/191999
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
@golang golang locked and limited conversation to collaborators Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

8 participants