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

Support the protobuf bundle format in Cosign #3139

Open
7 tasks
haydentherapper opened this issue Jul 26, 2023 · 13 comments
Open
7 tasks

Support the protobuf bundle format in Cosign #3139

haydentherapper opened this issue Jul 26, 2023 · 13 comments
Assignees
Labels
enhancement New feature or request pre-theseus

Comments

@haydentherapper
Copy link
Contributor

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

wata727 commented Feb 4, 2024

@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 d652a11

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 bundle support for sign-blob #3138 (comment). 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
Copy link
Contributor Author

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

cmurphy commented May 29, 2024

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

wata727 commented May 30, 2024

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

One other previous attempt: #3138

@steiza
Copy link
Member

steiza commented Jun 27, 2024

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

cmurphy commented Jun 27, 2024

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

@steiza
Copy link
Member

steiza commented Jul 8, 2024

FYI while #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
Copy link
Contributor Author

@steiza #3700 for tracking --trusted-root.

@haydentherapper
Copy link
Contributor Author

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

haydentherapper pushed a commit that referenced this issue Jul 23, 2024
This pull requests addresses the first part of #3139: adding protobuf bundle support for cosign sign-blob and cosign attest-blob.

Signed-off-by: Zach Steindler <steiza@github.com>
steiza added a commit to steiza/cosign that referenced this issue Jul 23, 2024
Part of sigstore#3139

Signed-off-by: Zach Steindler <steiza@github.com>
@steiza
Copy link
Member

steiza commented Jul 26, 2024

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

  • 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 pushed a commit that referenced this issue Jul 29, 2024
…#3796)

* Add new bundle support to `verify-blob` and `verify-blob-attestation`

Part of #3139

Signed-off-by: Zach Steindler <steiza@github.com>

* fix error message

Signed-off-by: Zach Steindler <steiza@github.com>

* Use sigstore-go v0.5.1 for cert issuer regex support

Signed-off-by: Zach Steindler <steiza@github.com>

* Use more specific `WithIntegratedTimestamps` with tlog verification

Signed-off-by: Zach Steindler <steiza@github.com>

---------

Signed-off-by: Zach Steindler <steiza@github.com>
@haydentherapper
Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pre-theseus
Projects
None yet
Development

No branches or pull requests

5 participants