Support intermediate certificates in bundle

[cmurphy]

Add support for processing and verifying a v0.3 bundle that contains a X509CertificateChain rather than a single X.509 certificate or public key.

Fixes #132

Summary

Release Note

Documentation

[steiza]

I don't totally understand the use-case of including intermediate certificates in your bundle verification materials, but maybe there's some private deployments that do so for some reason? It seems like you're better off with those intermediates in your trust root, for more control over revocation?

At any rate, I'm not opposed to these changes, but I do have some clarifying questions / naming suggestions.

[steiza]

Minor: maybe this should be GetIntermediateCerts(), as it doesn't return the whole chain?

[cmurphy]

Changed to GetIntermediates

[steiza]

I know there's a difference in protobundle between VerificationMaterial_X509CertificateChain and VerificationMaterial_Certificate, but if we had both use the CertificateChain type here, and had .GetCertificateChain() (or .GetIntermediateCerts() as I propose we call it) return an empty list if there's just a leaf certificate, I don't think we'd need CertificateChain and Certificate types?

@haydentherapper do you really want two separate types here, or have you changed your mind?

[haydentherapper]

I think that'd be fine to merge the types here.

[cmurphy]

Merged Certificate to CertificateChain

[steiza]

Minor: would GetLeafCertificate() be more clear?

[cmurphy]

done

[cmurphy]

I don't totally understand the use-case of including intermediate certificates in your bundle verification materials, but maybe there's some private deployments that do so for some reason?

@haydentherapper could speak more to this, but my understanding is it's wanted for some private deployments. But I don't know if anyone has specifically asked for it. If no one has actually said they need this, I wouldn't mind punting on it for a while.

[haydentherapper]

For the ask, yes, we had someone from nvidia asking about support for this in Cosign. Given it's already supported in Cosign, I'd love to see this supported here in sigstore-go as we migrate Cosign's internals over to sigstore-go.

There's maybe a bit of a meta question which is, do we want to try to add BYO-PKI features into the existing API, or create separate APIs that still live in sigstore-go but are intentionally designed to be more flexible and less bound to the Sigstore spec?

[steiza]

There's maybe a bit of a meta question which is, do we want to try to add BYO-PKI features into the existing API, or create separate APIs that still live in sigstore-go but are intentionally designed to be more flexible and less bound to the Sigstore spec?

Ideally there's one API, and the verification policies are flexible enough to cover BYO-PKI, although maybe we'll have to see how convoluted the BYO-PKI cases end up being.

But for these changes, I think that they can work with the existing API.

[steiza]

Looks good to me!

I had to check what order we expect GetIntermediates() to return certificates in, but it looks like this is compatible with our call to ctutil.

[codysoyland]

This seems like a step back from the decision in sigstore/protobuf-specs#190 / sigstore/protobuf-specs#191 that v0.3 bundles may only contain a leaf cert. Did I misunderstand that spec change? I do see the current client spec allows for intermediates in the verification materials, but I thought it was explicitly disallowed in a previous version.

There's maybe a bit of a meta question which is, do we want to try to add BYO-PKI features into the existing API, or create separate APIs that still live in sigstore-go but are intentionally designed to be more flexible and less bound to the Sigstore spec?

Is there an issue with providing a custom root.TrustedMaterial that contains additional intermediates? We could provide a helper function that accepts an existing root.TrustedMaterial and a list of additional certificates to augment it, for example:

func AugmentTrustedMaterialWithIntermediates(root.TrustedMaterial, []*x509.Certificate) root.TrustedMaterial

[steiza]

This seems like a step back from the decision in sigstore/protobuf-specs#190 / sigstore/protobuf-specs#191 that v0.3 bundles may only contain a leaf cert.

I think where this landed is that we want users of PGI to only have the leaf cert in the bundle, however there are private deployments that expect to have intermediate certificates in the bundle, even for bundle v0.3.

From https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_bundle.proto:

    // This allows key material to be conveyed in one of three forms:
    ...
    // 3. A single X.509 certificate, which MUST be a leaf certificate conveying
    //    the signing key.
    ...
    // When used in a `0.3` bundle with the PGI and "keyless" signing,
    // form (3) MUST be used.

My read of that is v0.3 bundles that are not using PGI may include intermediate certificates, which I think was the motivation for requesting this change.

We could bring back the check that v0.3 bundles only have leaf certs, but we'd need some way to distinguish between PGI and not - maybe checking the certificate subject?

[cmurphy]

Would the concerns about violating the spec be alleviated if the bundle wasn't providing the intermediates but something else was?

@steiza how were you envisioning the intermediates be supplied when you wrote #132?

[steiza]

@steiza how were you envisioning the intermediates be supplied when you wrote #132?

I created that issue because @haydentherapper asked me to 😆 #101 (comment)

I'm fine with either not having the check (like how the code currently is) or adding the check back in conditional on the certificate subject - but I'm open to other suggestions here!

[haydentherapper]

My read of that is v0.3 bundles that are not using PGI may include intermediate certificates, which I think was the motivation for requesting this change.
We could bring back the check that v0.3 bundles only have leaf certs, but we'd need some way to distinguish between PGI and not - maybe checking the certificate subject?

My two cents, the comments are meant as guidance to clients for what to implement. If a client wants to only support PGI, then it doesn't need to handle the non-PGI cases. The bundle being flexible allows it to be used more broadly for signing and verification - we actually got this feedback from chatting with a sigstore integrator that wanted to use their own PKI.

There is one attack that would be possible without a check - an intermediate CA is removed from the trust root due to compromise, with a malicious bundle provided that still references that intermediate. However, for the public good instance, this is a fairly unlikely scenario, as we would rotate the root and intermediate unless we were extremely confident that we could detect where the compromise occurred.

As a compromise, what do you think about a policy flag like TrustProvidedIntermediates, with a default value of false? For clients that only support the well-lit path for verification, no need to set it. For Cosign, when you set --private-infrastructure, we can set this flag as well.

[haydentherapper]

Also I'm good to merge this as-is and follow up with a PR for a flag if there's consensus.

[phillmv]

Drive by comment, I know this probably got discussed synchronously earlier today but I feel this contradicts the effort that went into adding the certificate field to the protobuf spec in the first place.

Q for whatever entity wants this feature: if they're BYO-PKI, presumably in a closed corporate environment, what is the blocker for distributing/trusting the intermediate certificate?

[phillmv]

(I don't really have a position on the security properties of certChain vs certificate, putting it behind a policy flag seems cromulent)

[codysoyland]

There is one attack that would be possible without a check - an intermediate CA is removed from the trust root due to compromise, with a malicious bundle provided that still references that intermediate. However, for the public good instance, this is a fairly unlikely scenario, as we would rotate the root and intermediate unless we were extremely confident that we could detect where the compromise occurred.

As a compromise, what do you think about a policy flag like TrustProvidedIntermediates, with a default value of false? For clients that only support the well-lit path for verification, no need to set it. For Cosign, when you set --private-infrastructure, we can set this flag as well.

This is my understanding of the threat as well, and I support adding a policy flag like this. 👍🏻

[kommendorkapten]

There is one attack that would be possible without a check - an intermediate CA is removed from the trust root due to compromise, with a malicious bundle provided that still references that intermediate. However, for the public good instance, this is a fairly unlikely scenario, as we would rotate the root and intermediate unless we were extremely confident that we could detect where the compromise occurred.

As a compromise, what do you think about a policy flag like TrustProvidedIntermediates, with a default value of false? For clients that only support the well-lit path for verification, no need to set it. For Cosign, when you set --private-infrastructure, we can set this flag as well.

I'm thinking more about the implementations here. As we don't offer any kind of certificate verification within sigstore-go (all certificates from the bundle or the trust root are deemed valid). I think this is the right choice right now:

• The PGI leaf certificates are short lived -- low risk of them being compromised
• The trust root is managed via TUF -- extremly fast failover due to compromised certificates (intermediates or root)

To support the other scenario, where untrusted intermediates are provided via the bundle for chain building. Instead of a flag/policy, how about a certificate verification function provided by the client? For the pgi use-case, this could be a simple function

func ValidateCertificate(c *x509.Certifiicate) (bool, error) {
        return true, nil
}

Buf for clients using intermediates in the bundle, they want to verify the certs during chain building, either by comparing against a CRL or relying on OCSP. (Note that such functions can be hard to implement securely, see CVE-2023-2422 as an example where client certificate validation can fail. I don't suggest we implement it, only provide such a hook.

If we offer such an interface during verification, we can make sure that even when untrusted intermediates are being provided, a client implementing that have the necessary hooks to perform certificate verification.

The other option is the force the client to perform out of band verification of all certificates, which works, but the UX is slightly worse as they would need to parse the bundle and the trust root. A callback function is nicer IMHO, and pushes the complexity out of sigstore-go into the client where it belongs.

edig: spelling error

[haydentherapper]

I'd be supportive of that design. It would make it easier to support requests like sigstore/cosign#2568, asking for CRLs.

For the interface, would we want to include intermediate and root certificates as well in the function parameters?

From Cosign's perspective, if we're only dealing with untrusted intermediates, these solutions - a policy flag or implementing a validate function - are roughly equivalent. If we go with a validate function, I'd suggest we put it in sigstore/sigstore as shared logic between Golang implementations.

[kommendorkapten]

Created this issue for discussion of certificate verification: #298