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: Documentation and/or signature of crypto/tls.Conn.VerifyHostname could be clearer #9063

Closed
gopherbot opened this issue Nov 5, 2014 · 6 comments

Comments

@gopherbot
Copy link

by ox@getlantern.org:

What does 'go version' print?

go version go1.3.3 darwin/amd64

What steps reproduce the problem?
If possible, include a link to a program on play.golang.org.

See this gist - https://gist.github.com/oxtoacart/e13883d91039dc44f5e6

What happened?

Function TestUsingVerifyHostname fails.

What should have happened instead?

It's actually okay that this test fails, but not intuitive.  conn.VerifyHostname doesn't
actually validate the certificate chain against the configured RootCAs.  In fact, it
doesn't really do anything with the "chain" at all, it simply checks the
peer's certificate.  Here's the code snippet from conn.go:

c.peerCertificates[0].VerifyHostname(host)

To make this clearer, I think the documentation for that function should read something
like:

"VerifyHostname checks that the peer's certificate is a valid certificate for the
named host. If so, it returns nil; if not, it returns an error describing the problem.
WARNING - VerifyHostname does not validate the peer certificate chain against any
CAs."

Perhaps even better, the function could have a signature that accepts an *x509.CertPool
called RootCAs and, if provided, actually does validate agains them.
@ianlancetaylor
Copy link
Contributor

Comment 1:

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

@agl
Copy link
Contributor

agl commented Nov 10, 2014

Comment 2:

Labels changed: added release-go1.5, removed release-none.

@agl
Copy link
Contributor

agl commented Nov 10, 2014

Comment 3:

Owner changed to @agl.

@rsc
Copy link
Contributor

rsc commented Jul 21, 2015

I checked the code and consulted @agl, and it looks like the only way c.peerCertificates[0] is not part of a verified chain is if you set InsecureSkipVerify in the TLS config. Are you doing that? If not, I think the docs are clear. And if you're setting InsecureSkipVerify, it's not too surprising that VerifyHostname does not do the "verify" part.

@rsc rsc modified the milestones: Go1.5Maybe, Go1.5 Jul 21, 2015
@gopherbot
Copy link
Author

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

@gopherbot
Copy link
Author

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

@rsc rsc closed this as completed in 3cf15b5 Jul 22, 2015
@mikioh mikioh modified the milestones: Go1.5, Go1.5Maybe Jul 23, 2015
@golang golang locked and limited conversation to collaborators Aug 5, 2016
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Fixes golang#9063.

Change-Id: I536ef1f0b30c94c1ebf7922d84cb2f701b7d8a1a
Reviewed-on: https://go-review.googlesource.com/12526
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Fixes golang#9063.

Change-Id: I536ef1f0b30c94c1ebf7922d84cb2f701b7d8a1a
Reviewed-on: https://go-review.googlesource.com/12526
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
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

6 participants