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

[johanbrandhorst]

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

• BaseContext use
• ConnContext use
• *tls.Conn first tested

[FiloSottile]

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]

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]

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

OK, I will attempt to implement this.

[johanbrandhorst]

Do the tags need updating?

[gopherbot]

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

[johanbrandhorst]

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]

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

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

[johanbrandhorst]

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

[johanbrandhorst]

Friendly ping.

[johanbrandhorst]

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

[johanbrandhorst]

Friendliest of bumps.

[johanbrandhorst]

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

[rsc]

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

Ping @FiloSottile and @katiehockman.

[FiloSottile]

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]

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]

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]

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.

[rsc]

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

[FiloSottile]

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

That was my fault, sorry @johanbrandhorst!

[johanbrandhorst]

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

[mvdan]

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]

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.

[gopherbot]

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

[gopherbot]

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

[gopherbot]

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

[FiloSottile]

Reopening to track http2 landing.

[johanbrandhorst]

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]

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!