/go

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: add support for TLS 1.3 #9671

Closed
mikioh opened this Issue Jan 23, 2015 · 38 comments

Comments

Assignees

@FiloSottile FiloSottile

Labels
Projects
None yet
Milestone
  Go1.12
@mikioh
Contributor

mikioh commented Jan 23, 2015

See https://tools.ietf.org/html/draft-ietf-tls-tls13.

Coexistence of IPv4 and IPv6 harms the net package.
Coexistence of HTTP/1.x and HTTP/2 will harm the net/http package.
For now looks coexistence of TLS 1.2 and 1.3 won't harm the crypto/tls package.
How about a variety of compositions on HTTP over TLS over IP?
@bradfitz

This comment has been minimized.

Sign in to view
Member

bradfitz commented Jan 23, 2015

Coexistence of IPv4 and IPv6 harms the net package.

Huh?

Coexistence of HTTP/1.x and HTTP/2 will harm the net/http package.

Huh?

This whole bug report seems to start on unfounded premises, or at least isn't clear.
@rsc

This comment has been minimized.

Sign in to view
Contributor

rsc commented Apr 10, 2015

The draft is still being updated. We can talk about having TLS 1.3 once it's actually an RFC.

[I have no idea what the text in the issue report body is supposed to mean, but the issue title is at least clear.]

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@joshuarubin

This comment has been minimized.

Sign in to view
Contributor

joshuarubin commented Sep 26, 2016

maybe it's time to consider this again?
@minux

This comment has been minimized.

Sign in to view
Member

minux commented Sep 26, 2016

@FiloSottile

This comment has been minimized.

Sign in to view
Member

FiloSottile commented Nov 12, 2016

https://go-review.googlesource.com/#/c/33115/ opened a branch for TLS 1.3 development.
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 12, 2016

CL https://golang.org/cl/33164 mentions this issue.
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 19, 2016

CL https://golang.org/cl/33416 mentions this issue.
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 20, 2016

CL https://golang.org/cl/33419 mentions this issue.
@mholt

This comment has been minimized.

Sign in to view

mholt commented Feb 23, 2017 •
edited

I see this is still "Unplanned" -- any possibility of getting this on track for Go 1.9? (Or is the final draft still too far out. I can never figure out where to find the status of these things.)

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Unplanned Feb 23, 2017

@bradfitz

This comment has been minimized.

Sign in to view
Member

bradfitz commented Feb 23, 2017

@FiloSottile is working on this. Status?
@FiloSottile

This comment has been minimized.

Sign in to view
Member

FiloSottile commented Feb 23, 2017

The server codebase we are using is pretty battle tested and complete now, so the commits starting crypto/tls in https://github.com/cloudflare/tls-tris will make their way into CLs after cleanup. Some already did.

If @agl has the review bandwidth, I can probably power through the client implementation in March. (Or maybe we can consider shipping server first?)

BTW, @bradfitz it would be useful if I could submit for review on Gerrit commits I don't Author. There are a couple in there made by other people that agreed to have them submitted and signed the CLA.

(Feel free to assign me this issue.)

@bradfitz bradfitz assigned FiloSottile Feb 24, 2017

@bradfitz

This comment has been minimized.

Sign in to view
Member

bradfitz commented Feb 24, 2017

@FiloSottile, email me the list of author email addresses you wish to push forged commits to Gerrit with. I'll then check their CLAs and verify they're cool with you spoofing them. (Maybe cc them on your email to me and have them confirm?)

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jun 29, 2017

@bradfitz bradfitz added FeatureRequest NeedsFix labels Jun 29, 2017

@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Jul 13, 2017

CL https://golang.org/cl/48455 mentions this issue.
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Jul 13, 2017

CL https://golang.org/cl/48454 mentions this issue.

@ginuerzh ginuerzh referenced this issue Aug 25, 2017

Open

关于TLS的建议 #151

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 15, 2017

Lekensteyn added a commit to Lekensteyn/go that referenced this issue Nov 23, 2017

@Lekensteyn
crypto/tls: add RSASSA-PSS support for handshake messages 
This adds support for RSASSA-PSS signatures in handshake messages as
required by TLS 1.3. Even if TLS 1.2 is negotiated, it must support PSS
when advertised in the Client Hello (this will be done later as the
testdata will change).

Updates golang#9671

Change-Id: I8006b92e017453ae408c153233ce5ccef99b5c3f
c1192f5

Lekensteyn added a commit to Lekensteyn/go that referenced this issue Nov 23, 2017

@Lekensteyn
[RFC] crypto/tls: advertise PSS support, add tests 
PSS signatures in X509 certificates are already supported, and now that
handshake messages also support PSS, let's advertise it in the client
and enable it for the server.

Add test that checks for PSS signatures in (1) handshake messages and
(2) certificates.

Updates golang#9671

Tested with:

    go test -race crypto/tls -v -test.run TestHandshakeClientCertRSAPSS -args -update

TODO update many other tests since signature_algorithms is updated.
RFC force s_server to disable PSS for other tests, and use PKCS#1 v1.5?
RFC ok to change tests to require openssl from git?

Change-Id: I1b2ce2d13a07f5dda98b918313f3c581ce1d7b1d
7b9830d

Lekensteyn added a commit to Lekensteyn/go that referenced this issue Nov 23, 2017

@Lekensteyn
crypto/tls: add RSASSA-PSS support for handshake messages 
This adds support for RSASSA-PSS signatures in handshake messages as
required by TLS 1.3. Even if TLS 1.2 is negotiated, it must support PSS
when advertised in the Client Hello (this will be done later as the
testdata will change).

Updates golang#9671

Change-Id: I8006b92e017453ae408c153233ce5ccef99b5c3f
02a7d0b

Lekensteyn added a commit to Lekensteyn/go that referenced this issue Nov 23, 2017

@Lekensteyn
[RFC] crypto/tls: advertise PSS support, add tests 
PSS signatures in X509 certificates are already supported, and now that
handshake messages also support PSS, let's advertise it in the client
and enable it for the server.

Add test that checks for PSS signatures in (1) handshake messages and
(2) certificates.

Updates golang#9671

Tested with:

    go test -race crypto/tls -v -test.run TestHandshakeClientCertRSAPSS -args -update

TODO update many other tests since signature_algorithms is updated.
RFC force s_server to disable PSS for other tests, and use PKCS#1 v1.5?
RFC ok to change tests to require openssl from git?

Change-Id: I1b2ce2d13a07f5dda98b918313f3c581ce1d7b1d
8f6324b
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 23, 2017

Change https://golang.org/cl/79738 mentions this issue: [RFC] crypto/tls: advertise PSS support, add tests

gopherbot pushed a commit that referenced this issue Nov 2, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 version-specific messages 
Note that there is significant code duplication due to extensions with
the same format appearing in different messages in TLS 1.3. This will be
cleaned up in a future refactor once CL 145317 is merged.

Enforcing the presence/absence of each extension in each message is left
to the upper layer, based on both protocol version and extensions
advertised in CH and CR. Duplicated extensions and unknown extensions in
SH, EE, HRR, and CT will be tightened up in a future CL.

The TLS 1.2 CertificateStatus message was restricted to accepting only
type OCSP as any other type (none of which are specified so far) would
have to be negotiated.

Updates #9671

Change-Id: I7c42394c5cc0af01faa84b9b9f25fdc6e7cfbb9e
Reviewed-on: https://go-review.googlesource.com/c/145477
Reviewed-by: Adam Langley <agl@golang.org>
0663fe9

gopherbot pushed a commit that referenced this issue Nov 2, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 version negotiation 
RFC 8446 recommends using the supported_versions extension to negotiate
lower versions as well, so begin by implementing it to negotiate the
currently supported versions.

Note that pickTLSVersion was incorrectly negotiating the ServerHello
version down on the client. If the server had illegally sent a version
higher than the ClientHello version, the client would have just
downgraded it, hopefully failing later in the handshake.

In TestGetConfigForClient, we were hitting the record version check
because the server would select TLS 1.1, the handshake would fail on the
client which required TLS 1.2, which would then send a TLS 1.0 record
header on its fatal alert (not having negotiated a version), while the
server would expect a TLS 1.1 header at that point. Now, the client gets
to communicate the minimum version through the extension and the
handshake fails on the server.

Updates #9671

Change-Id: Ie33c7124c0c769f62e10baad51cbed745c424e5b
Reviewed-on: https://go-review.googlesource.com/c/146217
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
7f5dce0

gopherbot pushed a commit that referenced this issue Nov 2, 2018

@FiloSottile
crypto/tls: advertise and accept rsa_pss_rsae signature algorithms 
crypto/x509 already supports PSS signatures (with rsaEncryption OID),
and crypto/tls support was added in CL 79736. Advertise support for the
algorithms and accept them as a peer.

Note that this is about PSS signatures from regular RSA public keys.
RSA-PSS only public keys (with RSASSA-PSS OID) are supported in neither
crypto/tls nor crypto/x509. See RFC 8446, Section 4.2.3.

testdata/Server-TLSv12-ClientAuthRequested* got modified because the
CertificateRequest carries the supported signature algorithms.

The net/smtp tests changed because 512 bits keys are too small for PSS.

Based on Peter Wu's CL 79738, who did all the actual work in CL 79736.

Updates #9671

Change-Id: I4a31e9c6e152ff4c50a5c8a274edd610d5fff231
Reviewed-on: https://go-review.googlesource.com/c/146258
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
ee7e443

gopherbot pushed a commit that referenced this issue Nov 2, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 client handshake (base) 
Implement a basic TLS 1.3 client handshake, only enabled if explicitly
requested with MaxVersion.

This CL intentionally leaves for future CLs:
  - PSK modes and resumption
  - client authentication
  - post-handshake messages
  - downgrade protection
  - KeyLogWriter support

Updates #9671

Change-Id: Ieb6130fb6f25aea4f0d39e3a2448dfc942e1de7a
Reviewed-on: https://go-review.googlesource.com/c/146559
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
4caa127

gopherbot pushed a commit that referenced this issue Nov 2, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 server handshake (base) 
Implement a basic TLS 1.3 server handshake, only enabled if explicitly
requested with MaxVersion.

This CL intentionally leaves for future CLs:
  - PSK modes and resumption
  - client authentication
  - compatibility mode ChangeCipherSpecs
  - early data skipping
  - post-handshake messages
  - downgrade protection
  - KeyLogWriter support
  - TLS_FALLBACK_SCSV processing

It also leaves a few areas up for a wider refactor (maybe in Go 1.13):
  - the certificate selection logic can be significantly improved,
    including supporting and surfacing signature_algorithms_cert, but
    this isn't new in TLS 1.3 (see comment in processClientHello)
  - handshake_server_tls13.go can be dried up and broken into more
    meaningful, smaller functions, but it felt premature to do before
    PSK and client auth support
  - the monstrous ClientHello equality check in doHelloRetryRequest can
    get both cleaner and more complete with collaboration from the
    parsing layer, which can come at the same time as extension
    duplicates detection

Updates #9671

Change-Id: Id9db2b6ecc2eea21bf9b59b6d1d9c84a7435151c
Reviewed-on: https://go-review.googlesource.com/c/147017
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
c21ba09
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 5, 2018

Change https://golang.org/cl/147420 mentions this issue: crypto/tls: implement TLS 1.3 PSK authentication (client side)
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 5, 2018

Change https://golang.org/cl/147417 mentions this issue: crypto/tls: implement TLS 1.3 KeyLogWriter support
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 5, 2018

Change https://golang.org/cl/147418 mentions this issue: crypto/tls: implement TLS 1.3 KeyUpdate messages
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 5, 2018

Change https://golang.org/cl/147419 mentions this issue: crypto/tls: implement TLS 1.3 middlebox compatibility mode
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 5, 2018

Change https://golang.org/cl/147445 mentions this issue: crypto/tls: implement TLS 1.3 PSK authentication (server side)
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 6, 2018

Change https://golang.org/cl/147599 mentions this issue: crypto/tls: implement TLS 1.3 client authentication
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 6, 2018

Change https://golang.org/cl/147617 mentions this issue: crypto/tls: implement TLS 1.3 downgrade protection
@gopherbot

This comment has been minimized.

Sign in to view

gopherbot commented Nov 6, 2018

Change https://golang.org/cl/147638 mentions this issue: crypto/tls: enable TLS 1.3 and update tests

@deepthi deepthi referenced this issue Nov 7, 2018

Merged

Count TLS/non-TLS connections separately, by TLS version #4341

@timcooijmans

This comment has been minimized.

Sign in to view
Contributor

timcooijmans commented Nov 9, 2018

@FiloSottile With this last commit, is TLS 1.3 support ready for testing?
@FiloSottile

This comment has been minimized.

Sign in to view
Member

FiloSottile commented Nov 9, 2018 •
edited

Once it gets merged (and this issue closed), you are very welcome to test TLS 1.3 by using Go master. Please file feedback and bugs as new GitHub issues and tag me. Note that the codebase will undergo much more testing before we are ok with shipping it in 1.12, so you shouldn't rely on its security the same way you shouldn't rely on master to be bug free.

gopherbot pushed a commit that referenced this issue Nov 12, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 KeyLogWriter support 
Also, add support for the SSLKEYLOGFILE environment variable to the
tests, to simplify debugging of unexpected failures.

Updates #9671

Change-Id: I20a34a5824f083da93097b793d51e796d6eb302b
Reviewed-on: https://go-review.googlesource.com/c/147417
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
29b01d5

gopherbot pushed a commit that referenced this issue Nov 12, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 KeyUpdate messages 
Since TLS 1.3 delivers handshake messages (including KeyUpdate) after
the handshake, the want argument to readRecord had became almost
pointless: it only meant something when set to recordTypeChangeCipherSpec.
Replaced it with a bool to reflect that, and added two shorthands to
avoid anonymous bools in calls.

Took the occasion to simplify and formalize the invariants of readRecord.

The maxConsecutiveEmptyRecords loop became useless when readRecord
started retrying on any non-advancing record in CL 145297.

Replaced panics with errors, because failure is better than undefined
behavior, but contained failure is better than a DoS vulnerability. For
example, I suspect the panic at the top of readRecord was reachable from
handleRenegotiation, which calls readHandshake with handshakeComplete
false. Thankfully it was not a panic in 1.11, and it's allowed now.

Removed Client-TLSv13-RenegotiationRejected because OpenSSL isn't
actually willing to ask for renegotiation over TLS 1.3, the expected
error was due to NewSessionTicket messages, which didn't break the rest
of the tests because they stop too soon.

Updates #9671

Change-Id: I297a81bde5c8020a962a92891b70d6d70b90f5e3
Reviewed-on: https://go-review.googlesource.com/c/147418
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
db27e78

gopherbot pushed a commit that referenced this issue Nov 12, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 middlebox compatibility mode 
Looks like the introduction of CCS records in the client second flight
gave time to s_server to send NewSessionTicket messages in between the
client application data and close_notify. There seems to be no way of
turning NewSessionTicket messages off, neither by not sending a
psk_key_exchange_modes extension, nor by command line flag.

Interleaving the client write like that tickled an issue akin to #18701:
on Windows, the client reaches Close() before the last record is drained
from the send buffer, the kernel notices and resets the connection,
cutting short the last flow. There is no good way of synchronizing this,
so we sleep for a RTT before calling close, like in CL 75210. Sigh.

Updates #9671

Change-Id: I44dc1cca17b373695b5a18c2741f218af2990bd1
Reviewed-on: https://go-review.googlesource.com/c/147419
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
dc0be72

gopherbot pushed a commit that referenced this issue Nov 12, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 PSK authentication (client side) 
Also check original certificate validity when resuming TLS 1.0–1.2. Will
refuse to resume a session if the certificate is expired or if the
original connection had InsecureSkipVerify and the resumed one doesn't.

Support only PSK+DHE to protect forward secrecy even with lack of a
strong session ticket rotation story.

Tested with NSS because s_server does not provide any way of getting the
same session ticket key across invocations. Will self-test like TLS
1.0–1.2 once server side is implemented.

Incorporates CL 128477 by @santoshankr.

Fixes #24919
Updates #9671

Change-Id: Id3eaa5b6c77544a1357668bf9ff255f3420ecc34
Reviewed-on: https://go-review.googlesource.com/c/147420
Reviewed-by: Adam Langley <agl@golang.org>
d669cc4

gopherbot pushed a commit that referenced this issue Nov 12, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 PSK authentication (server side) 
Added some assertions to testHandshake, but avoided checking the error
of one of the Close() because the one that would lose the race would
write the closeNotify to a connection closed on the other side which is
broken on js/wasm (#28650). Moved that Close() after the chan sync to
ensure it happens second.

Accepting a ticket with client certificates when NoClientCert is
configured is probably not a problem, and we could hide them to avoid
confusing the application, but the current behavior is to skip the
ticket, and I'd rather keep behavior changes to a minimum.

Updates #9671

Change-Id: I93b56e44ddfe3d48c2bef52c83285ba2f46f297a
Reviewed-on: https://go-review.googlesource.com/c/147445
Reviewed-by: Adam Langley <agl@golang.org>
6435d0c

gopherbot pushed a commit that referenced this issue Nov 12, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 client authentication 
Note that the SignatureSchemes passed to GetClientCertificate in TLS 1.2
are now filtered by the requested certificate type. This feels like an
improvement anyway, and the full list can be surfaced as well when
support for signature_algorithms_cert is added, which actually matches
the semantics of the CertificateRequest signature_algorithms in TLS 1.2.

Also, note a subtle behavior change in server side resumption: if a
certificate is requested but not required, and the resumed session did
not include one, it used not to invoke VerifyPeerCertificate. However,
if the resumed session did include a certificate, it would. (If a
certificate was required but not in the session, the session is rejected
in checkForResumption.) This inconsistency could be unexpected, even
dangerous, so now VerifyPeerCertificate is always invoked. Still not
consistent with the client behavior, which does not ever invoke
VerifyPeerCertificate on resumption, but it felt too surprising to
entirely change either.

Updates #9671

Change-Id: Ib2b0dbc30e659208dca3ac07d6c687a407d7aaaf
Reviewed-on: https://go-review.googlesource.com/c/147599
Reviewed-by: Adam Langley <agl@golang.org>
106db71

gopherbot pushed a commit that referenced this issue Nov 12, 2018

@FiloSottile
crypto/tls: implement TLS 1.3 downgrade protection 
TLS_FALLBACK_SCSV is extremely fragile in the presence of sparse
supported_version, but gave it the best try I could.

Set the server random canaries but don't check them yet, waiting for the
browsers to clear the way of misbehaving middleboxes.

Updates #9671

Change-Id: Ie55efdec671d639cf1e716acef0c5f103e91a7ce
Reviewed-on: https://go-review.googlesource.com/c/147617
Reviewed-by: Adam Langley <agl@golang.org>
46d4aa2

@gopherbot gopherbot closed this in 30cc978 Nov 12, 2018

@tie

This comment has been minimized.

Sign in to view

tie commented Nov 15, 2018

@FiloSottile, thanks for your awesome work!

I've looked through the commits and didn't find anything related to the encrypted SNI. Are there any plans on implementing the spec? Mozilla and Cloudflare have already implemented ESNI (though it's currently not enabled by default in Firefox).

@htfy96 htfy96 referenced this issue Nov 15, 2018

Open

TLS 1.3 #2080

@FiloSottile

This comment has been minimized.

Sign in to view
Member

FiloSottile commented Nov 16, 2018 •
edited

We generally wait for the browsers to experiment with new TLS features before implementing them, and anyway we are extremely unlikely to implement an Internet-Draft, so there are no plans for encrypted SNI.

Also, crypto/tls has a high benefit/complexity bar, so we will want to wait until encrypted SNI is widely deployed before implementing it.

@slanterns slanterns referenced this issue Nov 17, 2018

Open

Feature Request: TLS 1.3 Support with Encrypted SNI #35

@htfy96 htfy96 referenced this issue Nov 17, 2018

Open

TLS 1.3 support #28

@francislavoie francislavoie referenced this issue Nov 18, 2018

Open

feature request: encrypted SNI #2327

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment