net/http, crypto/tls: cloneTLSConfig is out of sync with tls.Config

[tombergan]

I added a new tls.Config field in Issue #14376
https://tip.golang.org/src/crypto/tls/common.go#L384

but didn't know I needed to update cloneTLSConfig
https://tip.golang.org/src/net/http/transport.go#L2014

Is there a reason to not add a Clone method to tls.Config? Cloning that struct from another package seems brittle. transport.go has a comment about a race between transport.go and tls.Server -- that race seems more simply resolved if tls.Server clones the config before mutating it internally. (There's also no comment on tls.Server saying that it will mutate the provided config, which makes the behavior surprising.)

/cc @bradfitz

[bradfitz]

We could add a Clone method to TLSConfig, but where do we draw the line at adding clone methods to all types? I don't know.

But let's fix up cloneTLSConfig for now. I thought it had a test for testing that all fields are cloned, but maybe I'm remembering a different clone test.

We can keep this bug open for thinking of a better solution (and tests) in Go 1.8.

[gopherbot]

CL https://golang.org/cl/23283 mentions this issue.

[bradfitz]

See also @ianlancetaylor's https://go-review.googlesource.com/24287 (which adds a private *tls.Config.clone method)

[bdarnell]

A tls.Config.Clone method is needed for more than the standard library. Several projects have their own copies of this method, which will need to be updated for Go 1.7 (and again for 1.8 if #16363 goes in).

GRPC recently stopped doing a naive copy of the tls.Config struct to avoid the incorrect copying of a lock, which resulted in bugs due to mutating caller-supplied memory.

[bradfitz]

@tombergan, @bdarnell, yeah, I'm sold at this point. We'll add one early in Go 1.8.

[bradfitz]

KeyLogWriter was added in golang.org/cl/27434 too.

[josharian]

Related: #16492. The implementation of Clone/Copy should be fairly trivial. Any volunteers to send a CL?

[bradfitz]

On it.

[gopherbot]

CL https://golang.org/cl/28075 mentions this issue.

[F21]

@bradfitz, with this change is it safe to clone an existing tls.Config, modify the config and then replace the old config for http servers and clients?

I am using Vault to manage my certificates internally and plan to rotate the certificates every 12 hours and would like to do the following:

Rotate the root certificates in the RootCAs and ClientCAs fields by cloning the tls.Config, setting a new x509.CertPool to those fields, and replacing the existing tls config of an http server with the new one. This is so that I can rotate the Root CAs without restarting the http server. Is this safe?

Would it also be safe to do the same for the http client (clone tls.Config and replace the existing tls config being used in a transport by a http client)?