search

sigstore / sigstore-conformance

Conformance testing for Sigstore clients
https://sigstore.dev
7 stars 10 forks source link

Fail when valid provided cert chain is not on log #119

Closed loosebazooka closed 6 months ago

loosebazooka commented 8 months ago

Ensure clients are reconstructing the rekor/log entry with the whole provided ceritificate rather than just the leaf. This fails because the entry is written with just the leaf but a validation event is requested with the full chain.

loosebazooka commented 8 months ago

@woodruffw I don't know enough about the python client, but I believe the reconstructed rekor object should not be found on rekor because the user provided a full chain (rather than just the leaf)?

haydentherapper commented 8 months ago

Oh this is a fun issue! It's currently undefined in the sigstore spec if a client must upload the entire chain or just the leaf. Cosign for example only uploads the leaf, to save on storage.

For certificate transparency, it mandates uploading the entire chain, though I think CT v2 (unimplemented) removed this to save on storage.

loosebazooka commented 8 months ago

I think clients should verify with what they were given though. So if they uploaded a full chain, that's what's used for client verification and that's what a user should provide to the client.

woodruffw commented 8 months ago

Yeah, I think this is a spec gap 🙂

Making sure I understand: the entry in question is the Rekor entry, correct? If so, I believe sigstore-python also only uploads the leaf:

https://github.com/sigstore/sigstore-python/blob/283588311a8670dc54a2e82551629c4b0a1bd01a/sigstore/sign.py#L213-L232

(I haven't fully thought this through yet, but is anything gained by having the client attempt to validate the Rekor entry with the "full" chain? My intuition is that chain construction ensures that this is sound regardless.)

loosebazooka commented 8 months ago

It's not necessarily the full chain, but I guess any provided cert chain during verification. If we're lopping off part of it would that lead to inconsistencies? How does one decided how much of the chain to remove?

I'm not exactly sure this provides any strong value except ensuring all clients behave the same way. But it could help in the future if there are untrusted intermediates?

I'm pretty bad at navigating python, but I believe with no provided rekor entry, reconstructing the UUID should lead to a rekor entry not found error (if we use the provided chain, instead of a leaf)

If I'm reading this correctly, it's validating some part of (https://github.com/sigstore/sigstore-python/blob/283588311a8670dc54a2e82551629c4b0a1bd01a/sigstore/verify/verifier.py#L240C31-L240C42)'s behavior

woodruffw commented 8 months ago

It's not necessarily the full chain, but I guess any provided cert chain during verification. If we're lopping off part of it would that lead to inconsistencies? How does one decided how much of the chain to remove?

Yeah, I think this needs to be clarified in the spec. One relevant part: the bundle specification at minimum says that the X509CertificateChain MUST NOT contain the root CA and SHOULD NOT contain intermediate CAs that are independently listed in the root of trust:

https://github.com/sigstore/protobuf-specs/blob/68b19c20c66ca941e6d19199688d97a2e3efa7ad/protos/sigstore_common.proto#L171-L191

This test case isn't formed as a bundle, but my (maybe incorrect) interpretation of that language was that clients only ever use and interchange the minimum viable chain state, which for the Public Good Instance is just the leaf signing certs themselves.

loosebazooka commented 8 months ago

This test case isn't formed as a bundle, but my (maybe incorrect) interpretation of that language was that clients only ever use and interchange the minimum viable chain state, which for the Public Good Instance is just the leaf signing certs themselves.

Yeah, I guess my confusion is how do we define the err or continue when someone provides a full chain to a verifier?

loosebazooka commented 8 months ago

So I think this test might need to become 2 tests:

  1. a bundle contains more than the leaf (although this might be solved by the update to protobuf-specs)
  2. a non-bundle input contains more than the leaf? (fail?)
woodruffw commented 8 months ago
  • a bundle contains more than the leaf (although this might be solved by the update to protobuf-specs)

Yep. Specifically, we'll probably need the following cases:

  1. For 0.1 and 0.2 bundles, a verifying client should accept whatever is in x509_certificate_chain but should always treat its members as untrusted (i.e., should never verify unless it can form a chain to the root of trust)
  2. For 0.3 bundles (pending https://github.com/sigstore/protobuf-specs/pull/191), a verifying client should reject anything that uses x509_certificate_chain instead of certificate with the PGI.
  3. When signing, regardless of bundle version, the resultant bundle MUST always have a single cert (the leaf) with the PGI.

2. a non-bundle input contains more than the leaf? (fail?)

I think this should be silently ignored? For better or worse, PEM parsers tend to be malleable in this way (they tend to scan the input looking for the first appropriate PEM tag, ignoring anything before and after). Producing an error here would probably require excessively specializing each implementation's PEM parsing/loading behavior.

(But I'm not confident about this.)

loosebazooka commented 6 months ago

I think this has been resolved by v0.3.0 spec and documentation