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: mention in the InsecureSkipVerify docs that it's ok to use with Verify callbacks #39074

Closed
ZhenLian opened this issue May 14, 2020 · 3 comments
Labels
Documentation FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@ZhenLian
Copy link

Hi Go team,

Our teams(security team and gRPC-Go team) are working on a project that might need to set the field 'InsecureSkipVerify' to true to enable 'VerfiyPeerCertificate' in tls.config. However, according to the comments,

This should be used only for testing.

we are a bit hesitant to set InsecureSkipVerify to true, since it is going to be used in production code. So my questions are:

  1. if we set InsecureSkipVerify to true but provide proper endpoint verification mechanisms other than the default hostname check, can it be used in the real environment?

  2. If the answer to the first question is "yes", can we improve the comments a little bit? If that's the case, "used only for testing" might be a bit misleading.

Thank you so much for the help!

@FiloSottile

@easwars
Copy link

easwars commented May 14, 2020

Also, would it make sense for the tls package to allow overriding only the hostname check?

That way, the tls package could still verify the cert chain provided by the peer, but could let the user do custom hostname checks like verifying that the hostname in the peer cert matches a list of acceptable host names or if it matches a wildcard SPIFFE id.

This could be similar to how the user can specify client certificates. They can either provide them upfront with the Certificates field of tls.Config or they can set the GetClientCertificate function which will be invoked at handshake time.

Similarly, we currently have ServerName which the user can specify upfront to check the hostname on the peer cert. If we also had something like CheckServerName(leaf *x509.Certificate) bool, user could provide custom server name checks.

@ALTree ALTree added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Documentation labels May 14, 2020
@FiloSottile
Copy link
Contributor

Ah, yes, the docs should be a little less categorical. If you set VerifyPeerCertificate you are fine to set InsecureSkipVerify (but should still be careful because you are without a parachute if your VerifyPeerCertificate is wrong).

Once you need to customize verification, there are too many different needs, so I prefer not to add a callback for each of them. Instead, in Go 1.15 we are adding an easier to use VerifyConnection callback (#36736). I'll drop in an example of how changing name verification is pretty easy with it.

@FiloSottile FiloSottile changed the title [Discussion] Setting InsecureSkipVerify to "true" in Production Env crypto/tls: mention in the InsecureSkipVerify docs that it's ok to use with Verify callbacks May 20, 2020
@gopherbot
Copy link

Change https://golang.org/cl/239746 mentions this issue: crypto/tls: relax the docs of InsecureSkipVerify

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants