crypto/tls: GetClientCertificate is not preserved by Clone #19264
Comments
|
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. |
bradfitz
added
the
WaitingForInfo
|
@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. |
|
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. |
|
CL https://golang.org/cl/37541 mentions this issue. |
|
Ah, I had this in my TODO because it was encountered recently at Cloudflare. Any chances this could be considered for 1.8.1? |
|
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 |
|
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. |
|
Reopening for Go 1.8.1. |
|
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 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 |
|
CL https://golang.org/cl/37944 mentions this issue. |
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>
|
CL https://golang.org/cl/37946 mentions this issue. |
|
Cherry-picked. |
…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>
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>
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>
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>
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>
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.)
The text was updated successfully, but these errors were encountered: