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

Jump to bottom

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

Closed
tombergan opened this issue May 20, 2016 · 10 comments
Closed

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

tombergan opened this issue May 20, 2016 · 10 comments
Labels
FrozenDueToAge
Milestone
Go1.8Early

Comments

@tombergan
Copy link
Contributor

tombergan commented May 20, 2016 •
edited

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

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.

@bradfitz bradfitz added this to the Go1.8 milestone May 20, 2016
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue May 20, 2016
@bradfitz
net/http: also clone DynamicRecordSizingDisabled in cloneTLSConfig
85e39f8 
Updates #15771

Change-Id: I5dad96bdca19d680dd00cbd17b72a03e43eb557e
Reviewed-on: https://go-review.googlesource.com/23283
Reviewed-by: Tom Bergan <tombergan@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@tombergan tombergan mentioned this issue Jun 2, 2016
Closed
@lmb lmb mentioned this issue Jun 21, 2016
Closed
@bradfitz
Copy link
Contributor

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

@bdarnell
Copy link

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 bradfitz modified the milestones: Go1.8Early, Go1.8 Jul 24, 2016
@bradfitz
Copy link
Contributor

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

@bdarnell bdarnell mentioned this issue Jul 24, 2016
Closed
@jboelter jboelter mentioned this issue Jul 25, 2016
Closed
@tamird tamird mentioned this issue Jul 26, 2016
Closed
@jboelter jboelter mentioned this issue Jul 27, 2016
Closed
@soltysh soltysh mentioned this issue Aug 5, 2016
Merged
@bradfitz
Copy link
Contributor

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

@josharian
Copy link
Contributor

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

@bradfitz
Copy link
Contributor

On it.

@gopherbot
Copy link

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

@F21
Copy link

F21 commented Nov 25, 2016 •
edited

@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)?

@shogo82148 shogo82148 mentioned this issue Nov 10, 2017
Merged
@golang golang locked and limited conversation to collaborators Nov 25, 2017
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@bradfitz
crypto/tls: add Config.Clone
a03d18b 
In Go 1.0, the Config struct consisted only of exported fields.

In Go 1.1, it started to grow private, uncopyable fields (sync.Once,
sync.Mutex, etc).

Ever since, people have been writing their own private Config.Clone
methods, or risking it and doing a language-level shallow copy and
copying the unexported sync variables.

Clean this up and export the Config.clone method as Config.Clone.
This matches the convention of Template.Clone from text/template and
html/template at least.

Fixes golang#15771
Updates golang#16228 (needs update in x/net/http2 before fixed)
Updates golang#16492 (not sure whether @agl wants to do more)

Change-Id: I48c2825d4fef55a75d2f99640a7079c56fce39ca
Reviewed-on: https://go-review.googlesource.com/28075
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@bradfitz
crypto/tls: add Config.Clone
1822025 
In Go 1.0, the Config struct consisted only of exported fields.

In Go 1.1, it started to grow private, uncopyable fields (sync.Once,
sync.Mutex, etc).

Ever since, people have been writing their own private Config.Clone
methods, or risking it and doing a language-level shallow copy and
copying the unexported sync variables.

Clean this up and export the Config.clone method as Config.Clone.
This matches the convention of Template.Clone from text/template and
html/template at least.

Fixes golang#15771
Updates golang#16228 (needs update in x/net/http2 before fixed)
Updates golang#16492 (not sure whether @agl wants to do more)

Change-Id: I48c2825d4fef55a75d2f99640a7079c56fce39ca
Reviewed-on: https://go-review.googlesource.com/28075
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8Early
Development

No branches or pull requests
6 participants
@bradfitz @josharian @bdarnell @F21 @gopherbot @tombergan