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: AppendCertsFromPEM fails on go1.10rc1 #23711

Closed
magiconair opened this issue Feb 6, 2018 · 7 comments
Closed

crypto/x509: AppendCertsFromPEM fails on go1.10rc1 #23711

magiconair opened this issue Feb 6, 2018 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.10

Comments

@magiconair
Copy link
Contributor

magiconair commented Feb 6, 2018 •
edited

When testing fabio with go1.10rc1 I've stumbled over a failing test (fabiolb/fabio#434)

I've traced it down to a call to AppendCertsFromPEM failing with a certificate that works fine in go1.9.2. The cert was created using the HashiCorp Vault PKI backend. I've included one of the certs in a full test case below.

I've checked for a note in https://tip.golang.org/doc/go1.10#library but couldn't find anything obvious. Please let me know if I missed something.

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

go1.10rc1

Does this issue reproduce with the latest release?

yes, checked master a0222ec

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

darwin_amd64 on macOS 10.13.3

What did you do?

see test case

package main

import (
	"crypto/x509"
	"fmt"
	"log"
)

const cert = `-----BEGIN CERTIFICATE-----
MIIDOzCCAiOgAwIBAgIUaFacnWI+18PkzWw3L1ZhDyWE6/UwDQYJKoZIhvcNAQEL
BQAwGDEWMBQGA1UEAxMNRmFiaW8gVGVzdCBDQTAeFw0xODAyMDYwMjA0MTNaFw0x
ODAyMDYwNDA0NDNaMBgxFjAUBgNVBAMTDUZhYmlvIFRlc3QgQ0EwggEiMA0GCSqG
SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDIBohqz/hHEN6y2emMDG/fgsggbAXzrIkw
58orFVbK/+hGFMUPpBTZ15r5rYW5nNhIf/tyecLN1CSy/12HLbpx7ebpvMOj2i7b
/VgiadSpTklJEGUIt+nYPeLT+8qLXqiOQA4CwsES/UgQ3h+QMtwjAdcaloho1Ara
RDctRLlEf/VvBYVozPqYfbKRh4ofmqLo9KJFF8yMAOgkFsSmP8/Pv8Cii2fThyGB
v4J56uPUxHW5ROuy1WGu25rEB7Txuvqgpl9m7lN/oqkUhHrUMwFUx5kcG0fWPc2t
Rv/mePw+cDh/5P8IsBLgYGpjGKUGgQduE4LHjjRyUf/xnzuW8jOrAgMBAAGjfTB7
MA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBTIDU8u
KA3PavIxercuzYTMDcSDcTAfBgNVHSMEGDAWgBTIDU8uKA3PavIxercuzYTMDcSD
cTAYBgNVHREEETAPgg1GYWJpbyBUZXN0IENBMA0GCSqGSIb3DQEBCwUAA4IBAQBr
0Nn+msFZjAA4gpHxt/7EvyfbUTjNmvXKALvhuX6z0qgdlFWVFYutNIkbKPF3a06H
gVW7zIvnKg2BX+bkhM/C/IC3oZsD3gK56A4Q71tK9q8QG0cWcqCyfqtG458Q1i8A
05t9rJ1V6ppN9VWe11tvTgEz5FK0e7jnQQEXH8hNjAXQXX7sbQzI+5GagUA3/ZT4
7bl/lkWg9yZCl/lM8UbFxlaRow4sVX5080ilrGlbCn29Ob6kFzWEv7rXs7BXrNgN
YgWDKTrsa4uD7NTuMmoJvsIw7FLkE8XgNNwuURVLsJ4/+TPbfdbVsZW8qO0s0dR+
AEFqi3JlxPWFVBU01oys
-----END CERTIFICATE-----`

func main() {
	p := x509.NewCertPool()
	ok := p.AppendCertsFromPEM([]byte(cert))
	fmt.Printf("p: %#v\n", p)
	if !ok {
		log.Fatal("AppendCertsFromPEM failed")
	}
}
$ ~/go1.9.2/bin/go run main.go
p: &x509.CertPool{bySubjectKeyId:map[string][]int{"\xc8\rO.(\r\xcfj\xf21z\xb7.̈́\xcc\răq":[]int{0}}, byName:map[string][]int{"0\x181\x160\x14\x06\x03U\x04\x03\x13\rFabio Test CA":[]int{0}}, certs:[]*x509.Certificate{(*x509.Certificate)(0xc4200a4000)}}

$ ~/go1.10rc1/bin/go run main.go
p: &x509.CertPool{bySubjectKeyId:map[string][]int{}, byName:map[string][]int{}, certs:[]*x509.Certificate(nil)}
2018/02/06 03:57:46 AppendCertsFromPEM failed
exit status 1
Update 1: updated the test app to be git bisect friendly
@magiconair
Copy link
Contributor Author

This looks like the commit that changed the behavior:

commit 9e76ce70701ceef8fbccfb953b33a2ae7fe0367c
Author: Adam Langley <agl@golang.org>
Date:   Sat Sep 9 17:05:41 2017 -0700

    crypto/x509: enforce all name constraints and support IP, email and URI constraints

    This change makes crypto/x509 enforce name constraints for all names in
    a leaf certificate, not just the name being validated. Thus, after this
    change, if a certificate validates then all the names in it can be
    trusted – one doesn't have a validate again for each interesting name.

    Making extended key usage work in this fashion still remains to be done.

    Updates #15196

    Change-Id: I72ed5ff2f7284082d5bf3e1e86faf76cef62f9b5
    Reviewed-on: https://go-review.googlesource.com/62693
    Run-TryBot: Adam Langley <agl@golang.org>
    TryBot-Result: Gobot Gobot <gobot@golang.org>
    Reviewed-by: Russ Cox <rsc@golang.org>

found with git bisect and then manually confirmed that the preceding commit a4aa5c3 works.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 6, 2018

AppendCertsFromPEM hides errors. The actual error is cannot parse dnsName "Fabio Test CA", as shown in:

bradfitz@gdev:~$ cat x.go
package main
 
import (
        "crypto/x509"
        "encoding/pem"
        "fmt"
        "log"
)
 
func main() {
        const cert = `-----BEGIN CERTIFICATE-----
MIIDOzCCAiOgAwIBAgIUaFacnWI+18PkzWw3L1ZhDyWE6/UwDQYJKoZIhvcNAQEL
BQAwGDEWMBQGA1UEAxMNRmFiaW8gVGVzdCBDQTAeFw0xODAyMDYwMjA0MTNaFw0x
ODAyMDYwNDA0NDNaMBgxFjAUBgNVBAMTDUZhYmlvIFRlc3QgQ0EwggEiMA0GCSqG
SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDIBohqz/hHEN6y2emMDG/fgsggbAXzrIkw
58orFVbK/+hGFMUPpBTZ15r5rYW5nNhIf/tyecLN1CSy/12HLbpx7ebpvMOj2i7b
/VgiadSpTklJEGUIt+nYPeLT+8qLXqiOQA4CwsES/UgQ3h+QMtwjAdcaloho1Ara
RDctRLlEf/VvBYVozPqYfbKRh4ofmqLo9KJFF8yMAOgkFsSmP8/Pv8Cii2fThyGB
v4J56uPUxHW5ROuy1WGu25rEB7Txuvqgpl9m7lN/oqkUhHrUMwFUx5kcG0fWPc2t
Rv/mePw+cDh/5P8IsBLgYGpjGKUGgQduE4LHjjRyUf/xnzuW8jOrAgMBAAGjfTB7
MA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBTIDU8u
KA3PavIxercuzYTMDcSDcTAfBgNVHSMEGDAWgBTIDU8uKA3PavIxercuzYTMDcSD
cTAYBgNVHREEETAPgg1GYWJpbyBUZXN0IENBMA0GCSqGSIb3DQEBCwUAA4IBAQBr
0Nn+msFZjAA4gpHxt/7EvyfbUTjNmvXKALvhuX6z0qgdlFWVFYutNIkbKPF3a06H
gVW7zIvnKg2BX+bkhM/C/IC3oZsD3gK56A4Q71tK9q8QG0cWcqCyfqtG458Q1i8A
05t9rJ1V6ppN9VWe11tvTgEz5FK0e7jnQQEXH8hNjAXQXX7sbQzI+5GagUA3/ZT4
7bl/lkWg9yZCl/lM8UbFxlaRow4sVX5080ilrGlbCn29Ob6kFzWEv7rXs7BXrNgN
YgWDKTrsa4uD7NTuMmoJvsIw7FLkE8XgNNwuURVLsJ4/+TPbfdbVsZW8qO0s0dR+
AEFqi3JlxPWFVBU01oys
-----END CERTIFICATE-----`

        block, _ := pem.Decode([]byte(cert))
        if block == nil {
                log.Fatal("no block")
        }
        if block.Type != "CERTIFICATE" || len(block.Headers) != 0 {
                log.Fatalf("Bogus: %#v", block)
        }

        _, err := x509.ParseCertificate(block.Bytes)
        if err != nil {
                log.Fatalf("ParseCertificate: %v", err)
        }
        fmt.Println("ok")
}

bradfitz@gdev:~$ GOROOT=$HOME/go1.9 go run x.go
ok

bradfitz@gdev:~$ go run x.go
2018/02/06 06:16:21 ParseCertificate: x509: cannot parse dnsName "Fabio Test CA"
exit status 1

bradfitz@gdev:~$ go version
go version devel +a0222ec Tue Feb 6 00:25:23 2018 +0000 linux/amd64

This is probably working as intended. @agl?

@bradfitz bradfitz changed the title AppendCertsFromPEM fails on go1.10rc1 crypto/x509: AppendCertsFromPEM fails on go1.10rc1 Feb 6, 2018
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 6, 2018
@bradfitz bradfitz added this to the Go1.10 milestone Feb 6, 2018
@magiconair
Copy link
Contributor Author

@bradfitz Yes, changing the name to a DNS pattern like fabio-ca.com fixes it. Thx a lot

Maybe add something to the release notes? Feel free to close this.

@magiconair magiconair mentioned this issue Feb 6, 2018
Open
@agl
Copy link
Contributor

agl commented Feb 6, 2018

PEM, in general, tries to pull out certificates or private keys embedded in basically anything. Thus it'll blow through all sorts of PEM syntax errors looking for something valid.

In that vein, AppendCertsFromPEM also ignores certificate parsing errors and carries on to try and find something valid. That may have been a bridge too far and I can certainly see that, once we find a valid PEM block with the correct type, errors should be bubbled up.

If others agree, I can cook up that change. (Although it's someone else's call whether it makes 1.10.)

But, otherwise, it's unfortunate that we allowed invalid DNS names to survive for so long. But, to make the name constraints code sane, an invariant was needed that DNS names in parsed certificates look vaguely DNS-like.

@gopherbot
Copy link

Change https://golang.org/cl/92635 mentions this issue: doc: note that x509 cert parsing rejects some more certs now

gopherbot pushed a commit that referenced this issue Feb 12, 2018
@bradfitz @andybons
doc: note that x509 cert parsing rejects some more certs now
74b5602 
Fixes #23711
This was referenced Feb 21, 2018
Closed
Closed
@gopherbot
Copy link

Change https://golang.org/cl/96378 mentions this issue: crypto/x509: parse invalid DNS names and email addresses.

gopherbot pushed a commit that referenced this issue Feb 28, 2018
@agl
crypto/x509: parse invalid DNS names and email addresses.
4b1d704 
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>
@dannyk81 dannyk81 mentioned this issue Mar 19, 2018
Closed
@gopherbot
Copy link

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
@agl @andybons
[release-branch.go1.10] crypto/x509: parse invalid DNS names and emai…
b8c62b1 
…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>
@golang golang locked and limited conversation to collaborators Mar 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
4 participants
@bradfitz @agl @magiconair @gopherbot