crypto/tls: add (*tls.Conn).HandshakeContext and add context to ClientHelloInfo and CertificateRequestInfo #32406

johanbrandhorst · 2019-06-03T10:47:04Z

Proposal

I propose an unexported context.Context field is added to the ClientHelloInfo and CertificateRequestInfo crypto/tls types. We should also add a Context() context.Context method to these types to access this context. Further, we should add a new method, HandshakeContext(context.Context) error to the tls.Conn struct, which will be used to propagate a context down the handshake call stack. The existing Handshake() error would call to the new method with a context.Background() context.

Standard library uses of (*tls.Conn).Handshake() should be moved over to the new method, where appropriate. For example, it is not clear that it is appropriate to change the existing Read and Write methods on the *tls.Conn to use the new handshake method, but in (*net/http.Server).serve, it is clear that moving to the new function would enhance the request lifetime control in the function.

The context.Context provided to HandshakeContext would only be used for cancellation of the handshake itself, and once the handshake has completed, cancelling the context will have no effect. This is in line with the predecent set by (*net.Dialer).DialContext.

Motivation

In recent Go releases, we've been able to use the handy GetCertificate and GetClientCertificate methods of the *tls.Config to dynamically control certificate management in Go apps. This is fantastic, and has lead to things like https://godoc.org/golang.org/x/crypto/acme/autocert and https://github.com/johanbrandhorst/certify which are somewhat unique to the Go ecosystem.

Unfortunately, one glaring omission from the API is a connection context for cancellation and request scoped variable propagation. This means users have to implement custom timeouts or block their TLS connections forever in case of problems. It also means powerful observability tools like tracing and metrics that make use of the context cannot be used.

Interaction with net/http

net/http.Server provide BaseContext, which is used to set a global context for the duration of (*http.Server).Serve, and ConnContext, which is used on every new connection. The context passed to (*tls.Conn).HandshakeContext would necessarily be a child of these contexts, as the existing Handshake call is made after these contexts are created. See

The text was updated successfully, but these errors were encountered:

FiloSottile · 2019-06-04T09:57:27Z

It would help if you could elaborate on the various use cases: what you are trying to do in each situation, what doesn't work at the moment, and how a context would help.

I've in the past wanted to surface details of the ClientHelloInfo to net/http Handlers, so I can see the use case, but I'd like to build a generic solution.

johanbrandhorst · 2019-06-04T10:09:24Z

My use case specifically is to allow my library (certify) to cancel its outgoing requests if the incoming connection is closed. Additionally, it would allow detailed tracing to capture the latency cost of dynamically provisioned TLS certificates, something that is currently hidden inside the TLS handshake time in the standard library. Simply having a context associated with the underlying connection that could be used in outgoing net/http requests would be enough.

bradfitz · 2019-06-04T14:59:23Z

@johanbrandhorst, sounds like good reasons. For the same reason we didn't add a Context struct field to net/http.Request and used a method instead, we should instead add a Context method to crypto/tls.ClientHelloInfo.

johanbrandhorst · 2019-06-04T15:06:02Z

OK, I will attempt to implement this.

johanbrandhorst · 2019-06-04T15:30:11Z

Do the tags need updating?

gopherbot · 2019-06-06T21:16:55Z

Change https://golang.org/cl/181097 mentions this issue: crypto/tls, net/http: add context to tls structs

johanbrandhorst · 2019-09-04T07:25:18Z

Since the tree has recently opened again, bumping this for another look at the initial implementation. Could we tag this with 1.14? I think this proposal was accepted in #32406 (comment).

mvdan · 2019-09-17T09:51:24Z

@bradfitz @FiloSottile could you please clarify whether this proposal is accepted? If so, we can milestone it for 1.14 and review the CL.

bradfitz · 2019-09-17T15:07:20Z

Looks like it's stuck in the Crypto Proposal Review queue. @FiloSottile owns that meeting.

johanbrandhorst · 2019-09-30T14:28:11Z

Friendly ping on this. @FiloSottile is there an update on this?

johanbrandhorst · 2019-10-14T08:03:36Z

Friendly ping.

johanbrandhorst · 2019-10-28T09:04:04Z

Friendly ping. @FiloSottile anything I can do to help with the Crypto Proposal Review queue?

johanbrandhorst · 2019-11-11T17:35:30Z

Friendliest of bumps.

johanbrandhorst · 2020-04-15T15:46:58Z

With 1.14 out, is there a plan to review the crypto proposals?

rsc · 2020-05-20T18:00:20Z

@johanbrandhorst, crypto proposal review is proceeding fairly well (see all the crypto proposals in the minutes at https://golang.org/s/proposal-minutes), but there are many.

rsc · 2020-06-03T17:55:53Z

Ping @FiloSottile and @katiehockman.

FiloSottile · 2020-06-03T18:49:44Z

Hey, sorry for the delay, this one fell off my radar.

Another common problem that would be nice to solve at the same time is propagating info from the callbacks to net/http handlers. Maybe exposing this context also from ConnectionState would work, as it's exposed as Request.TLS? Or should the net/http Request context be a child of the TLS one? (How would that work with Server.BaseContext and Server.ConnContext?)

Another reason to expose it on ConnectionState is that the new VerifyConnection callback gets a ConnectionState as input. All other callbacks should be covered by ClientHelloInfo and CertificateRequestInfo.

Can we get a full API proposal here on the issue, along with details of how it would interact with net/http?

johanbrandhorst · 2020-06-04T16:03:58Z

Thanks for looping back on this @FiloSottile, I've updated the first post with a full API proposal. It does not support passing data back via the context as it is read only. We would need to make the context an exported field to support this, which was argued against in #32406 (comment). I'm open to discuss other solutions, or changing the context to be an exported field.

What are your thoughts?

FiloSottile · 2020-06-04T16:22:42Z

If the TLS context needs to be a child of the ConnContext, how does that happen? I can't see a way for crypto/tls to reach the ConnContext, nor for net/http to set the TLS context.

I unfortunately haven't designed a lot of APIs with Context, so I could use some advice (maybe from @bcmills?) on this proposal.

johanbrandhorst · 2020-06-04T16:24:49Z

I'm not an expert on the net/http server by any stretch, but I thought we could assign to it here:

go/src/net/http/server.go

Line 1782 in b371f18

if tlsConn, ok := c.rwc.(*tls.Conn); ok {

, where we have both the ctx inherited from ConnContext and the *tls.Conn.

gopherbot · 2020-11-12T20:39:53Z

Change https://golang.org/cl/269697 mentions this issue: crypto/tls: revert "crypto/tls: add HandshakeContext method to Conn"

FiloSottile · 2020-11-12T23:42:03Z

No change in consensus, so accepted. But we should probably wait for Go 1.17.

@rsc The CL went though plenty of review cycles and landed in time for the freeze, do you mean we should revert it?

johanbrandhorst · 2020-11-22T12:36:46Z

Friendliest of bumps, this is in an inbetween state between done and reverted at this time.

johanbrandhorst · 2020-12-09T16:42:11Z

Friendly bump, this is still in an in between state (http2 changes were never merged). @rsc we need a decision here.

rsc · 2020-12-15T20:05:15Z

Please revert. And in the future, please don't start landing changes before the proposal is accepted. Thanks!

FiloSottile · 2020-12-15T21:42:28Z

And in the future, please don't start landing changes before the proposal is accepted.

That was my fault, sorry @johanbrandhorst!

johanbrandhorst · 2020-12-16T10:52:06Z

This will be reverted in https://go-review.googlesource.com/c/go/+/269697

mvdan · 2020-12-16T12:51:44Z

before the proposal is accepted

As someone just following the thread, I'm confused; I thought it was already accepted?

If what is meant is that the changes were landed for 1.16 when they should have waited for 1.17, that makes more sense.

rsc · 2020-12-16T17:04:19Z

There was definitely some confusion about what was and was not subject to the proposal approval, but the current state is that only half the changes are done right now, very late in the freeze, so we should back them out and get everything in for Go 1.17.

This reverts CL 246338. Reason for revert: waiting for 1.17 release cycle Updates #32406 Change-Id: I074379039041e086c62271d689b4b7f442281663 Reviewed-on: https://go-review.googlesource.com/c/go/+/269697 Run-TryBot: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com> Run-TryBot: Katie Hockman <katie@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Katie Hockman <katie@golang.org> Trust: Katie Hockman <katie@golang.org> Trust: Roland Shoemaker <roland@golang.org>

gopherbot · 2020-12-17T20:26:05Z

Change https://golang.org/cl/278992 mentions this issue: api/go1.16: remove crypto/tls APIs that are moved to Go 1.17

CL 269697 was created before CL 276454 and submitted after, so the api/go1.16.txt file needs to be updated accordingly to fix the build. Updates #32406. Change-Id: I6bf79cc981be504e0baefa82982814aaee4434dc Reviewed-on: https://go-review.googlesource.com/c/go/+/278992 Trust: Dmitri Shuralyov <dmitshur@golang.org> Trust: Katie Hockman <katie@golang.org> Reviewed-by: Katie Hockman <katie@golang.org>

gopherbot · 2021-02-23T10:44:20Z

Change https://golang.org/cl/295370 mentions this issue: crypto/tls: add HandshakeContext method to Conn

gopherbot · 2021-02-23T10:52:14Z

Change https://golang.org/cl/295173 mentions this issue: http2: use (*tls.Conn).HandshakeContext in dialTLS

FiloSottile · 2021-03-16T14:27:19Z

Reopening to track http2 landing.

This lets us propagate the request context into the TLS handshake. Related to CL 295370 Updates golang/go#32406 Change-Id: Ie10c301be19b57b4b3e46ac31bbe87679e1eebc7 Reviewed-on: https://go-review.googlesource.com/c/net/+/295173 Trust: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com> Run-TryBot: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org>

johanbrandhorst · 2021-05-04T14:43:11Z

I think we can close this now 🎉. Thanks everyone involved for helping me get this in, it took longer than we hoped but here we are! Special thanks to @FiloSottile for all the reviews and comments. Also @katiehockman @bradfitz @bcmills for helping at various stages. I'm really excited for 1.17!

dmitshur · 2021-05-21T18:25:08Z

Reopening to track http2 landing.

If this refers to pulling in CL 295173 into net/http, that has happened in CL 315831. (Please reopen if I missed something.)

Thank you @johanbrandhorst and everyone else for your patience on working through this, and congratulations on getting this in!

agnivade changed the title ~~crypto/tls: add request context to ClientHelloInfo and CertificateRequestInfo~~ proposal: crypto/tls: add request context to ClientHelloInfo and CertificateRequestInfo Jun 3, 2019

gopherbot added this to the Proposal milestone Jun 3, 2019

gopherbot added the Proposal label Jun 3, 2019

agnivade added Proposal-Crypto Proposal related to crypto packages or other security issues Proposal and removed Proposal labels Jun 3, 2019

ianlancetaylor added this to Incoming in Proposals (old) Apr 15, 2020

rsc moved this from Incoming to Active in Proposals (old) Jun 3, 2020

rsc mentioned this issue Jun 3, 2020

proposal: review meeting minutes #33502

Open

johanbrandhorst changed the title ~~proposal: crypto/tls: add request context to ClientHelloInfo and CertificateRequestInfo~~ proposal: crypto/tls: add request context to ClientHelloInfo, CertificateRequestInfo and ConnectionState Jun 4, 2020

dmitshur modified the milestones: Backlog, Go1.17 Dec 28, 2020

gopherbot closed this as completed in 8607043 Mar 16, 2021

FiloSottile reopened this Mar 16, 2021

FiloSottile removed the Proposal-FinalCommentPeriod label Mar 17, 2021

katiehockman mentioned this issue Mar 18, 2021

crypto/tls: add TestClientHandshakeContextCancellation back after fixing flake #45106

Closed

dmitshur closed this as completed May 21, 2021

rsc mentioned this issue Jun 10, 2021

api: audit for Go 1.17 #46688

Closed

83 tasks

Code-Hex mentioned this issue Oct 18, 2021

fixed to use context.Context in TLS handshake gorilla/websocket#730

Closed

rhysh mentioned this issue Feb 2, 2022

net/http: Transport wastes TLS handshake progress #50984

Open

golang locked and limited conversation to collaborators May 21, 2022

gopherbot added the FrozenDueToAge label May 21, 2022

crypto/tls: add (*tls.Conn).HandshakeContext and add context to ClientHelloInfo and CertificateRequestInfo #32406

crypto/tls: add (*tls.Conn).HandshakeContext and add context to ClientHelloInfo and CertificateRequestInfo #32406

Comments

johanbrandhorst commented Jun 3, 2019 • edited

Proposal

Motivation

Interaction with net/http

FiloSottile commented Jun 4, 2019

johanbrandhorst commented Jun 4, 2019

bradfitz commented Jun 4, 2019

johanbrandhorst commented Jun 4, 2019

johanbrandhorst commented Jun 4, 2019

gopherbot commented Jun 6, 2019

johanbrandhorst commented Sep 4, 2019

mvdan commented Sep 17, 2019

bradfitz commented Sep 17, 2019

johanbrandhorst commented Sep 30, 2019

johanbrandhorst commented Oct 14, 2019

johanbrandhorst commented Oct 28, 2019

johanbrandhorst commented Nov 11, 2019

johanbrandhorst commented Apr 15, 2020

rsc commented May 20, 2020 • edited

rsc commented Jun 3, 2020

FiloSottile commented Jun 3, 2020

johanbrandhorst commented Jun 4, 2020

FiloSottile commented Jun 4, 2020

johanbrandhorst commented Jun 4, 2020

gopherbot commented Nov 12, 2020

FiloSottile commented Nov 12, 2020

johanbrandhorst commented Nov 22, 2020

johanbrandhorst commented Dec 9, 2020

rsc commented Dec 15, 2020

FiloSottile commented Dec 15, 2020

johanbrandhorst commented Dec 16, 2020

mvdan commented Dec 16, 2020

rsc commented Dec 16, 2020

gopherbot commented Dec 17, 2020

gopherbot commented Feb 23, 2021

gopherbot commented Feb 23, 2021

FiloSottile commented Mar 16, 2021

johanbrandhorst commented May 4, 2021

dmitshur commented May 21, 2021

johanbrandhorst commented Jun 3, 2019 •

edited

rsc commented May 20, 2020 •

edited