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
Labels
Comments
CL https://golang.org/cl/128930043 mentions this issue. |
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. |
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. |
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. |
Comment 7 by ox@getlantern.org: +1 on making this a policy option handled in crypto/tls, configured via something on tls.Config. |
Sent out https://golang.org/cl/137940043 which I believe satisfies all the needs. |
CL https://golang.org/cl/137940043 mentions this issue. |
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:
|
This issue was closed by revision ae47e04. Status changed to Fixed. |
wheatman
pushed 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
pushed 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.
by c@apcera.com:
Attachments:
The text was updated successfully, but these errors were encountered: