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

proposal: crypto/tls: add GetConfigForServer callback to *tls.Config #22836

Closed
nhooyr opened this issue Nov 21, 2017 · 30 comments
Closed

proposal: crypto/tls: add GetConfigForServer callback to *tls.Config #22836

nhooyr opened this issue Nov 21, 2017 · 30 comments

Comments

@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Nov 21, 2017

I'd like to be able to dynamically update a *tls.Config's RootCAs field to support changes in root CAs without having to restart the server.

If I want to update the ClientCAs field, I can easily do this by using the GetConfigForClient hook. Whereas for RootCAs, there is no such callback. Reason being that the ClientCAs field is used by servers and that is the only time GetConfigForClient is called. The RootCAs field is used only by clients.

My only option would be to create a *tls.Config structure that is protected by a mutex. For dialing, I would lock the mutex, grab the pointer and then unlock and use the Config normally. When the Config needs to be updated, I would clone the *tls.Config, update the cloned Config, lock the mutex, replace the pointer and then unlock.

I could easily abstract the above away into a function so this isn't a big issue but my code would be significantly more clean and symmetric across server and client if there was a callback to grab the Config for the connection dynamically as a client.

Of course, my use case is distinct from the use cases that gave rise to the callbacks, where the server/client wants to respond to the server/client dynamically. I just want to update the Config dynamically regardless of who the server/client is. So I suspect my use case may be a misuse of even the GetConfigForClient callback but it is in my opinion still the cleanest solution and makes it really easy to play with third party frameworks that do not expect the *tls.Config to be dynamically updated like gRPC.

@bradfitz suggested this in #16066 (comment) but it was never implemented.

@titanous titanous changed the title crypto/tls: add GetConfigForServer callback to *tls.Config proposal: crypto/tls: add GetConfigForServer callback to *tls.Config Dec 6, 2017
@gopherbot gopherbot added this to the Proposal milestone Dec 6, 2017
@gopherbot gopherbot added the Proposal label Dec 6, 2017
@titanous

This comment has been minimized.

Copy link
Member

@titanous titanous commented Dec 6, 2017

/cc @agl

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 5, 2018

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Feb 5, 2018

GetConfigForClient turned out to be a simple and extremely powerful addition, enabling a number of advanced use cases without needing to expose a knob for each one. The latest example being #23679. (The only annoyance is how it plays with H/2, where you have to remember to configure it on the outer Config.)

So given how much of a success that API was, I'd be in favor of making the symmetric one on the client side.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 19, 2018

@agl @bradfitz Any thoughts on this?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Apr 2, 2018

I'm fine with it, but I'll defer to @agl and @FiloSottile for decision. They'd be the ones primarily impacted by any maintenance cost.

@zachwalton

This comment has been minimized.

Copy link

@zachwalton zachwalton commented Jun 27, 2019

Were there ever further discussions about this proposal? GetConfigForClient is a super-elegant way to solve this on the server side, but on the client side our solution is... much less elegant.

The implementation looks somewhat straightforward so I suppose this is just a judgment call?

@krasi-georgiev

This comment has been minimized.

Copy link

@krasi-georgiev krasi-georgiev commented Oct 31, 2019

so IIUC there is no way to rotate the RootCAs at the moment ?

@krasi-georgiev

This comment has been minimized.

Copy link

@krasi-georgiev krasi-georgiev commented Dec 3, 2019

@zachwalton I see that you implemented this did you happen to open a PR that we can link here?

@krasi-georgiev

This comment has been minimized.

Copy link

@krasi-georgiev krasi-georgiev commented Dec 4, 2019

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jan 21, 2020

What would be the argument to the GetConfigForServer callback? When the connection opens the only thing available is the net.Conn with its RemoteAddr and LocalAddr, not even the server name.

There isn't really a lot of dynamic decisions that can be made based on that, so it would be useful only as a way to mutate a Config over time, right? That feels much less useful than GetConfigForClient, and as you say can be implemented with a mutex and a Dial wrapper.

Also, clients usually involve calling Dial for every connection (as opposed to using ListenAndServe helpers), so it's much easier to implement GetConfigForServer outside of crypto/tls than GetConfigForClient.

On second thought I don't think this is useful enough to add another entry to Config.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

@nhooyr nhooyr commented Jan 21, 2020

What would be the argument to the GetConfigForServer callback? When the connection opens the only thing available is the net.Conn with its RemoteAddr and LocalAddr, not even the server name.

Symmetric API is the strongest argument for me, it's something that feels like it should be there since its there on the server side. And like I mentioned above, it makes it really easy to play with third party frameworks that do not expect *tls.Config to be dynamically updated like gRPC.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jan 21, 2020

What would be the argument to the GetConfigForServer callback? When the connection opens the only thing available is the net.Conn with its RemoteAddr and LocalAddr, not even the server name.

Symmetric API is the strongest argument for me, it's something that feels like it should be there since its there on the server side. And like I mentioned above, it makes it really easy to play with third party frameworks that do not expect *tls.Config to be dynamically updated like gRPC.

I meant literally what the function arguments would be. I agree that being symmetric is nice, but since this would have to run before the ClientHello is sent, it's not really symmetric in that it doesn't actually provide any details of the server.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

@nhooyr nhooyr commented Jan 21, 2020

I meant literally what the function arguments would be.

That's a very fair point. It's a little weird I agree from that aspect, it'd take no arguments.

I agree that being symmetric is nice, but since this would have to run before the ClientHello is sent, it's not really symmetric in that it doesn't actually provide any details of the server.

While it's true the *tls.Config cannot be adjusted per server, there's still value in dynamically adjusting the *tls.Config as time progresses without a mutex. Like I mentioned above, for things like gRPC, it'd be much cleaner to just use such a callback versus performing your own tls dial and then passing a net.Conn in. Likewise with net/http's client.

@krasi-georgiev

This comment has been minimized.

Copy link

@krasi-georgiev krasi-georgiev commented Jan 22, 2020

My only option would be to create a *tls.Config structure that is protected by a mutex. For dialing, I would lock the mutex, grab the pointer and then unlock and use the Config normally. When the Config needs to be updated, I would clone the *tls.Config, update the cloned Config, lock the mutex, replace the pointer and then unlock.
I could easily abstract the above away into a function so this isn't a big issue but my code would be significantly more clean and symmetric across server and client if there was a callback to grab the Config for the connection dynamically as a client.

Did you already implemented this in your code? I would be intereste to see an example if one exists.

@rsc rsc added this to Incoming in Proposals Jan 28, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 5, 2020

Adding to proposal minutes for visibility but this seems headed toward likely decline.

@rsc rsc moved this from Incoming to Active in Proposals Feb 5, 2020
@zachwalton

This comment has been minimized.

Copy link

@zachwalton zachwalton commented Feb 5, 2020

My use case for this is live client-side certificate rotation. Our system rotates certificates on disk every 10 days. We use the server-side equivalent of this callback to rotate the certificate when its mtime changes.

On the client side to accomplish the same thing, we have to pass custom transports to every HTTP client and had to implement a handshake interface for our GRPC clients. This can become problematic for HTTP or GRPC clients that aren’t ours that only support passing tls.Config.

We can do cert rotation before ClientHello.

Edit: More generally this is just an ask (at least from me) for a client certificate factory that’s invoked on every connection. I think the “ForServer” bit has reasonably led to the discussion above, so maybe that’s just a misnomer?

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

@nhooyr nhooyr commented Feb 5, 2020

We ran into this issue again at @cdr where we wanted to make *http.Transport rotate certificates like @zachwalton mentioned but then we have to replace the *http.Transport and lose the pool of existing connections.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Feb 12, 2020

For certificate rotation GetClientCertificate should work, or am I misunderstanding?

The only use case otherwise seems to be rotating RootCAs, which I think is narrow enough to be worth solving with a custom VerifyPeerCertificate.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

@nhooyr nhooyr commented Feb 12, 2020

For certificate rotation GetClientCertificate should work, or am I misunderstanding?

Sorry, I meant root CAs.

The only use case otherwise seems to be rotating RootCAs, which I think is narrow enough to be worth solving with a custom VerifyPeerCertificate.

Fair enough, it'd just be unfortunate if in the future there was another use case where dynamically adjusting another field on the *tls.Config made sense as then we'd have to add another mechanism.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Feb 12, 2020

The only use case otherwise seems to be rotating RootCAs, which I think is narrow enough to be worth solving with a custom VerifyPeerCertificate.

Fair enough, it'd just be unfortunate if in the future there was another use case where dynamically adjusting another field on the *tls.Config made sense as then we'd have to add another mechanism.

I'd be happy to discuss again in a new proposal if another use case comes up, as it would be easier to evaluate the design then.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

@nhooyr nhooyr commented Feb 12, 2020

Oh I misinterpreted what you were suggesting, I didn't realize there is already a VerifyPeerCertificate hook. I agree, I think we could close this issue by adding an example to the docs on how to implement it correctly.

@zachwalton

This comment has been minimized.

Copy link

@zachwalton zachwalton commented Feb 12, 2020

Does VerifyPeerCertificate work for rotating root CAs before verifying the server certificate?

    // VerifyPeerCertificate, if not nil, is called after normal
    // certificate verification by either a TLS client or server.

Seems like you'd need to either rotate root CAs after verifying (probably not the desired behavior), or set InsecureSkipVerify and do your own certificate verification in VerifyPeerCertificate.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

@nhooyr nhooyr commented Feb 12, 2020

@zachwalton I believe setting InsecureSkipVerify and then doing your own is what @FiloSottile is suggesting. I believe you'd parse each certificate with https://golang.org/pkg/crypto/x509/#ParseCertificate and then create verify options with the appropriate roots and parsed intermediates. Then you'd call https://golang.org/pkg/crypto/x509/#Certificate.Verify on the leaf certificate with those options.

We could make it easier by adding a helper in x509 or TLS.

@zachwalton

This comment has been minimized.

Copy link

@zachwalton zachwalton commented Feb 12, 2020

I think that's ok for my purpose (we're actually using VerifyPeerCertificate already). Just pointing out that in order to dynamically rotate client root CAs before verifying a server cert, the user has to (correctly) implement their own certificate verification, vs. the easier + safer server-side option.

It just seems a bit tricky.

(that said, the suggestions here will work for our purposes and I'm going to go implement that, thanks for the great advice 👍)

@krasi-georgiev

This comment has been minimized.

Copy link

@krasi-georgiev krasi-georgiev commented Feb 12, 2020

I am also interested in seing an example how to rotate the server CAs by using VerifyPeerCertificate

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Feb 12, 2020

This comes up fairly often, so we have an example for it in Go 1.14.

https://tip.golang.org/pkg/crypto/tls/#example_Config_verifyPeerCertificate

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

@nhooyr nhooyr commented Feb 12, 2020

How about we add a method Verify(certs [][]byte, opts *VerifyOptions) (chains [][]byte, error) to *x509.CertPool to verify certificates with the given options and the pool as Roots? Any roots already set in the options would be ignored.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 26, 2020

Based on the discussion above, this is a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Feb 26, 2020
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Feb 26, 2020

#36736 would also make it even easier to write a custom verification function.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Mar 4, 2020

No change in consensus, so declined.

@rsc rsc closed this Mar 4, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.