sigstore / sigstore-conformance

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

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

Open woodruffw opened 7 months ago

woodruffw commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

Timeline is Soonβ„’ πŸ˜„

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

woodruffw commented 7 months ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 (https://github.com/sigstore/sigstore-go/pull/277#issuecomment-2297508679), 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.