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

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

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

Comments

Projects
None yet
6 participants
@tombergan
Copy link
Contributor

commented May 20, 2016

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

This comment has been minimized.

Copy link
Member

commented May 20, 2016

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

This comment has been minimized.

Copy link

commented May 20, 2016

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

gopherbot pushed a commit that referenced this issue May 20, 2016

net/http: also clone DynamicRecordSizingDisabled in cloneTLSConfig
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>
@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 27, 2016

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

@bdarnell

This comment has been minimized.

Copy link

commented Jul 24, 2016

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

This comment has been minimized.

Copy link
Member

commented Jul 24, 2016

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 27, 2016

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

@josharian

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2016

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

On it.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 30, 2016

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

@gopherbot gopherbot closed this in d24f446 Sep 1, 2016

@F21

This comment has been minimized.

Copy link

commented Nov 25, 2016

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

@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

crypto/tls: add Config.Clone
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

crypto/tls: add Config.Clone
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.
You can’t perform that action at this time.