This is #23711, except here the invalid dnsName is in an intermediate instead of in a leaf. As @agl said this was part of the work to introduce name constraints #15196.

We might decide to ignore these errors in the chain in some cases, but I suspect that would introduce a lot of complexity. To @agl as I'm still getting familiar with crypto/x509.

1.10.1 because technically a regression.

I'm pretty sure that it's (in this case) on the intermediate and the root for us.

I figured it would be a case of checking the if it's a CA cert before checking if it has alt names but I confess I didn't look that deep in the code.

This is horrifying. None the less, it could be called a regression since invalid SANs in CA certificates are not a security problem that I can see.

I've prepared a change to address in case Go folks want to land it, although my recommendation would be a weak no.

Change https://golang.org/cl/96378 mentions this issue: crypto/x509: allow invalid SANs in CA certificates.

I made the title more discoverable, let's see if this affects a lot of people. (If this affects you, please comment or use the GitHub reaction feature.) I would like Go to be a force for cleanup in the X.509 ecosystem too, but a widespread regression would not be acceptable.

We should check that CreateCertificate will refuse to make invalid dnsNames too before closing.

Ah the joys, I've been bitten by this as well.

I would definitely be in favor of making CreateCertificate incapable of generating bad certs, but breaking certs that are already out there is causing inconvenience.
In fact I'm pro breaking those certs too, but it'd be nice if that was some how optional because people have to work.

Our service identifies issues in customers' websites. One part of this is to identify cert chains involving blacklisted certs, such as DigiNotar's. The update to go1.10 has broken this, as it now fails to parse the invalid but blacklisted cert /C=US/O=Global Trustee/OU=Global Trustee/L=Tampa/ST=Florida/SA=Sea Village 10/PC=38477/CN=global trustee:

-----BEGIN CERTIFICATE-----
MIIG3TCCBcWgAwIBAgIRANjzX063hystqwaS4xU4L7AwDQYJKoZIhvcNAQEFBQAw
gZcxCzAJBgNVBAYTAlVTMQswCQYDVQQIEwJVVDEXMBUGA1UEBxMOU2FsdCBMYWtl
IENpdHkxHjAcBgNVBAoTFVRoZSBVU0VSVFJVU1QgTmV0d29yazEhMB8GA1UECxMY
aHR0cDovL3d3dy51c2VydHJ1c3QuY29tMR8wHQYDVQQDExZVVE4tVVNFUkZpcnN0
LUhhcmR3YXJlMB4XDTExMDMxNTAwMDAwMFoXDTE0MDMxNDIzNTk1OVowgeMxCzAJ
BgNVBAYTAlVTMQ4wDAYDVQQREwUzODQ3NzEQMA4GA1UECBMHRmxvcmlkYTEOMAwG
A1UEBxMFVGFtcGExFzAVBgNVBAkTDlNlYSBWaWxsYWdlIDEwMRcwFQYDVQQKEw5H
bG9iYWwgVHJ1c3RlZTEXMBUGA1UECxMOR2xvYmFsIFRydXN0ZWUxKDAmBgNVBAsT
H0hvc3RlZCBieSBHVEkgR3JvdXAgQ29ycG9yYXRpb24xFDASBgNVBAsTC1BsYXRp
bnVtU1NMMRcwFQYDVQQDEw5nbG9iYWwgdHJ1c3RlZTCCAiIwDQYJKoZIhvcNAQEB
BQADggIPADCCAgoCggIBANl08qpBHd/1whZDSVwpv7aJdCm8nI0MRk9ZfrJBF2Y0
DGWJ4Wwl44YKniJFIozdneajld7ciAJVXONbkXXrJmljuS7Gyi4n34i6AiBu/rkL
Kden1tdIGhzO3R+pJw5iT6GWHt1UOjRjSnb1d31ZZ9gQ1LUPOkMimNv0CcQKcM7d
kNQv73QTw83CiTliFZ3mdKjom/BjbpyJtg6tm/fMgujoLbgL2iLsSYUHiJmYP/R0
qQn3gXyXC1mZGHKL25SCK6foqmuXv4h+dbCLRUUMx6gJ6htBWDA7X3hlFTTS5Dw0
DR3YZDyKpVZJmSgtS/LPzdluSWSbqXmQd1WpCButGnSe4AOTCgm3rae0XO+DbLea
tMZoQIAdQtFueZupGSGanPmGLQDRNP7gtvlVtvUmxZUWpXxznwopiaw6mPebdGe3
kLddCSNqau0sEO5TChDwFh9Xs7ENeZEZsOvNMD+gFF+zxv1cM6ew/5iwVYy5pfJv
RyRJIWnMQqJRAECFjIKCqzKly5rc0NkYDd8Z9K+DDcE+MdskSLZ1gKHhyXdkHqfl
i38VTUunwtDteZVekTHsGP9On0gU6nW6Ic4pdukfTlGHLrPMBGC6Ix8fZbIKuNVu
j0tCiUepgZBbK7K2ruagcHt4kAp6xeXnxfsK9i9pjIwfV+AGmf8R1VIyIJcnmO5l
AgMBAAGjggHUMIIB0DAfBgNVHSMEGDAWgBShcl8mGyiYQ5VdBzfVhZadS9LDRTAd
BgNVHQ4EFgQUt8PeGkPtQZepjyl4nAO5rEBCAKwwDgYDVR0PAQH/BAQDAgWgMAwG
A1UdEwEB/wQCMAAwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMCMEYGA1Ud
IAQ/MD0wOwYMKwYBBAGyMQECAQMEMCswKQYIKwYBBQUHAgEWHWh0dHBzOi8vc2Vj
dXJlLmNvbW9kby5jb20vQ1BTMHsGA1UdHwR0MHIwOKA2oDSGMmh0dHA6Ly9jcmwu
Y29tb2RvY2EuY29tL1VUTi1VU0VSRmlyc3QtSGFyZHdhcmUuY3JsMDagNKAyhjBo
dHRwOi8vY3JsLmNvbW9kby5uZXQvVVROLVVTRVJGaXJzdC1IYXJkd2FyZS5jcmww
cQYIKwYBBQUHAQEEZTBjMDsGCCsGAQUFBzAChi9odHRwOi8vY3J0LmNvbW9kb2Nh
LmNvbS9VVE5BZGRUcnVzdFNlcnZlckNBLmNydDAkBggrBgEFBQcwAYYYaHR0cDov
L29jc3AuY29tb2RvY2EuY29tMBkGA1UdEQQSMBCCDmdsb2JhbCB0cnVzdGVlMA0G
CSqGSIb3DQEBBQUAA4IBAQCPunW6OdQm03APxLMCp8USI3HJ/mPpo2J4JERP1LkR
Ph/HKOdVa+704QCRhorJCWufLqRFOdFhYl6TpQVFeJ9gEiz0bGVlDcxGNIsouqDG
9JlxZPMidqxP82LJpzNaBx89yYaA3NsEL4cn6L9IRIHA8Ekjbh/l5AOGJBOihWJ8
WATK5o0Tcgq6VkSiD7z7oD0NKn/7nqkJPbda1IqN4SXopAmEcK0SRLnPuTN6ulzm
S6a7BQaY//KYUnt3gCdK2eL6uVLU+/vm1i2ej8EVRI2bdC/ulFpO08SLiqxDnXP2
rgyHia2HycnH3boUYHr4tTWdwo3GloENqVKKKUAE6Rm0
-----END CERTIFICATE-----

x509: cannot parse dnsName "global trustee"

@jamie-digital For my own curiosity, where does that certificate show up? Because I see it has CA:FALSE.

@FiloSottile I found it in a list of blacklisted CA certs, so presumably someone has/had it in their trust pool. Earlier versions of X.509 didn't have the isCA field, so implementations often don't require the flag for root certs.

@jamie-digital I found the source of that certificate, and it's a weird artefact of a breach. The other certificates were for real sites. https://bugzilla.mozilla.org/show_bug.cgi?id=642395

This means it was never trusted as a root, and with CA:FALSE it's harmless, so in this case you can safely drop checking for it.

This will affect all CA certs that have been generated by HashiCorp Vault 0.9.3 or earlier. Those versions always put the CN also into the DNS alt names, without checking if it actually is a DNS name. hashicorp/vault#3953 fixed that in anticipation of the change in Go.

Relaxing the verification for CA certs would greatly reduce pain for Vault operators because they wouldn't have to re-issue all of their cert chains.

This currently affects our entire vault setup as-well, due to reasons already mentioned by @pschultz.
This forces us to re-issue all our CAs, including our root-ca. Making this verification optional would really help us out a lot.

I'm fine including @agl's fix for this in 1.10.1. @ianlancetaylor?

Given the unexpected spread of this mistake, I'm now at "weak yes" for including this change.

In my opinion this isn't a regression in Go. It is an error to put something that isn't a DNS hostname in a dNSName SAN, and failing to parse/verify such certificates is a reasonable and good thing.

It's unfortunate that some software was generating invalid certificates, but I think they should be replaced instead of letting Go's (already intentionally strict) parser accept them.

Yeah, alas, I agree we need to fix (well, break) this for 1.10.1. It's a fairly widespread break in functionality and a semantic change (parsing/verifying unneeded fields) not just an increase in verification strictness.

Are we talking about putting https://golang.org/cl/96378 into 1.10.1? I'm fine with that if everyone else is although it would be nice to commit it to tip first.

Reopening because of potential 1.10.1 inclusion.

I actually used an EasyRSA CA setup that generated such invalid certs. Nothing complained about it before.

Just to clarify re: #23995 (comment) this didn't break any cert issued where the CN was a valid dns name, and adding the CN to SANs is optional but on by default. Basically, it's in the same boat as a lot of other CAs that didn't validate DNS compatibility, but saying it broke all certs issued by Vault is overkill and incorrect.

@titanous

In my opinion this isn't a regression in Go. It is an error to put something that isn't a DNS hostname in a dNSName SAN, and failing to parse/verify such certificates is a reasonable and good thing.

It's unfortunate that some software was generating invalid certificates, but I think they should be replaced instead of letting Go's (already intentionally strict) parser accept them.

The problem is that such software relying on Go for their crypto stack were allowed, by Go, to generate such invalid certificates, and then disallowed, by Go, to actually read them back once Go 1.10 came out. If Go had never allowed these certificates to be generated in the first place due to strict parsing and validity requirements, I'd be more agreeable to your statement, but I strongly suggest an "allow old/broken certs, disallow generating new broken certs" approach based on where we are right now.

We can all agree it would be nice if such broken certs didn't exist in the first place, but there are plenty of things in the RFC that various libraries only adhere to to some extent or another. In fact, the x509 library explicitly breaks with the spec with verify behavior (https://golang.org/pkg/crypto/x509/#VerifyOptions). Allowing non-DNS-compatible names in DNSNames was likely an unintentional break, unlike the verification logic, but if every crypto stack out there has been successfully accepting such certs that Go created, and Go has been successfully accepting such certs that other CAs created, it's reasonable for users to assume that this was simply a widely-allowed relaxation of the spec (especially given the complexity of dealing with otherNames with Go's asn1 library, which pretty much ends up necessitating using the out-of-stdlib cryptobytes lib).

will this be included on 1.10.1, or we'll have to wait til 1.11?

CL 96378 OK for Go 1.10.1

Change https://golang.org/cl/102783 mentions this issue: [release-branch.go1.10] crypto/x509: parse invalid DNS names and email addresses.