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/http: Allow verification of certificates beyond just hostname #8522

Closed
gopherbot opened this issue Aug 13, 2014 · 11 comments
Closed

net/http: Allow verification of certificates beyond just hostname #8522

gopherbot opened this issue Aug 13, 2014 · 11 comments
Assignees

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2014

by c@apcera.com:

What does 'go version' print?
go version devel +0449858880be Mon Aug 11 17:11:31 2014 -0400 darwin/amd64

What steps reproduce the problem?

1. Revoke a TLS certificate (for example, to mitigate Heartbleed)
2. Attempt to prevent a golang client from securing a connection using the revoked
certificate
3. Realize you can verify the hostname, but not other parts of the certificate prior to
establishing a connection and sending a request.

What happened?

Golang clients cannot currently be written to reject certificates based on factors other
than hostname.

What should have happened instead?

The certificate should be available to client code when establishing connections to
allow for more granular verification. The attached file is client code which could be
used to reject blacklisted certs if a hook were available in net/http.

Attachments:

  1. certblacklist.go (932 bytes)
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Aug 13, 2014

Comment 1:

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

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 13, 2014

Comment 2:

Labels changed: added repo-main, release-none.

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 13, 2014

Comment 3:

I'll own getting this fixed one way or another, but copying Adam in case he has opinions
on where such a hook should live.
For instance, the CL above adds an HTTP-specific hook to check the
*tls.Conn+*tls.Config's suitability once it's already made.
Some alternatives as food for thought:
a)  not add a tls.Conn-checking hook in http but instead just add a DialTLS func
alongside net/http.Transport's Dial (http://golang.org/pkg/net/http/#Transport)
b) modify http://golang.org/pkg/crypto/tls/#Client to take policy in the Config, perhaps
as a func. Or add a Dialer type. (but that's basically the *Config struct).  Then the
user can just add the policy func to the existing net/http Transport's TLSClientConfig.
Is it better to have the policy/hook be in crypto/tls instead where maybe it can close
the connection more nicely, or check things earlier? I'm unsure.

Owner changed to @bradfitz.

Status changed to Accepted.

Loading

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Aug 13, 2014

Comment 4 by c@apcera.com:

Would a)'s DialTLS be of the form func(network, addr string, cfg *tls.Config) (net.Conn,
error) ? I assume net/http would provide a default implementation that used
VerifyHostname internally? This approach seems viable.
b) is a direction I was leaning when thinking about possible cleaner implementations of
the above CL. I wasn't as clear on how to modify crypto/tls properly.

Loading

@philpennock
Copy link

@philpennock philpennock commented Aug 13, 2014

Comment 5:

To me, as someone using the API, (b) makes more sense.  The people using the API aren't
changing how the layers interact, there should be no need to modify the code which the
HTTP layer uses to talk to the TLS layer.
In effect, we want to be able to set TLS policy; in this case, on whether or not to
accept an X.509 certificate; some parts of the policy are already available (chain back
to trust anchor; hostname to validate) others aren't.  This is no different from the TLS
verifications already available, except in not being available, so a callback made the
most sense.
The original PR/patch changed the "net/http".Transport but changing "crypto/tls".Config
instead lets us make the policy checks available to all TLS clients, not just HTTP and
is better, you're right.

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 18, 2014

Comment 6:

Adam: thoughts?

Loading

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Aug 25, 2014

Comment 7 by ox@getlantern.org:

+1 on making this a policy option handled in crypto/tls, configured via something on
tls.Config.

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 2, 2014

Comment 8:

Sent out https://golang.org/cl/137940043 which I believe satisfies all the
needs.

Loading

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 2, 2014

Comment 9:

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

Loading

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 3, 2014

Comment 10 by c@apcera.com:

Brad: I wrote a test based on example_test.go in package tls. Attached here since
https://golang.org/cl/137940043 already has LGTM.

Attachments:

  1. client_verify_test.patch.txt (1126 bytes)

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 8, 2014

Comment 11:

This issue was closed by revision ae47e04.

Status changed to Fixed.

Loading

@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman added a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
Per discussions out of https://golang.org/cl/128930043/
and golang-nuts threads and with agl.

Fixes golang#8522

LGTM=agl, adg
R=agl, c, adg
CC=c, golang-codereviews
https://golang.org/cl/137940043
wheatman added a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
Per discussions out of https://golang.org/cl/128930043/
and golang-nuts threads and with agl.

Fixes golang#8522

LGTM=agl, adg
R=agl, c, adg
CC=c, golang-codereviews
https://golang.org/cl/137940043
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
4 participants