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: GetClientCertificate is not preserved by Clone #19264

Closed
bhiggins opened this issue Feb 24, 2017 · 14 comments
Closed

crypto/tls: GetClientCertificate is not preserved by Clone #19264

bhiggins opened this issue Feb 24, 2017 · 14 comments
Assignees
Milestone

Comments

@bhiggins
Copy link

@bhiggins bhiggins commented Feb 24, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.8 (this feature was introduced in 1.8)

What operating system and processor architecture are you using (go env)?

Linux (but it doesn't matter here)

What did you do?

Used tls.Dial (or tls.DialWithDialer) without setting ServerName in Config.

What did you expect to see?

A successful connection.

What did you see instead?

"remote error: tls: bad certificate" on the client and "tls: client didn't provide a certificate" on the server.

Workaround: set ServerName so that the config isn't cloned by tls.DialWithDialer.

(BTW, maybe it would be useful in unit tests to clone configs immediately before use.)

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 24, 2017

I thought I replied to this already, but I can't find it.

What makes you think this is not being cloned? The source code and this snippet sure suggests it is:

https://play.golang.org/p/Hk6eSLa4-l

Please share some minimal code you expect to work and what it does instead. Thanks.

@bhiggins

This comment has been minimized.

Copy link
Author

@bhiggins bhiggins commented Feb 24, 2017

@bradfitz, this is regarding GetClientCertificate (added in 1.8), not GetCertificate. I took a look at Clone and GetClientCertificate was not included.

Here is an example: https://play.golang.org/p/6zbpqxY6U1

I expect it to print "Works for me" but instead line 16 panics.

@bradfitz bradfitz added this to the Go1.9 milestone Feb 24, 2017
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 24, 2017

Hey @agl, if you'd have accepted an earlier version of my CL (patchset 3: https://go-review.googlesource.com/c/28075/3/src/crypto/tls/tls_test.go#471) with the robust regexp-based test, we wouldn't be here. :-)

But our existing test for Config.Clone lets things slip through the cracks, like this.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 1, 2017

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

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Mar 2, 2017

Ah, I had this in my TODO because it was encountered recently at Cloudflare.

Any chances this could be considered for 1.8.1?

@jefferai

This comment has been minimized.

Copy link

@jefferai jefferai commented Mar 2, 2017

And in that vein any idea when such a release might be? This sadly makes this (otherwise exciting) new feature fairly unusable in real code, especially for any code wanting to use the http2 package.

@gopherbot gopherbot closed this in 87649d3 Mar 2, 2017
@mikedanese

This comment has been minimized.

Copy link
Contributor

@mikedanese mikedanese commented Mar 8, 2017

The kubernetes team was also greatly anticipating this feature in 1.8. As far as patches go, this one seems fairly safe. Any chance we could get it into 1.8.1? Otherwise we will likely have to build kubernetes with a patched go toolchain.

cc @cjcullen @jcbsmpsn

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 8, 2017

Would've been nice if Kubernetes had helped test Go 1.8 sooner back when I was asking (kubernetes/kubernetes#38228) and maybe found this bug before Go 1.8. Oh, well. Better late than never, I guess.

@bradfitz bradfitz modified the milestones: Go1.8.1, Go1.9 Mar 8, 2017
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 8, 2017

Reopening for Go 1.8.1.

@bradfitz bradfitz reopened this Mar 8, 2017
@jefferai

This comment has been minimized.

Copy link

@jefferai jefferai commented Mar 8, 2017

Just as a note, while I also badly hope this will be in 1.8.1, it's pretty trivial to work around this with a locally-patched vendored version of the http2 library, if that's where you're running into this problem, in the file go1.8.go:

func cloneTLSConfig(c *tls.Config) *tls.Config {
	// MODIFIED from upstream to work around Go bug
	ret := c.Clone()
	ret.GetClientCertificate = c.GetClientCertificate
	return ret
}

Perhaps this could make its way into http2 for now?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 8, 2017

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 8, 2017

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

gopherbot pushed a commit to golang/net that referenced this issue Mar 8, 2017
Updates golang/go#19264

Change-Id: Ib5b483d2d830d7a51d59eb7bc5eac106da5d5476
Reviewed-on: https://go-review.googlesource.com/37944
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 8, 2017

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

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 8, 2017

Cherry-picked.

@bradfitz bradfitz closed this Mar 8, 2017
gopherbot pushed a commit that referenced this issue Mar 8, 2017
…etClientCertificate field

Using GetClientCertificate with the http client is currently completely
broken because inside the transport we clone the tls.Config and pass it
off to the tls.Client. Since tls.Config.Clone() does not pass forward
the GetClientCertificate field, GetClientCertificate is ignored in this
context.

Fixes #19264

Change-Id: Ie214f9f0039ac7c3a2dab8ffd14d30668bdb4c71
Signed-off-by: Mike Danese <mikedanese@google.com>
Reviewed-on: https://go-review.googlesource.com/37541
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 87649d3)
Reviewed-on: https://go-review.googlesource.com/37946
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
xoebus added a commit to pivotal-cf/scantron that referenced this issue Apr 4, 2017
This is on a branch because it requires an unreleased version of Go to
work due to a bug in crypto/tls[1]. It can be merged after that is
released.

[1]: golang/go#19264

[#120538799]

Signed-off-by: Adam Berkovec <aberkovec@pivotal.io>
xoebus added a commit to pivotal-cf/scantron that referenced this issue Apr 10, 2017
This feature requirest Go 1.8.1 due to a bug fix.

[1]: golang/go#19264

[#120538799]

Signed-off-by: Adam Berkovec <aberkovec@pivotal.io>
@golang golang locked and limited conversation to collaborators Mar 8, 2018
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Using GetClientCertificate with the http client is currently completely
broken because inside the transport we clone the tls.Config and pass it
off to the tls.Client. Since tls.Config.Clone() does not pass forward
the GetClientCertificate field, GetClientCertificate is ignored in this
context.

Fixes golang#19264

Change-Id: Ie214f9f0039ac7c3a2dab8ffd14d30668bdb4c71
Signed-off-by: Mike Danese <mikedanese@google.com>
Reviewed-on: https://go-review.googlesource.com/37541
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Using GetClientCertificate with the http client is currently completely
broken because inside the transport we clone the tls.Config and pass it
off to the tls.Client. Since tls.Config.Clone() does not pass forward
the GetClientCertificate field, GetClientCertificate is ignored in this
context.

Fixes golang#19264

Change-Id: Ie214f9f0039ac7c3a2dab8ffd14d30668bdb4c71
Signed-off-by: Mike Danese <mikedanese@google.com>
Reviewed-on: https://go-review.googlesource.com/37541
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
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.