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/x509: VerifyHostname validating wrong SAN #22922

Closed
rikatz opened this issue Nov 29, 2017 · 4 comments
Closed

crypto/x509: VerifyHostname validating wrong SAN #22922

rikatz opened this issue Nov 29, 2017 · 4 comments

Comments

@rikatz
Copy link

@rikatz rikatz commented Nov 29, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)? 1.9.2

Does this issue reproduce with the latest release? Yes

What operating system and processor architecture are you using (go env)?

GOHOSTARCH="amd64"
GOHOSTOS="linux"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

I'm using a program (NGINX Ingress Controller) that uses the VerifyHostname.

While this function is called, it returns me an error about Certificate not containing a valid CommonName (certificate is not valid for any names, but wanted to match server.domain.tld).

Looking to the Certificate, it does contain the name server.domain.tld as a CommonName.

While investigating, I've faced that it's entering the if here.

This function verifies if the Certificate has an extension (and it does, an email, but not a DNSName) and if this is the case, it tries to add the value to the rest of the 'valid' string.

As my certificate does contain an extension, but not a DNSName, shouldn't this function be something like if len(c.DNSNames) > 0 instead of c.hasSANExtension()?

If this is the case, I can open a PR to fix this :) Anyway I'm going to ask the re-creation of the Certificate with the right SAN.

Thanks!

@bradfitz bradfitz changed the title x509 VerifyHostname validating wrong SAN crypto/x509: VerifyHostname validating wrong SAN Nov 29, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 29, 2017

Have you tried Go master? (what will become Go 1.10 in early Feb)

There's been work in this space.

@amandahla
Copy link

@amandahla amandahla commented Nov 29, 2017

I believe that this change here caused this behavior:

GO 1.6

if len(c.DNSNames) > 0 {

if len(c.DNSNames) > 0 {
		for _, match := range c.DNSNames {
			if matchHostnames(toLowerCaseASCII(match), lowered) {
				return nil
			}
		}
		// If Subject Alt Name is given, we ignore the common name.
	} else if matchHostnames(toLowerCaseASCII(c.Subject.CommonName), lowered) {
		return nil
	}

GO 1.9 (master)

if c.hasSANExtension() {

if c.hasSANExtension() {
		for _, match := range c.DNSNames {
			if matchHostnames(toLowerCaseASCII(match), lowered) {
				return nil
			}
		}
		// If Subject Alt Name is given, we ignore the common name.
	} else if matchHostnames(toLowerCaseASCII(c.Subject.CommonName), lowered) {
		return nil
	}
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 29, 2017

Please file separate bugs for separate issues.

But keep in mind we don't fix old versions of Go. We mostly care about recent regressions, not long-standing bugs. We fix long-standing bugs in the next Go release (Go 1.10, in Feb). So please try Go master now and report any issues with that.

@titanous
Copy link
Member

@titanous titanous commented Nov 29, 2017

This is working as intended. commonName is deprecated and is intentionally not used if a SAN extension is present (whether or not it has dnsNames).

The commit message for the change mentioned above makes this clear:

crypto/x509: ignore CN if SAN extension present.

The code previously tested only whether DNS-name SANs were present in a
certificate which is only approximately correct. In fact, /any/ SAN
extension, including one with no DNS names, should cause the CN to be
ignored.

This post explains the rationale for the complete removal of support commonName in Chrome, the same information is relevant in this case.

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
5 participants
You can’t perform that action at this time.