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: wrong private key used for SNI connections #3367

Closed
benburkert opened this issue Mar 21, 2012 · 9 comments
Closed

crypto/tls: wrong private key used for SNI connections #3367

benburkert opened this issue Mar 21, 2012 · 9 comments

Comments

@benburkert
Copy link
Contributor

@benburkert benburkert commented Mar 21, 2012

A client can use the server name indication TLS extension to specify the desired
hostname of the server certificate used in the TLS handshake. Go supports this SNI
extension in server connections with the tls.Config struct's NameToCertificate member
and BuildNameToCertificate function. These are used in the server connection's handshake
implementation to lookup a certificate which may not be the default certificate used by
the server. In such case, the key exchange functionality will still use the default
certificate's private key.

The linked code sample demonstrates this problem when the client recieve a "remote
error: bad record MAC" error during handshake. The included patch fixes this error
by keeping track of the non-default certificate's private key in the key exchange struct.

What steps will reproduce the problem?

https://gist.github.com/2151037

go run sni_test.go

What is the expected output?

the program should exit 0

What do you see instead?

a "remote error: bad record MAC" panic

Which compiler are you using (5g, 6g, 8g, gccgo)?

6g

Which operating system are you using?

OSX

Which revision are you using?  (hg identify)

15a98eba66e0

Please provide any additional information below.
@benburkert

This comment has been minimized.

Copy link
Contributor Author

@benburkert benburkert commented Mar 21, 2012

Comment 1:

It appears that the same problem (always using config.Certificates[0] as the
certificate) affects the OCSP stapling functionality, but i have not verified this. I'm
working on patch with with a fix & test for this as well.
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Mar 22, 2012

Comment 3:

Yea, we do indeed use the wrong private key. Nobody has tried using SNI support before
because not enough client support it, but we should fix it none the less.
But I'll wait until after the initial Go 1 release.

Labels changed: added priority-later, packagebug, removed priority-triage.

Owner changed to @agl.

Status changed to Accepted.

@benburkert

This comment has been minimized.

Copy link
Contributor Author

@benburkert benburkert commented Mar 22, 2012

Comment 4:

in that case, would you consider removing and/or deprecating the server implementation
of SNI before 1.0? The existing API is inelegant and diverges from the pattern
implemented in OpenSSL and followed by other languages.
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Mar 22, 2012

Comment 5:

What would you like the API to be? If the existing API worked as expected I believe that
it would cover most use cases.
@benburkert

This comment has been minimized.

Copy link
Contributor Author

@benburkert benburkert commented Mar 22, 2012

Comment 6:

I would like to see a callback based API that can change the tls.Config used per client.
Something similar to this: https://gist.github.com/2161321
I believe this could be further simplified by removing tls.Config.NameToCertificate and
tls.Config.BuildNameToCertificate, and switching tls.Config.Certificates back to
tls.Config.Certificate tls.Certificate.
This would be more inline with OpenSSL's SSL_CTX_set_tlsext_servername_callback API
where the callback can return a SSL_CTX (analogous to tls.Config) or null, in which case
it uses the existing SSL_CTX.
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Mar 22, 2012

Comment 7:

There are plenty of odd cases where a callback can be nice (enabling client auth only on
some hostnames etc), but the crypto libraries deliberately only aim to solve the 90%
case. And I think the 90% case is that you have several certificates on the same IP and
want to serve the right one.
That doesn't preclude adding the callback in the future, but it would need a firm reason.
If you're doing something odd then there's no shame in forking the code.
@benburkert

This comment has been minimized.

Copy link
Contributor Author

@benburkert benburkert commented Mar 22, 2012

Comment 8:

very true, thanks for the quick response!
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Apr 12, 2012

Comment 9:

This issue was closed by revision e6e8b72.

Status changed to Fixed.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Apr 25, 2012

Comment 10:

This issue was closed by revision 018462b9df95.

agl added a commit that referenced this issue May 11, 2015
… key.

««« backport 6a2ea47583df
crypto/tls: don't always use the default private key.

When SNI based certificate selection is enabled, we previously used
the default private key even if we selected a non-default certificate.

Fixes #3367.

R=golang-dev, bradfitz
CC=golang-dev
https://golang.org/cl/5987058
»»»
@golang golang locked and limited conversation to collaborators Jun 24, 2016
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
When SNI based certificate selection is enabled, we previously used
the default private key even if we selected a non-default certificate.

Fixes golang#3367.

R=golang-dev, bradfitz
CC=golang-dev
https://golang.org/cl/5987058
This issue was closed.
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
3 participants
You can’t perform that action at this time.