Export ParseAttestation and Verifier struct

[kilimnik]

I need to access the Attestation values without verifying the certificates. I also wanted to have the option to input my own root CA instead of the one defined in this library.

I'm not sure why these should not be exposed.

[ericchiang]

Can you provide some context for what you're trying to do here?

[kilimnik]

You can import a different CA to your yubikey. Then your verification function will not work anymore, because you force the CA to be the Yubico CA.

[ericchiang]

cool, thanks! Exporting the verifier is easy, but it'd need some comments. Something like:

// Verifier allows specifying options when verifying attestations produced by
// YubiKeys.
type Verifier struct {
    // Root certificates to use to validate challenges. If nil, this defaults to Yubico's
    // CA bundle.
    //
    // https://developers.yubico.com/PIV/Introduction/PIV_attestation.html
    // https://developers.yubico.com/PIV/Introduction/piv-attestation-ca.pem
    // https://developers.yubico.com/U2F/yubico-u2f-ca-certs.txt
    Roots *x509.CertPool
}

// Verify proves that a key was generated on a YubiKey.
//
// As opposed to the package level [Verify], it uses any options enabled on the [Verifier].
func (v *Verifier) Verify(attestationCert, slotCert *x509.Certificate) (*Attestation, error) {
    // ...
}

I generally don't want to provide any unsafe operations, such as parsing a cert without verifying it. See some of the discussion in: https://github.com/go-piv/piv-go/pull/83

Can we start by exporting Verifier then see if you need anything else related to parseAttestation?

[kilimnik]

Alright I get your point about security. I added the comments you proposed. I'm a bit unsure about the one talking about the default Yubico CA bundle. I think it should be mentioned, that the CA is included in this repository and is not pulled from Yubico, rather it is included in this repository. But maybe it's just me and it's completely fine.

[balena]

There is value in exporting the Verifier struct also when building e2e tests that get executed on the cloud, or where YubiKeys aren't available by design.

Just as an example, nitrite is a package built to allow remote attestation of AWS Nitro Enclaves and they allow customizing the Roots (check https://github.com/hf/nitrite/blob/main/nitrite.go#L61). This allows unit tests to be performed as long as they are capable to build attestation documents (though, of course, signed by a different root).

As security enforcement, you may suggest to users maintaining a hash of the Yubico root CA at their code to enforce checks at the initialization, just like they do here. This would avoid the possibility of simple tampering made at build time by replacing the Yubico keys using ldflags. Of course, the hash could be overridden too in the process, so it isn't a hard protection in any ways. It can be also verified with a second tool which keeps the hash and doesn't participate of the build... which is also subject to tampering, but story is left to the user to complete under their premises and threat model assumptions...

[ericchiang]

There is value in exporting the Verifier struct also when building e2e tests that get executed on the cloud, or where YubiKeys aren't available by design.

If you'd like a follow up, I'd also be happy to add a build tag that lets you get the verifier without requiring linking against smartcard libraries.

As security enforcement, you may suggest to users maintaining a hash of the Yubico root CA at their code to enforce checks at the initialization, just like they do here. This would avoid the possibility of simple tampering made at build time by replacing the Yubico keys using ldflags. Of course, the hash could be overridden too in the process, so it isn't a hard protection in any ways. It can be also verified with a second tool which keeps the hash and doesn't participate of the build... which is also subject to tampering, but story is left to the user to complete under their premises and threat model assumptions...

There's no way any Go package is going to effectively defend against build-time tampering. I'm against adding features to go-piv are for that purpose.

[balena]

There's no way any Go package is going to effectively defend against build-time tampering.

"Effectively" is tricky. There are runtime measures you can take that harden your solution, and which may be suitable for your threat model. While not enough for one, another might consider sufficient.

(...) adding features to go-piv are for that purpose.

I didn't support adding features, but suggesting, at the documentation maybe.