Fix attestation manufacture bug in old yubikeys

[oreparaz]

This changeset works around a manufacture bug in some 4C Nano yubikeys from the 853#### range. The PIV attestation on these yubikeys doesn't work as expected because the PIV device certificate was certified using the U2F root CA (instead of the PIV root CA). Yubico has acknowledged this issue recently in [0]. Thanks to Chris Halos from Yubico for helping root cause this.

This changeset adds the U2F root CA as an additional CA to work around this. After this PR, go-piv attestation works with both yubikeys from the lucky batch and more recent yubikeys that don't exhibit the bug.

Since testing this without a Yubikey from this series is difficult, I've added new positive and negative tests for both attestation chains.

[0] Yubico/developers.yubico.com@a58f100

This is a snippet of a device cert. Notice the root U2F CA certifying a PIV attestation cert

    Data:
        Version: 3 (0x2)
        Serial Number: 13542434377556493093 (0xbbf06450c44b0325)
    Signature Algorithm: sha256WithRSAEncryption
        Issuer: CN=Yubico U2F Root CA Serial 457200631
        Validity
            Not Before: Aug  1 00:00:00 2014 GMT
            Not After : Sep  4 00:00:00 2050 GMT
        Subject: CN=Yubico PIV Attestation

[ericchiang]

Thanks! Left a comment

[ericchiang]

nit: if this option is provided, don't modify it.

[oreparaz]

addressed, thanks. now if the option is provided, it is used as is

[ericchiang]

The x509 package actually already has builtin support for validating a certificate against multiple roots. I think it'd be a good simplification to see if the upstream methods work. Does using a x509.CertPool instead of a x509.Certificate correctly validate? If so can we update the code to be something like:

o := x509.VerifyOptions{KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}}
o.Roots = v.Roots
if o.Roots != nil {
    cas, err := yubicoRoots()
    if err != nil {
        // ...
    }
    o.Roots = cas
}

o.Intermediates := x509.NewCertPool()
o.Intermediates.AddCert(attestationCert)

if err := slotCert.Verify(o); err != nil {
    // ...
}
return parseAttestation(slotCert)

I'm also happy to send that myself.

[jalseth]

Yes, the x509.Certificate.Verify() function returns one or more verified certificate chains given a set of roots and intermediates and would simplify this. In this use case, the returned chains can be ignored as we only care that there isn't a verification error.

[oreparaz]

I think this is a great idea. We shouldn't be duplicating validation logic that already lives in x509. I've added a change in 87636ea to use CertPool and x509.Certificate.Verify(). It works after fixing two wrinkles:

1. the U2F root cert has an invalid pathlen x509 basic constraint of 0. as per RFC 5280 this means that no intermediate cert is allowed in the validation path. this isn't the first time this happens with yubico CAs, see https://labanskoller.se/blog/2019/12/30/pki-is-hard-how-yubico-trusted-openssl-and-got-it-wrong/ for a similar bug in another yubico root cert. I work around this by setting pathlen to 1 in the root cert.
2. the device attestation certs from the lucky batch do not encode X509v3 Basic Constraints. this makes x509.Certificate.Verify() fail. We need to set this constraint in the device cert.

Overall I'm not too surprised of these wrinkles --after all, x509.Certificate.Verify() performs a more strict validation conforming to RFC 5280. I know this isn't pretty, but we're fixing a hardware bug after all, and hardware bugs are never pretty to fix 🙂

[ericchiang]

Some nits, but overall lgtm.

Please update and squash your commits (so there's only one commit) and I'll merge. Thanks for your work here!

[oreparaz]

Address your comments and squashed. Thanks for the detailed review!