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: feature request: add option to JUST skip hostname verification #21971

Closed
robin865 opened this issue Sep 21, 2017 · 14 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@robin865
Copy link

I understand in "fca335e" we made changes to enforce either requiring ServerName or InsecureSkipVerify to be set in our tls libraries.

What about making this the default behaviour but having a "SkipHostnameVerification" option? I have a situation where we are using certs signed by our own private CA. The hostname verification won't work since we pre-generate certs using a CN that isn't an actual DSN/Host name. I still want to validate that other servers certs are at least signed by the same private CA as the client but JUST want to skip the hostnameVerify check. Today that doesn't seem possible.

@ianlancetaylor ianlancetaylor changed the title feature request: add option to JUST skip hostname verification crypto/tls: feature request: add option to JUST skip hostname verification Sep 21, 2017
@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 21, 2017
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Sep 21, 2017
@ianlancetaylor
Copy link
Contributor

CC @agl @FiloSottile

@FiloSottile
Copy link
Contributor

You can currently do this by using VerifyPeerCertificate and (*Certificate).Verify (and remembering to put the remaining rawCerts into VerifyOptions.Intermediates).

certs := make([]*x509.Certificate, len(certMsg.certificates))
for i, asn1Data := range certMsg.certificates {
cert, err := x509.ParseCertificate(asn1Data)
if err != nil {
c.sendAlert(alertBadCertificate)
return errors.New("tls: failed to parse certificate from server: " + err.Error())
}
certs[i] = cert
}
if !c.config.InsecureSkipVerify {
opts := x509.VerifyOptions{
Roots: c.config.RootCAs,
CurrentTime: c.config.time(),
DNSName: c.config.ServerName,
Intermediates: x509.NewCertPool(),
}
for i, cert := range certs {
if i == 0 {
continue
}
opts.Intermediates.AddCert(cert)
}
c.verifiedChains, err = certs[0].Verify(opts)
if err != nil {
c.sendAlert(alertBadCertificate)
return err
}
}

I don't think this is common enough to add another verification option to the already crowded tls.Config.

@robin865
Copy link
Author

robin865 commented Oct 5, 2017 via email

@robin865
Copy link
Author

As to your original suggestion, curious why on lines 334-336 we seem to skip the first cert in the chain?

	if i == 0 { 
		continue 
	}

@FiloSottile
Copy link
Contributor

The first cert is the leaf, and it’s used on line 339. All the others are intermediates and are used on line 337.

@rsc
Copy link
Contributor

rsc commented Oct 23, 2017

Problem is I want to use a library (rafthttp) that only exposes a transport.TLSInfo (https://github.com/coreos/etcd/blob/master/pkg/transport/listener.go) , not a tls Config

This is a very special case. I would suggest working with the library maintainers (or just having your own fork) to expose the tls.Config. We shouldn't add a second way to do something in the standard library (especially a somewhat confusing security knob) just because one downstream package hides the first way.

@benma
Copy link

benma commented Aug 14, 2018

I am also interested in skipping the hostname verification only. The problem is that if the ServerName is unset, it ill be inferred. As stated above, one way to achieve it is to set InsecureSkipVerify to true and copy/paste the code from the stdlib to config.VerifyPeerCertificate, but that is suboptimal, as one would miss fixes done to this part of the code in the stdlib.

👍 for a SkipHostnameVerification option.

@sikemullivan
Copy link

👍 for a SkipHostnameVerification option as well.

Trying to get client authentication working with MySQL. Using a self-signed cert because I'm running everything out of VPS machines. Everything is IP based. Has anyone built the code for the suggested work around? I don't want to reinvent the wheel.

@sikemullivan
Copy link

Awesome, thanks for the quick response.

@sikemullivan
Copy link

@benma

Should line 109 set the verified chains? Or does it not matter?

Now:

_, err := certs[0].Verify(opts)
return err

Then:

verifiedChains, err := certs[0].Verify(opts)
return err

@benma
Copy link

benma commented Nov 23, 2018

Based on a quick look through the stdlib, I would say it would make sense, yes. Seems to be exposed only in conn.ConnectionState(), and conn.VerifyHostname(). Not sure why I didn't put it in back then.

That was too quick. I looked again, and verifiedChains is input to the function, not output. The api/callback doesn't support setting verifiedChains (which should probably be fixed upstream).

@sikemullivan
Copy link

Agree, everything seems to work without it getting returned. Thanks for your help.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/193620 mentions this issue: crypto/tls: add ExampleConfig_VerifyPeerCertificate

gopherbot pushed a commit that referenced this issue Nov 9, 2019
Setting InsecureSkipVerify and VerifyPeerCertificate is the recommended
way to customize and override certificate validation.

However, there is boilerplate involved and it usually requires first
reimplementing the default validation strategy to then customize it.
Provide an example that does the same thing as the default as a starting
point.

Examples of where we directed users to do something similar are in
issues #35467, #31791, #28754, #21971, and #24151.

Fixes #31792

Change-Id: Id033e9fa3cac9dff1f7be05c72dfb34b4f973fd4
Reviewed-on: https://go-review.googlesource.com/c/go/+/193620
Reviewed-by: Adam Langley <agl@golang.org>
gcurtis added a commit to gcurtis/pgconn that referenced this issue Mar 18, 2020
ParseConfig currently treats the libpq "verify-ca" SSL mode as
"verify-full". This is okay from a security standpoint because
"verify-full" performs certificate verification and hostname
verification, whereas "verify-ca" only performs certificate
verification.

The downside to this approach is that checking the hostname is
unnecessary when the server's certificate has been signed by a private
CA. It can also cause the SSL handshake to fail when connecting to an
instance by IP. For example, a Google Cloud SQL instance typically
doesn't have a hostname and uses its own private CA to sign its
server and client certs.

This change uses the tls.Config.VerifyPeerCertificate function to
perform certificate verification without checking the hostname when the
"verify-ca" SSL mode is set. This brings pgconn's behavior closer to
that of libpq.

See golang/go#21971 (comment)
and https://pkg.go.dev/crypto/tls?tab=doc#example-Config-VerifyPeerCertificate
for more details on how this is implemented.
@golang golang locked and limited conversation to collaborators Sep 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants