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

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

Closed
wants to merge 1 commit into from

Conversation

loosebazooka
Copy link
Member

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.

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.

Signed-off-by: Appu Goundan <[email protected]>
@loosebazooka
Copy link
Member Author

@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
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

loosebazooka commented Jan 5, 2024

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

loosebazooka commented Jan 10, 2024

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
Copy link
Member

  • 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 gen, protos: 0.3, single cert protobuf-specs#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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants