crypto/x509: AppendCertsFromPEM fails on go1.10rc1

[magiconair]

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]

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]

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?

[magiconair]

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

[agl]

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]

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

[gopherbot]

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