Support the protobuf bundle format in Cosign

haydentherapper commented 1 year ago

Background

Sigstore created a common format in sigstore/protobuf-specs for the output from Sigstore clients. sigstore-python, sigstore-java and sigstore-js currently support the bundle format. Golang currently does not support the bundle: sigstore-go is under active development, and Cosign has defined its own format.

Requirements

[ ] [P0] Output bundle for cosign sign-blob or cosign attest-blob
[ ] [P0] Read bundle for cosign verify-blob or cosign verify-blob-attestation
[ ] [P0] Verify conformance with other clients

Secondary goals

[ ] [P1] Persist bundle in OCI image manifest for cosign sign or cosign attest
[ ] [P1] Read bundle from OCI image manifest for cosign verify or cosign verify-attestation

Nice to haves

[ ] [P2] Separation of utilities in a dedicated package to make it easy to move code to sigstore-go
[ ] [P2] Support for uploading a bundle to OCI with cosign attach

Resources

Justification for sign-blob being a P0 and sign being a P1 is that OCI handles storage already, so it's less critical to have a new format.

wata727 commented 8 months ago

@haydentherapper I'm interested in working on this. Can we move this forward? If you don't have any other blockers, I can open a pull request after tweaking the patch https://github.com/sigstore/cosign/commit/d652a110510e7f5c12109a9385acc3620b48847b

A summary of this patch is:

Introduce pkg/cosign/bundle.SignedEntity instead of cosign.LocalSignedPayload.
- In order to cut out a dedicated package to sigstore-go that will generate a Sigstore bundle in the future, I thought we would need a struct that satisfies an interface like sigstore-go/pkg/verify.SignedEntity.
- A side effect of introducing this is that we can refactor to use this entity for creating a Cosign bundle.
Introduce pkg/bundle package.
- This package is intended to be moved to sigstore-go in the future and accepts the SignedEntity interface as an argument to Build method.
- This interface is almost the same as sigstore-go/pkg/verify.SignedEntity, but has been copied to avoid adding sigstore-go dependencies on Cosign. We can also add and replace the sigstore-go dependencies if you wish.
Add --sigstore-bundle flag to sign-blob command.

This patch is a work in progress, and if you agree with the direction, I could add support for attest-blob and verify, refactor redundant implementations, add tests, etc.

Having said that, I'm concerned about the following points at the moment:

Handling of the root certificate
- As I was proceeding with this implementation, I noticed https://github.com/sigstore/cosign/pull/3138#discussion_r1275534788. I did some research, but I couldn't figure out a pattern that would actually return anything other than a leaf certificate.
Public key hint
- In the current implementation, the Hint field is always empty when returning a public key. When I looked at the implementations of sigstore-js, sigstore-python, and sigstore-java, they didn't seem to generate hints either. Is this okay?
- According to the spec, it says "The format of the hint must be agreed upon out of band by the signer and the verifiers" but it is my understanding that there is no consensus across the Sigstore ecosystem yet.
Package interface compatibility
- I ended up choosing an interface centered around sigstore-go/pkg/verify.SignedEntity, but I'm not sure if this is appropriate. It may be arguable to add this as a public interface if there is not already enough discussion to design a suitable interface.

haydentherapper commented 7 months ago

Hey @wata727, sorry for the delay, I will reply to this next week! I do have lots of thoughts on this and really appreciate you taking a look!

cmurphy commented 4 months ago

@wata727 are you still working on this? Do you want to go ahead and submit your patch as a PR so it can be reviewed?

This interface is almost the same as sigstore-go/pkg/verify.SignedEntity, but has been copied to avoid adding sigstore-go dependencies on Cosign

My understanding is eventually cosign will leverage sigstore-go more and more, so it should be fine to add as a dependency.

wata727 commented 4 months ago

My original motivation was solved by GitHub introducing Artifact Attestations, so to be honest I don't have any motivation to move forward with this right now. So if you have your own plans, feel free to reject my patch.

However, if you think my patch could be useful in your work, I'd be happy to submit a PR.

haydentherapper commented 3 months ago

One other previous attempt: https://github.com/sigstore/cosign/pull/3138

steiza commented 3 months ago

@cmurphy I'm thinking about making a pull request for:

[P0] Output bundle for cosign sign-blob or cosign attest-blob

but I wanted to check with you first to see if you've already started on that. If not, I could break that item out into a separate issue and assign that to myself? Otherwise, if I should just be patient, that's fine too.

cmurphy commented 3 months ago

@steiza I have not started on this, go for it

steiza commented 3 months ago

FYI while https://github.com/sigstore/cosign/pull/3752 is waiting for reviews I am also starting to work on:

[P0] Read bundle for cosign verify-blob or cosign verify-blob-attestation

It's quite a bit harder! ~It appears that some of the arguments to cosign verify-blob like --certificate-identity, --certificate-oidc-issuer, --use-signed-timestamps, and several others, aren't actually wired up to the command.~

Additionally, I think we should add something like --trusted-root to make verification more user friendly, instead of requiring new bundle format users to specify several flags with trust material. Trusted roots are friendly to folks who may have private deployments, but we might also support TUF servers to make verification even more user friendly.

haydentherapper commented 3 months ago

@steiza https://github.com/sigstore/cosign/issues/3700 for tracking --trusted-root.

haydentherapper commented 3 months ago

For identity flags, are they under https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/verify.go#L113?

steiza commented 2 months ago

Okay, while waiting for reviews on #3796, I started working on conformance testing. For the most part it went smoothly, but there's one last test failure that might be a bit tricky to resolve.

I was assuming that for conformance testing, verify-bundle and sign-bundle would make use of the new codepaths that implement --new-bundle-format, and verify and sign would use the previous / existing cosign codepaths.

However, there are conformance tests that use --trusted-root with verify, which we did not implement support for in #3796. I think there's a couple of options here:

Go back and implement --trusted-root support in cosign, even when you aren't specifying --new-bundle-format
- Maybe we want to do this anyways? Do we want this support for the older codepaths of cosign?
Use the sigstore-go trick of taking the conformance materials provided by verify and wrap them in a bundle, and then always call cosign with --new-bundle-format when doing conformance tests
- This might make sense if we're moving to protobuf bundles sooner than later, and don't need conformance testing for the existing, older codepaths of cosign
... other ideas here!

codysoyland commented 2 months ago

Go back and implement --trusted-root support in cosign, even when you aren't specifying --new-bundle-format

I know we've agreed that we should tread lightly to avoid regression in cosign's existing verification logic as we add bundle support, but I do think it would be smart to begin adding support for the TrustedRoot for existing verification code paths with a plan to deprecate the existing TUF custom metadata format. We can't upgrade to go-tuf v2 until we do that, as go-tuf v2 is more opinionated about following spec and disallows iterating over targets as we are currently doing.

haydentherapper commented 2 months ago

I am very supportive of adding support for the trusted root format in Cosign. The only regression to consider is for private instances that have deployed their own TUF repos using the custom metadata for target selection. We can make any changes to the internals of Cosign for selection of which trusted root, eg changing fulcio root selection to use the TrustedRoot struct.

Adding support for specifying a trusted root via CLI should be straightforward. For fetching roots via TUF, I'd recommend we initialize two TUF clients, the new and old one, and merge targets fetched from the old TUF client (which would handle private deployments with the custom metadata) into the TrustedRoot struct. Then as part of Cosign V3, we'll drop support for the old TUF client.

How does that sound?

sigstore / cosign