/cc @agl

/cc @FiloSottile

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.

@agl @bradfitz Any thoughts on this?

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.

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?

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

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

ping @FiloSottile

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.

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.

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.

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.

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.

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

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?

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.

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.

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.

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.

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.

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.

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

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 👍)

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

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

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.

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

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

No change in consensus, so declined.