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: VerifyOptions.KeyUsages went from any required to all required in 1.10 #24162

Closed
grittygrease opened this issue Feb 27, 2018 · 7 comments
Closed

crypto/x509: VerifyOptions.KeyUsages went from any required to all required in 1.10 #24162

grittygrease opened this issue Feb 27, 2018 · 7 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.10.1

Comments

@grittygrease
Copy link
Contributor

grittygrease commented Feb 27, 2018 •
edited

The behavior of x509.Verify changed in Go 1.10, which broke a test in CFSSL.

The previous behavior was that when the array of extended key usages in x509.VerifyOptions contains ExtKeyUsageClientAuth, then calling x509.Verify on a valid chain to a leaf certificate with the client certificate auth EKU set results in a valid response, which is the expected behavior. In Go 1.10, this behavior only works when x509.VerifyOptions only contains ExtKeyUsageClientAuth. If x509.VerifyOptions contains both ExtKeyUsageClientAuth and ExtKeyUsageServerAuth, then x509.Verify fails this check for leaf certificate with a client authentication usage.

Is there a new requirement for leaf certificates with Client Auth?

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

go 1.10

Steps to reproduce

The following test fails in cfssl 1.3 (https://github.com/cloudflare/cfssl/tree/1.3.0)
https://github.com/cloudflare/cfssl/blob/master/bundler/bundler_test.go#L914

Output in Go 1.10

go test -v -run TestBundlerClientAuth ./bundler/...
=== RUN   TestBundlerClientAuth
2018/02/27 14:45:30 [INFO] bundling certificate for
2018/02/27 14:45:30 [INFO] the anchoring root is O=Root CA,L=San Francisco,ST=California,C=US
2018/02/27 14:45:30 [INFO] bundling certificate for
--- FAIL: TestBundlerClientAuth (0.02s)
	bundler_test.go:925: {"code":1214,"message":"x509: certificate specifies an incompatible key usage: asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 3, 1}"}
FAIL
FAIL	github.com/cloudflare/cfssl/bundler	0.034s
@grittygrease grittygrease changed the title crypto/x509: x509.Verify enforces EKU constraints on all certificates in a certificate chain instead of leaf in Go 1.10 crypto/x509: x509.Verify enforces EKU constraints differently on client certificates in Go 1.10 Feb 28, 2018
@grittygrease
Copy link
Contributor Author

Excuse the edits. I think this can be boiled down to this loop:

go/src/crypto/x509/verify.go

Line 861 in 0681c7c

for _, eku := range requestedKeyUsages {

in which every EKU in opts.KeyUsages is required to pass the ekuPermittedBy test. Compare this to the previous logic where only one of opts.KeyUsages was required to be valid.

@FiloSottile
Copy link
Contributor

The failure does not reproduce on any tagged cfssl version, checkout commit b9f59aa instead. Longer version (or skip to the <hr>):

cfssl 1.3.0 does not build with Go 1.10, and the test passes in cfssl 1.3.1 because it looks like it was fixed here cloudflare/cfssl@c68df53.

Before that fix Verify was invoked with only x509.ExtKeyUsageClientAuth in VerifyOptions (WithKeyUsages(x509.ExtKeyUsageClientAuth)) on a certificate with only ServerAuth EKU:

Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            29:cb:26:4b:b1:a9:26:b0:11:a4:16:b9:de:4d:fe:fa:07:db:7e:cd
    Signature Algorithm: ecdsa-with-SHA256
        Issuer:
        Validity
            Not Before: Sep 20 20:06:00 2016 GMT
            Not After : Sep 21 02:06:00 2021 GMT
        Subject:
        Subject Public Key Info:
            Public Key Algorithm: id-ecPublicKey
                Public-Key: (256 bit)
                pub:
                    04:dc:dd:b6:ce:ed:77:77:9d:c4:c1:50:25:91:cc:
                    41:a5:23:76:fa:6a:3d:06:29:c4:29:2e:4e:9f:96:
                    f8:3d:ae:89:3e:69:ed:ff:81:46:2a:b5:7b:fe:7c:
                    2a:a0:e9:c4:0a:27:d6:7b:9b:b9:a9:63:a8:9d:db:
                    65:7a:75:98:80
                ASN1 OID: prime256v1
                NIST CURVE: P-256
        X509v3 extensions:
            X509v3 Extended Key Usage:
                TLS Web Server Authentication
            X509v3 Basic Constraints: critical
                CA:FALSE
            X509v3 Subject Key Identifier:
                2A:71:13:98:C7:E3:FB:52:D8:CF:45:88:FD:F2:66:50:83:7A:70:F5
            X509v3 Authority Key Identifier:
                keyid:2F:95:E2:1D:11:36:B7:30:22:88:49:89:7A:17:D8:B3:76:34:29:D6

    Signature Algorithm: ecdsa-with-SHA256
         30:46:02:21:00:de:e2:8d:25:ab:3e:9e:be:74:fd:59:e4:1a:
         09:1c:78:cd:64:89:e8:36:c6:46:8a:fe:8d:96:81:d6:f0:bf:
         38:02:21:00:9a:a6:d5:14:cd:b7:2f:56:74:40:1e:5b:f1:2f:
         4b:6f:a9:2b:34:b8:57:3c:52:68:98:cd:c4:4d:12:e8:15:e0
-----BEGIN CERTIFICATE-----
MIIBaDCCAQ2gAwIBAgIUKcsmS7GpJrARpBa53k3++gfbfs0wCgYIKoZIzj0EAwIw
ADAeFw0xNjA5MjAyMDA2MDBaFw0yMTA5MjEwMjA2MDBaMAAwWTATBgcqhkjOPQIB
BggqhkjOPQMBBwNCAATc3bbO7Xd3ncTBUCWRzEGlI3b6aj0GKcQpLk6flvg9rok+
ae3/gUYqtXv+fCqg6cQKJ9Z7m7mpY6id22V6dZiAo2UwYzATBgNVHSUEDDAKBggr
BgEFBQcDATAMBgNVHRMBAf8EAjAAMB0GA1UdDgQWBBQqcROYx+P7UtjPRYj98mZQ
g3pw9TAfBgNVHSMEGDAWgBQvleIdETa3MCKISYl6F9izdjQp1jAKBggqhkjOPQQD
AgNJADBGAiEA3uKNJas+nr50/VnkGgkceM1kieg2xkaK/o2WgdbwvzgCIQCaptUU
zbcvVnRAHlvxL0tvqSs0uFc8UmiYzcRNEugV4A==
-----END CERTIFICATE-----

So failing is expected behavior AFAICT.

But still I couldn't figure out why it was failing before, because the point of #23884 was to make this specific case fail again. So I checked out c68df53^ (e9bb1b2) and tested with Go 1.10, and indeed it passed, while it failed with tip as expected.

But then again it was failing on the other certificate. WithKeyUsages(x509.ExtKeyUsageClientAuth) was indeed introduced at cloudflare/cfssl@459761ceb. Rollbacking to before that commit gets us to a test that fails with Go 1.10 on the ServerAuth certificate and with tip on the ClientAuth certificate (or probably both). (#23884 explains the discrepancy.)

Verify is being called with four values in VerifyOptions.KeyUsages.

KeyUsages: []x509.ExtKeyUsage{
    x509.ExtKeyUsageServerAuth,
    x509.ExtKeyUsageClientAuth,
    x509.ExtKeyUsageMicrosoftServerGatedCrypto,
    x509.ExtKeyUsageNetscapeServerGatedCrypto,
},

The certificates only have ExtKeyUsageServerAuth or ExtKeyUsageClientAuth.

The current behavior is that all VerifyOptions.KeyUsages need to match for verification to pass, so this fails.

go/src/crypto/x509/verify.go

Lines 860 to 870 in b5bd5bf

NextUsage:
for _, eku := range requestedKeyUsages {
for _, leafEKU := range c.ExtKeyUsage {
if ekuPermittedBy(eku, leafEKU, checkingAgainstLeafCert) {
continue NextUsage
}
}
oid, _ := oidFromExtKeyUsage(eku)
return nil, CertificateInvalidError{c, IncompatibleUsage, fmt.Sprintf("%#v", oid)}
}

However the previous behavior was that any VerifyOptions.KeyUsages needed to match for verification to pass.

go/src/crypto/x509/verify.go

Lines 515 to 517 in 6732fcc

// We walk down the list and cross out any usages that aren't supported
// by each certificate. If we cross out all the usages, then the chain
// is unacceptable.

The old behavior matches the docs, even if vague.

// KeyUsage specifies which Extended Key Usage values are acceptable.

So this is a pretty bad regression AFAICT, breaking most multi-value uses of VerifyOptions.KeyUsages (which must be pretty rare for no one to notice). Targeting 1.10.1.

(I don't think SANs are involved, nor the root or intermediate lack of EKUs.)

/cc @agl

@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 28, 2018
@FiloSottile FiloSottile added this to the Go1.10.1 milestone Feb 28, 2018
@FiloSottile FiloSottile changed the title crypto/x509: x509.Verify enforces EKU constraints differently on client certificates in Go 1.10 crypto/x509: VerifyOptions.KeyUsages went from any required to all required in 1.10 Feb 28, 2018
@gopherbot
Copy link

Change https://golang.org/cl/97720 mentions this issue: crypto/x509: matching any requested EKU should be sufficient.

@agl
Copy link
Contributor

agl commented Feb 28, 2018

I misremembered this and thought that all specified EKUs needed to match, but the previous behaviour was clearly that any of them was sufficient.

Sending CL to specify that in the docs and restore old behaviour.

@FiloSottile
Copy link
Contributor

Reopening for cherry-pick.

@FiloSottile FiloSottile reopened this Mar 26, 2018
@andybons andybons mentioned this issue Mar 27, 2018
Closed
@andybons
Copy link
Member

CL 97720 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

Change https://golang.org/cl/102790 mentions this issue: [release-branch.go1.10] crypto/x509: matching any requested EKU should be sufficient.

gopherbot pushed a commit that referenced this issue Mar 29, 2018
@agl @andybons
[release-branch.go1.10] crypto/x509: matching any requested EKU shoul…
c917b3c 
…d be sufficient.

The documentation was unclear here and I misremembered the behaviour and
changed it in 1.10: it used to be that matching any EKU was enough but
1.10 requires that all EKUs match.

Restore 1.9 behaviour and clarify the documentation to make it official.

Fixes #24162.

Change-Id: Ic9466cd0799cb27ec3a3a7e6c96f10c2aacc7020
Reviewed-on: https://go-review.googlesource.com/97720
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-on: https://go-review.googlesource.com/102790
Run-TryBot: Andrew Bonventre <andybons@golang.org>
@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.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.10.1
Development

No branches or pull requests
5 participants
@agl @andybons @FiloSottile @grittygrease @gopherbot