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

Discussion: remove non-bundle ("detached materials") tests from the conformance suite? #122

Open
woodruffw opened this issue Jan 24, 2024 · 9 comments
Labels
blocked question Further information is requested

Comments

@woodruffw
Copy link
Member

Opening this up for discussion, since other clients may have strong opinions here.

The context on my end: sigstore-python may begin moving towards a position of only supporting bundles on the CLI, if we can. The rationale for this is threefold:

  1. Bundles are standardized and included in the client spec, while "detached" materials are only mentioned in passing (and more in terms of what they provide, not their format). This makes it harder to define consistent client behavior around detached materials.
  2. "Detached" materials result in a much worse user experience: there are 3+ sidecar files for every input being verified instead of just one (foo.{crt,sig,rekor} instead of foo.sigstore.json), and handling them correctly results in a more complicated CLI than strictly necessary.
  3. "Detached" materials complicate the handling of Rekor entries: unlike bundles where the bundle version tells us whether an entry must be supplied, clients must individually determine how to handle the case of foo.rekor missing, present, etc.

If this rationale is sound and amenable to other clients, then I propose the following changes to the conformance suite:

  1. Existing "detached" tests should be converted or re-created as bundle tests, if they don't already have bundle equivalents;
  2. The CLI "protocol" should be amended to remove sign and verify entirely, leaving only sign-bundle and verify-bundle.

On the other hand, if this is too hasty, we could retain the current detached tests and use the "skip" machinery to ensure that non-supporting clients can continue to use the conformance suite.

Thoughts are greatly welcome here! I'm not 100% sure this is a good idea yet; on one hand it'll simplify conformance testing substantially but on the other it's a significant change 🙂

@steiza
Copy link
Member

steiza commented Jan 25, 2024

I'm in favor of making the conformance test suite be all bundle-based; I think bundles are the future direction we want to go in.

But I'll add another option to the mix - sigstore-python could only support bundles and still be able to handle the existing non-bundle conformance tests by using the detached materials to construct a bundle.

This is exactly what sigstore-go does, as it only understands how to verify bundles: see https://github.com/sigstore/sigstore-go/blob/2e5477d970784bb700b510402c1b1325cd2d47f6/cmd/conformance/main.go#L101-L152

EDIT: of course, this is for the verification side, not the signature side! I keep forgetting about the signing side of conformance testing, because sigstore-go doesn't yet sign things.

@woodruffw
Copy link
Member Author

But I'll add another option to the mix - sigstore-python could only support bundles and still be able to handle the existing non-bundle conformance tests by using the detached materials to construct a bundle.

True -- I can't remember why I excluded this as an option 😅. Thinking about it some more, this is probably the right way to go (for sigstore-python), since we're already making a lot of breaking changes in the 3.0 series.

So, to summarize:

  1. I propose we remove the detached testcases;
  2. sigstore-python will retain support for detached inputs on the CLI for the time being, at least through the 3.x series (and we may drop them in the 4.x series)

@haydentherapper
Copy link
Collaborator

One data point - If we want to add the conformance tests to Cosign before landing bundle support in Cosign, we would need to rely on the detached materials. I would like to have Cosign support bundles asap, so it's really just a question of priorities - would it be better to verify cosign is conformant asap, or enforce bundle support across clients?

@woodruffw
Copy link
Member Author

woodruffw commented Jan 30, 2024

Thanks for bringing that up! Do you know what the rough timeline for bundles in Cosign is?

(Either way, I'm good with keeping detached materials in the conformance suite for now -- we'll be able to accomodate them on the sigstore-python side for the foreseeable future, and I don't want to cause unnecessary pain for other clients 😅)

Edit: realized I didn't actually answer: yes, I think conformance support ASAP is more important 🙂

@haydentherapper
Copy link
Collaborator

Timeline is Soon™ 😄

Probably in the next few months? If conformance is straightforward, it might make sense to prioritize that over bundle support.

@woodruffw
Copy link
Member Author

Probably in the next few months? If conformance is straightforward, it might make sense to prioritize that over bundle support.

Sounds good to me! In that case, I'll mark this as blocked/frozen 🙂

@haydentherapper
Copy link
Collaborator

I think we can revisit this issue now, and move forward with deprecating the detached material tests.

@steiza I believe we are only relying on the bundle tests, correct? This comment notes we might want to use the detached tests in the future, but I think we can continue to use the bundle.

I was chatting with @loosebazooka about verifying short-lived certificates for the detached material test, and he mentioned to me that Java does a live lookup to fetch an SET given one is not present with only a certificate and signature. I assume other clients do the same, @woodruffw mentioned this for sigstore-python as well. Moving towards only verifying with bundles would let us fully remove online lookups from clients (or we could add SETs/RFC 3161 timestamps as an option for detached materials, but I'd rather just deprecate the detached path).

@loosebazooka
Copy link
Member

To clarify, it's not included in the normal flow. A user must use the helper function in https://github.com/sigstore/sigstore-java/blob/2c30236c1c23ad8f1140390067255b8bf98ca146/sigstore-java/src/main/java/dev/sigstore/rekor/client/RekorEntryFetcher.java to obtain the rekor entry and then create a bundle to pass into the verifier. The standard verifier only accepts bundles.

@haydentherapper
Copy link
Collaborator

I'm going down a related rabbit hole of seeing if we can change how sigstore-go is handling conformance testing for short-lived certs (sigstore/sigstore-go#277 (comment)), and I think we should either do the same as Java and craft a bundle with an SET fetched on demand, or remove detached material tests alltogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants