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

crypto/tls: Clean up semantics of copying/immutability of tls.Config in 1.8. #16492

Closed
agl opened this issue Jul 25, 2016 · 6 comments
Closed

crypto/tls: Clean up semantics of copying/immutability of tls.Config in 1.8. #16492

agl opened this issue Jul 25, 2016 · 6 comments
Assignees
Milestone

Comments

@agl
Copy link
Contributor

@agl agl commented Jul 25, 2016

A function to copy a tls.Config should probably be exposed in 1.8 because net/http could use it. Also, the current DialWithDialer function copies the config in order to set an SNI value, but that has a complex interaction if the Config is being used for serving. Maybe that's an unreasonable use-case but, if so, what's reasonable could be better documented.

@agl agl self-assigned this Jul 25, 2016
@jboelter

This comment has been minimized.

Copy link

@jboelter jboelter commented Jul 25, 2016

also #15771

@quentinmit quentinmit changed the title Clean up semantics of copying/immutability of tls.Config in 1.8. crypto/tls: Clean up semantics of copying/immutability of tls.Config in 1.8. Jul 29, 2016
@quentinmit quentinmit added this to the Go1.8 milestone Jul 29, 2016
@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Aug 29, 2016

also #16228

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 30, 2016

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

gopherbot pushed a commit that referenced this issue Sep 1, 2016
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 #15771
Updates #16228 (needs update in x/net/http2 before fixed)
Updates #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>
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Sep 9, 2016

Also, the current DialWithDialer function copies the config in order to set an SNI value, but that has a complex interaction if the Config is being used for serving.

The net/http package also has its own variant of tls.Config.Clone which configs most but not all fields for the same or similar field race reason, because you can't clone a Config already in use for serving.

tls.Config.CloneClient? Kinda gross.

@quentinmit quentinmit added the NeedsFix label Oct 5, 2016
@lmb

This comment has been minimized.

Copy link
Contributor

@lmb lmb commented Oct 21, 2016

Maybe the right step is to make the exported Clone() skip the SessionTickets* variable? This would break session resumption on cloned Config, but that seems like less of a foot gun than having to discern what kind of copy we want.

Also, I think the title of this issue should be amended to mention that DialWithDialer is currently racy.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 22, 2016

I think https://golang.org/cl/31595 (which didn't mention this issue) was the fix and the last piece of this bug. Closing.

@bradfitz bradfitz closed this Oct 22, 2016
@golang golang locked and limited conversation to collaborators Oct 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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