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

CLI: Expose verification for GitHub-specific claims #322

Closed
tetsuo-cpp opened this issue Nov 29, 2022 · 11 comments · Fixed by #381
Closed

CLI: Expose verification for GitHub-specific claims #322

tetsuo-cpp opened this issue Nov 29, 2022 · 11 comments · Fixed by #381
Assignees
Labels
component:cli CLI components component:verification Core verification functionality enhancement New feature or request
Milestone

Comments

@tetsuo-cpp
Copy link
Contributor

Description

I noticed that Cosign has a series of flags for checking the signing certificate extensions. From the help text:

--certificate-github-workflow-name string                                                  contains the workflow claim from the GitHub OIDC Identity token that contains the name of the executed workflow.
--certificate-github-workflow-ref string                                                   contains the ref claim from the GitHub OIDC Identity token that contains the git ref that the workflow run was based upon.
--certificate-github-workflow-repository string                                            contains the repository claim from the GitHub OIDC Identity token that contains the repository that the workflow run was based upon
--certificate-github-workflow-sha string                                                   contains the sha claim from the GitHub OIDC Identity token that contains the commit SHA that the workflow run was based upon.
--certificate-github-workflow-trigger string                                               contains the event_name claim from the GitHub OIDC Identity token that contains the name of the event that triggered the workflow run

This should be a good fit for @woodruffw's policy API.

@tetsuo-cpp tetsuo-cpp added the enhancement New feature or request label Nov 29, 2022
@tetsuo-cpp tetsuo-cpp self-assigned this Nov 29, 2022
@woodruffw
Copy link
Member

Yep! These should map 1-1 to their equivalent policies.

Since cosign is currently in the process of reducing insecure/ineffective uses of their verification CLI, let's make sure we only allow these flags in ways that don't make policy verification effectively a no-op. For example, we should probably never allow --certificate-github-workflow-trigger on its own, since just about anybody can mint a cert with any particular workflow trigger embedded in its claims.

CC @znewman01 and @jku for thoughts (and particularly Zack to make sure we're aligned with cosign here).

@jku
Copy link
Member

jku commented Nov 30, 2022

I still think a usable CLI for this probably requires a service specific "endpoint" (whether it's a separate binary that just uses sigstore-python API or a mode in this cli app) that is somewhat opinionated about what it's doing. So I don't think adding options like this will make the generic CLI better for actually verifying GitHub releases. Alignment with cosign may be a good reason though.

A couple of reasons for my opinion:

  • A reasonable set of arguments for the verification ends up repeating many things and still requires the caller to construct the complex identity string
  • like @woodruffw says, it's easy to end up not verifying something you actually should have verified
  • what happens when sigstore becomes more popular -- do we add more --service-specific-claim flags as more services start attesting things?

A custom CLI endpoint on the other hand can A) make the inputs simpler and B) can require things that a "generic sigstore client" can not. So just as an example github-release-verify with just arguments <repo_name> <workflow_path> <tag> [<sha>] should work (for 99% of cases) and is probably simple enough to use without accidents. Now with the policy API that should be almost trivial to implement.

@woodruffw
Copy link
Member

Yeah, I tend to agree (and, given that cosign is undergoing significant changes at the same time, this may be an opportunity for sigstore-python to prove out a better CLI here).

This would be a little more invasive of a change, but I think turning every kind of verification operation into a discrete subcommand of verify might be nice, e.g.:

# equivalent to the current sigstore verify
sigstore verify identity --cert-identity ...

# enables the functionality discussed in this PR
sigstore verify github --workflow-sha ...

This allows us to keep the <noun> <verb> <noun> CLI pattern, while also letting us do some of the work for the user (e.g. sigstore verify github implies that the certificate's issuer can't be anything except the GitHub OIDC issuer, so we can pass that for them).

Thoughts?

@znewman01
Copy link

Alignment with cosign may be a good reason though.

I don't believe it is 😄 Cosign has a few design warts

(and, given that cosign is undergoing significant changes at the same time, this may be an opportunity for sigstore-python to prove out a better CLI here).

Please do so, sigstore-python is much less calcified and so can do a better job figuring out what this "should" look like. We can then slowly make Cosign look more like that over time if we like it.


Overall I like the plan of specific endpoints for common use cases. I wouldn't hate having an "advanced user escape hatch" for cases where sigstore-python hasn't caught up to some new application, but it doesn't need to be in a first draft.

@tetsuo-cpp
Copy link
Contributor Author

I'm beginning working on this. At this point, I'm liking @woodruffw's design as it leaves us room to extend with more "endpoints" by adding new subcommands after the verify, without introducing another CLI.

What do you think @di?

@di
Copy link
Member

di commented Dec 7, 2022

I like it! As long as we can keep the 'default' endpoint around for a while to maintain backwards compatibility.

@woodruffw
Copy link
Member

N.B.: It might be a little tricky to do "default" subcommands with argparse, so we could draft this out under a temporary subcommand (like sigstore verify-v2) until we're ready to stabilize it and swap the behavior out.

@di
Copy link
Member

di commented Dec 7, 2022

I guess what I'm concerned about is current 'bare' invocations of sigstore verify starting to fail on upgrade because they are missing the new 'endpoint' invocation pattern. I think we should do everything we can to avoid a breaking change like that.

@woodruffw woodruffw changed the title Support --certificate-github-workflow- flags CLI: Expose verification for GitHub-specific claims Dec 12, 2022
@woodruffw woodruffw added component:cli CLI components component:verification Core verification functionality labels Dec 12, 2022
@woodruffw woodruffw added this to the Stable release (1.0) milestone Dec 12, 2022
@woodruffw
Copy link
Member

Posting our latest resolution: we're going to see if we can glue together a compatibility layer with argparse here, so that both of the following patterns work:

sigstore verify [args ...]
sigstore verify identity [args ...]

...where the former is an alias/default for the latter.

@tetsuo-cpp tetsuo-cpp assigned woodruffw and unassigned tetsuo-cpp Dec 13, 2022
@jleightcap jleightcap self-assigned this Dec 13, 2022
@woodruffw
Copy link
Member

Doing this today.

@woodruffw
Copy link
Member

#379 has the first part of this: it enables sigstore verify [subcommand], where [subcommand] is optional. I'll do a follow-up PR for sigstore verify github after that's in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:cli CLI components component:verification Core verification functionality enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants