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

Jump to bottom

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
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@rikatz
Copy link

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

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

There's been work in this space.

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 29, 2017
@amandahla
Copy link

I believe that this change here caused this behavior:

GO 1.6

go/src/crypto/x509/verify.go

Line 427 in aa1e69f

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)

go/src/crypto/x509/verify.go

Line 1010 in 70ee9b4

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

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

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.

@rikatz rikatz mentioned this issue Nov 29, 2017
Closed
@cyli cyli mentioned this issue Apr 11, 2018
Closed
@golang golang locked and limited conversation to collaborators Nov 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@bradfitz @titanous @rikatz @gopherbot @amandahla