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: add (*tls.Conn).HandshakeContext and add context to ClientHelloInfo and CertificateRequestInfo #32406

Open
johanbrandhorst opened this issue Jun 3, 2019 · 71 comments

Comments

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Jun 3, 2019

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

@agnivade 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 gopherbot added this to the Proposal milestone Jun 3, 2019
@gopherbot gopherbot added the Proposal label Jun 3, 2019
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 4, 2019

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
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Jun 4, 2019

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
Copy link
Contributor

@bradfitz bradfitz commented Jun 4, 2019

@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
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Jun 4, 2019

OK, I will attempt to implement this.

@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Jun 4, 2019

Do the tags need updating?

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 6, 2019

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

@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Sep 4, 2019

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
Copy link
Member

@mvdan mvdan commented Sep 17, 2019

@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
Copy link
Contributor

@bradfitz bradfitz commented Sep 17, 2019

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

@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Sep 30, 2019

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

@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Oct 14, 2019

Friendly ping.

@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Oct 28, 2019

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

@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Nov 11, 2019

Friendliest of bumps.

@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Apr 15, 2020

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

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Apr 15, 2020
@rsc
Copy link
Contributor

@rsc rsc commented May 20, 2020

@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
Copy link
Contributor

@rsc rsc commented Jun 3, 2020

@rsc rsc moved this from Incoming to Active in Proposals Jun 3, 2020
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 3, 2020

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 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
@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Jun 4, 2020

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
Copy link
Member

@FiloSottile FiloSottile commented Jun 4, 2020

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
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Jun 4, 2020

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

if tlsConn, ok := c.rwc.(*tls.Conn); ok {
, where we have both the ctx inherited from ConnContext and the *tls.Conn.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 4, 2020

Based on the discussion above, then, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Nov 4, 2020
@gopherbot gopherbot closed this in fdecb5c Nov 9, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 10, 2020

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

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Nov 10, 2020

Ah I thought this was already approved. Reopening to finish the process. We can revert if it's not approved.

@FiloSottile FiloSottile reopened this Nov 10, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 11, 2020

Change https://golang.org/cl/269217 mentions this issue: crypto/tls: test HandshakeContext context hiearchy

@rsc
Copy link
Contributor

@rsc rsc commented Nov 11, 2020

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

@rsc rsc moved this from Likely Accept to Accepted in Proposals Nov 11, 2020
@rsc rsc changed the title proposal: crypto/tls: add (*tls.Conn).HandshakeContext and add context to ClientHelloInfo and CertificateRequestInfo crypto/tls: add (*tls.Conn).HandshakeContext and add context to ClientHelloInfo and CertificateRequestInfo Nov 11, 2020
@rsc rsc modified the milestones: Proposal, Backlog Nov 11, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 12, 2020

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

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Nov 12, 2020

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
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Nov 22, 2020

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

@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Dec 9, 2020

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

@rsc
Copy link
Contributor

@rsc rsc commented Dec 15, 2020

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

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Dec 15, 2020

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

That was my fault, sorry @johanbrandhorst!

@johanbrandhorst
Copy link
Member Author

@johanbrandhorst johanbrandhorst commented Dec 16, 2020

@mvdan
Copy link
Member

@mvdan mvdan commented Dec 16, 2020

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
Copy link
Contributor

@rsc rsc commented Dec 16, 2020

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 pushed a commit that referenced this issue Dec 17, 2020
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
Copy link

@gopherbot gopherbot commented Dec 17, 2020

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

gopherbot pushed a commit that referenced this issue Dec 17, 2020
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>
@dmitshur dmitshur modified the milestones: Backlog, Go1.17 Dec 28, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 23, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 23, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants