-
Notifications
You must be signed in to change notification settings - Fork 51
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: allow DSSE verification #1015
Conversation
Signed-off-by: William Woodruff <[email protected]>
sigstore/_cli.py
Outdated
if not stmt._matches_digest(digest): | ||
raise VerificationError( | ||
f"in-toto statement has no subject for digest {digest.digest.hex()}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging: this is consistent with what gh attestation verify
does, but I think it's potentially susceptible to misuse: in most contexts you also care about the name assigned to each subject, to prevent domain confusion attacks (e.g. where you request foo==1.2.3
, but the attacker gives you an old, vulnerable foo==1.0.0
with a valid signature for that older version).
The argument in this case for not doing that is that gh attestation verify
doesn't bother to do it either, plus the pre-existing hashedrekord
-based verification can't do it (since it was only over the input bytes, not the input name). So as-is there's no degradation in the security model, but it's worth highlighting and thinking about in the future.
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]> Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DSSE parts look fine. The verify github
change seems a bit fishy in that we no longer make sure the issuer is github -- maybe we should change policy.Identity
to accept identity: str | None, issuer: str
as args and then use that in _verify_github, or alternatively have a separate policy.Issuer
that only checks the issuer?
Signed-off-by: William Woodruff <[email protected]>
Ah yeah, good catch -- I think we can re-use the already existing |
Signed-off-by: William Woodruff <[email protected]>
sigstore/_cli.py
Outdated
# No matter what the user configures above, we require the OIDC issuer to | ||
# be GitHub Actions. We add this below the check above, since it doesn't | ||
# constitute a sufficient policy check on its own. | ||
inner_policies.append( | ||
policy.OIDCIssuer("https://token.actions.githubusercontent.com") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging: right now we allow any of the CLI options to be a "sufficient" option for verification with sigstore verify github
, which is potentially misleading or insufficient.
For example, a user could do sigstore verify github ... --name foo
, which would only check the workflow's name and allow any GitHub user/org and repo, which probably isn't what they intended.
Given that, should be restrict this to a few sufficient combinations? Some ideas:
--cert-identity
, as before, to bind the fulluser/repo
and workflow path--repository
, to bind theuser/repo
- Others, from the "V2" extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tweaked this with 41d3fc9 -- we now require --cert-identity
OR --repository
, which should be misuse-resistant without being burdensome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is more or less consistent with what gh attestation verify
does, although they also allow just --owner
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
41d3fc9 seems quite reasonable.
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for all the work on this. Reasoning for required options seems good to me.
Suggestions for docs (could file additional issues for these):
- The README examples of github verification are now not completely up to date: we should add a
--repository
example (maybe as the first one) - there is no "body text" in
sigstore verify github --help
: A description that mentions the required flags would make sense
inner_policies.append( | ||
policy.Identity( | ||
identity=args.cert_identity, | ||
# We always explicitly check the issuer below, so configuring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "below" here and "above" a few lines earlier are now a bit confusing
This allows for a very limited form of DSSE verification via the
sigstore
CLI. In particular, it allows a DSSE-containing bundle to be verified, but only if:This limited form is compatible with what GitHub Artifact Attestations is doing and, in particular, is compatible with how
gh attestation verify
behaves.WIP while I clean things up a little.Closes #999.
Closes #780.