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/smtp: STARTTLS with server certificate hash pinning #9451

Closed
christophershirk opened this issue Dec 27, 2014 · 2 comments
Closed

net/smtp: STARTTLS with server certificate hash pinning #9451

christophershirk opened this issue Dec 27, 2014 · 2 comments

Comments

@christophershirk
Copy link

It would be nice if clients of the net/smtp package could access the TLS connection state in order to implement server certificate hash checking with STARTTLS. Currently net/smtp's (c *Client) does not expose the conn field, which is set during (c *Client) StartTLS().
https://github.com/golang/go/blob/go1.4/src/net/smtp/smtp.go#L154

As an example, this is the kind of server certificate hash checking I'd like to do with net/smtp:
https://github.com/agl/xmpp/blob/a5b10608a5c441a99c6380efa91d7f4fb517e9c2/xmpp.go#L509-517

I ended up forking net/smtp in order to access the TLS connection state after STARTTLS, which is a lot of code duplication for such a small change. If this isn't too esoteric, an additional method on smtp's Client struct could expose the connection state to clients of the package, making server certificate hash verification possible.

@mikioh mikioh changed the title net/smtp STARTTLS with server certificate hash pinning net/smtp: STARTTLS with server certificate hash pinning Dec 27, 2014
@bradfitz bradfitz self-assigned this Dec 28, 2014
@bradfitz
Copy link
Contributor

Sent https://go-review.googlesource.com/2151 net/smtp: add TLSConnectionState accessor

@christophershirk
Copy link
Author

Apologies, @bradfitz , but I think my issue report suggested a naive fix and I'd like to bring this up now before the API change introduced with 26d5573 becomes part of the permanent stdlib API per the point release backwards compatibility guarantee. You may want to revert that change.

Two things stand out to me now that I've looked through the open issues for crypto/tls:

  1. Using the TLSConnectionState accessor might allow for TOC/TOU races if crypto/tls ever implements renegotiation (see crypto/tls: does not support renegotiation #5742 ). That is, if TLS renegotiation occurs after smtp client.TLSConnectionState() but before smtp client.Auth(), the application could get a false negative (in terms of cert pinning) if the server switches certs.

  2. If net/http: DialTLS is not used w/ proxy (by design) #9126 gets fixed, that API would provide a more general-purpose solution, including what I was doing with net/smtp and server certificate hash checking.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants