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

verify* subcommands should share signature verification flag parsing #2462

Open
kpk47 opened this issue Nov 16, 2022 · 5 comments
Open

verify* subcommands should share signature verification flag parsing #2462

kpk47 opened this issue Nov 16, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@kpk47
Copy link
Contributor

kpk47 commented Nov 16, 2022

Description

There's a lot of duplicated and almost-duplicated code in the verify, verify-blob, verify-attestation, and verify-blob-attestation subcommands. I wrote a short doc suggesting how to refactor them and would like feedback: https://docs.google.com/document/d/1OBg0OuoIDDv8Sy8bXNU9OnZvji82JhgyjYD2ykXhY4o/edit?usp=sharing&resourcekey=0-fg8AokOOQAtyKdpelRSeFA

@znewman01
Copy link
Contributor

Overall LGTM!

FWIW the intended outcome of the sigstore-go work (proposal), along with the protobuf-specs repo, is to remove a lot of redundancy. The plan is then that Cosign becomes a relatively thin wrapper.

I think the proposed doc is broadly consistent with this plan because it deals mostly with flag parsing, but it's worth keeping in mind for the purposes of future-proofing.

@asraa
Copy link
Contributor

asraa commented Nov 16, 2022

Nice! Yeah, maybe we can start with chunking out the smaller verification pieces in sigstore-go? e.g. verifying a cert against the cert related options?

FWIW I started a PR that combines all the verification code here: #2461
It's a little hard for me to manage since we're making a bunch of changes at the same time.

Basically, from that PR, the steps are all equivalent (they're numbered in the comments), except that the verification algorithm for blobs and attestations and the check claims (e.g. checking that the attestation subject digest matches the artifact) are different.

@asraa
Copy link
Contributor

asraa commented Nov 16, 2022

Would definitely love this in sigstore-go to start.

@lcarva
Copy link
Contributor

lcarva commented Jun 13, 2023

Is there agreement that this should go into sigstore-go from the start?

My only concern is that it doesn't look like cosign currently uses signstore-go. I'd be hesitant to add something there that will eventually get stale.

@znewman01
Copy link
Contributor

IIUC @asraa meant that "from day one, sigstore-go should be built this way" rather than "we should do this in sigstore-go before we implement it in Cosign"

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

No branches or pull requests

4 participants