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: verification fails with "cannot parse dnsName" in intermediate #23995

Closed
freman opened this issue Feb 21, 2018 · 25 comments · Fixed by weaveworks/weave#3273
Closed
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone

Comments

@freman
Copy link

freman commented Feb 21, 2018

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

go version go1.10 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/shannon/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/shannon/go"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/08/9qf64r3d209cvmgznzj49zg40000gn/T/go-build638418624=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

So it seems at some point in our distant past, we ended up with an intermediate CA that had an altname incorrectly configured. Probably something we munged while configuring Hashicorps Vault as our CA so many months ago.

The end result being that anything written in go and compiled with go 1.10 now fails all our tls connections (where curl, and openssl succeed)

nb, these examples don't actually run on play due to the time being pinned in the past :)

In the real world these certs are part of a bundle offered up by nginx, caddy, vault, consul, and individual services so its a tls error like this we see

tls: failed to parse certificate from server: x509: cannot parse dnsName "SnakeOil Intermediate CA 2"

One might argue that the intermediate certificates are invalid, but another equally fair argument is that maybe we don't need to verify altnames on certificates that are marked for CA's

            X509v3 Basic Constraints: critical
                CA:TRUE

The only evidence I have to support that is that curl, openssl, and chrome don't seem to care.

What did you expect to see?

yay

What did you see instead?

panic: x509: cannot parse dnsName "SnakeOil Intermediate CA 2"

goroutine 1 [running]:
main.main()
	/Users/shannon/tlsissue/ca/test/main.go:164 +0x59f
exit status 2
@FiloSottile FiloSottile changed the title TLS failing Intermediate CA certs with Altnames crypto/x509: verification fails with invalid dnsName in intermediate Feb 21, 2018
@FiloSottile
Copy link
Contributor

FiloSottile commented Feb 21, 2018

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.

@FiloSottile FiloSottile added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 21, 2018
@FiloSottile FiloSottile added this to the Go1.10.1 milestone Feb 21, 2018
@freman
Copy link
Author

freman commented Feb 21, 2018

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.

@agl
Copy link
Contributor

agl commented Feb 22, 2018

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.

@gopherbot
Copy link
Contributor

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

@FiloSottile FiloSottile changed the title crypto/x509: verification fails with invalid dnsName in intermediate crypto/x509: verification fails with "cannot parse dnsName" in intermediate Feb 22, 2018
@FiloSottile
Copy link
Contributor

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.

@l33t0
Copy link

l33t0 commented Feb 22, 2018

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

@freman
Copy link
Author

freman commented Feb 22, 2018

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.

@jamie-digital
Copy link

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"

@FiloSottile
Copy link
Contributor

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

@jamie-digital
Copy link

@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.

@FiloSottile
Copy link
Contributor

@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.

@pschultz
Copy link

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.

@msvechla
Copy link

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.

@bradfitz
Copy link
Contributor

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

@agl
Copy link
Contributor

agl commented Feb 27, 2018

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

@titanous
Copy link
Member

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.

@FiloSottile
Copy link
Contributor

FiloSottile commented Feb 27, 2018

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.

@FiloSottile FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Feb 27, 2018
@ianlancetaylor
Copy link
Contributor

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.

dlespiau added a commit to weaveworks/launcher that referenced this issue Feb 28, 2018
Ilya noticed that the dnsName parser has got more stringent.

golang/go#23995

Stick with go 1.9 until that's addressed.
@agl
Copy link
Contributor

agl commented Feb 28, 2018

Reopening because of potential 1.10.1 inclusion.

@agl agl reopened this Feb 28, 2018
@pzduniak
Copy link

pzduniak commented Mar 3, 2018

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

@jefferai
Copy link

jefferai commented Mar 8, 2018

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.

@jefferai
Copy link

jefferai commented Mar 8, 2018

@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).

@trunet
Copy link

trunet commented Mar 9, 2018

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

@andybons
Copy link
Member

CL 96378 OK for Go 1.10.1

@andybons andybons added CherryPickApproved Used during the release process for point releases and removed NeedsFix The path to resolution is known, but the work has not been done. labels Mar 27, 2018
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue Mar 29, 2018
…l addresses.

Go 1.10 requires that SANs in certificates are valid. However, a
non-trivial number of (generally non-WebPKI) certificates have invalid
strings in dnsName fields and some have even put those dnsName SANs in
CA certificates.

This change defers validity checking until name constraints are checked.

Fixes #23995, #23711.

Change-Id: I2e0ebb0898c047874a3547226b71e3029333b7f1
Reviewed-on: https://go-review.googlesource.com/96378
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/102783
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
grobie added a commit to prometheus/golang-builder that referenced this issue Mar 31, 2018
The latest patch versions include two important fixes for the Prometheus
project:

* golang/go#23995
* golang/go#24059
grobie added a commit to prometheus/golang-builder that referenced this issue Mar 31, 2018
The latest patch versions include two important fixes for the Prometheus
project:

* golang/go#23995
* golang/go#24059
grobie added a commit to prometheus/golang-builder that referenced this issue Mar 31, 2018
The latest patch versions include two important fixes for the Prometheus
project:

* golang/go#23995
* golang/go#24059
jandubois added a commit to SUSE/scf-helper-release that referenced this issue Oct 31, 2018
Using Kubernetes from Docker for Mac I ran into this error
with the scf-secrets-generator:

2018/10/30 14:20:06 Post https://10.96.0.1:443/api/v1/namespaces/my-nats/configmaps: tls: failed to parse certificate from server: x509: cannot parse dnsName "kubernetes.default.svc."

This seems to be a regression in Go 1.10: golang/go#23995

Everything works fine again with Go 1.11.1 (it should be fixed in 1.10.1,
but I went straight to the latest release).
@golang golang locked and limited conversation to collaborators Mar 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

Successfully merging a pull request may close this issue.