`gh attestation verify` should only verify provenance attestations by default #9825

malancas · 2024-10-25T18:16:08Z

Currently gh attestations verify will attempt to verify all attestations by default unless one is specified with the --predicate-type flag. This updates the flag so it now defaults to the predicate attestation's predicate type. This means that unless specified with the flag, the command will only attempt to verify provenance attestations.

cc #9850

Signed-off-by: Meredith Lancaster <[email protected]>

…alancas/cli into verify-provenance-predicate-by-default

Signed-off-by: Meredith Lancaster <[email protected]>

cliAutomation · 2024-10-25T21:37:54Z

Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message.

phillmv

I approve of this PR but left one suggested change (the doc string comment) that should be addressed before merging 😉.

phillmv · 2024-10-28T18:44:52Z

pkg/cmd/attestation/verify/verify.go

-		}
-
-		attestations = filteredAttestations
+	filteredAttestations := verification.FilterAttestations(opts.PredicateType, attestations)


this is fine for now, but let's think about how to humanize this – i.e. distinguishing between "no attestations found" and "we only found the following predicate types", upon which we can offer some hints on how to tweak the command.

when we next chat, i have Some Ideas on how to organize this 🤔

(To be specific, maaaaaybe this step ought to be happening within the Verify function? or at least, as part of the policy struct that gets built right after.)

I agree this can be reorganized a bit. I was thinking of revisiting it in a follow up. I think I'm going to be doing reorganizing soon when I plan to print more information about the policy the command is verifying, was thinking of addressing this at that point. But I can address it sooner.

phillmv · 2024-10-28T18:48:38Z

pkg/cmd/attestation/verify/verify_test.go

+				Limit:            30,
 				OIDCIssuer:       verification.GitHubOIDCIssuer,
 				Owner:            "sigstore",
+				PredicateType:    verification.SLSAPredicateType,


do we have any tests that don't use provenance attestations?

The test case named "Use specified predicate type" in this table test uses the SPDX predicate type.

Is there a test too that fails when no predicate type attestations are returned?

I have a test that does not set the predicate type specifically to ensure it defaults to "https://slsa.dev/provenance/v1" since this is just testing that the flags are being set correctly. We don't currently have any code validating that all required options are set before moving on to the main verification function but we could add some to confirm that the SLSA predicate type, along with owner/repo and digest algorithm are always set as expected before verification.

pkg/cmd/attestation/verification/attestation.go

phillmv · 2024-10-28T19:00:01Z

pkg/cmd/attestation/verify/verify.go

 			with the provided artifact from the GitHub API. If you would prefer to verify
 			the artifact using attestations stored on disk (c.f. the %[1]sdownload%[1]s command),
-			provide a path to the %[1]s--bundle%[1]s flag.
+			provide a path to the %[1]s--bundle%[1]s flag. Additionally, the command


This help page is getting so wordy we'll soon have to provide a table of contents and some headers. Or link to a docs page.

Anyways here is a suggested rewording:

By default, the verify command will: - only verify provenance attestations - attempt to fetch relevant attestations via the GitHub API. To verify other types of attestations, use the predicate-type flag. To use your artifact's OCI registry instead of GitHub's API, use the --bundle-from-oci flag. For offline verification, using attestations stored on desk (c.f. the download command), provide a path to the --bundle flag.

(Like, re-reading the whole text, I'm like, we could stand to reword the section on specifying the owner. I'm thinking we need to call out that you're verifying the identity of the signer, and maybe describe the reusable workflow options there instead of at the bottom – but this is a problem for another PR)

Signed-off-by: Meredith Lancaster <[email protected]>

Co-authored-by: Phill MV <[email protected]>

Signed-off-by: Meredith Lancaster <[email protected]>

pkg/cmd/attestation/verify/verify.go

Co-authored-by: Phill MV <[email protected]>

Signed-off-by: Meredith Lancaster <[email protected]>

phillmv

i think we're good to go!

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.59.0` -> `v2.62.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.62.0`](https://github.com/cli/cli/releases/tag/v2.62.0): GitHub CLI 2.62.0 [Compare Source](cli/cli@v2.61.0...v2.62.0) #### What's Changed - Update monotonic verification logic and testing by [@malancas](https://github.com/malancas) in cli/cli#9856 - Check extension for latest version when executed by [@andyfeller](https://github.com/andyfeller) in cli/cli#9866 - Shorten extension release checking from 3s to 1s by [@andyfeller](https://github.com/andyfeller) in cli/cli#9914 - Mention GitHub CLI team on discussion issues by [@andyfeller](https://github.com/andyfeller) in cli/cli#9920 **Full Changelog**: cli/cli@v2.61.0...v2.62.0 #### Security - A security vulnerability has been identified in GitHub CLI that could allow remote code execution (RCE) when users connect to a malicious Codespace SSH server and use the `gh codespace ssh` or `gh codespace logs` commands. For more information, see GHSA-p2h2-3vg9-4p87 #### GitHub CLI notifies users about latest extension upgrades Similar to the notification of latest `gh` releases, the `v2.62.0` version of GitHub CLI will notify users about latest extension upgrades when the extension is used: ```shell $ gh ado2gh ... A new release of ado2gh is available: 1.7.0 → 1.8.0 To upgrade, run: gh extension upgrade ado2gh --force https://github.com/github/gh-ado2gh ``` ##### Why does this matter? This removes a common pain point of extension authors as they have had to reverse engineer and implement a similar mechanism within their extensions directly. With this quality of life improvement, there are 2 big benefits: 1. Extension authors will hopefully see increased adoption of newer releases while having lower bar to maintaining their extensions. 2. GitHub CLI users will have greater awareness of new features, bug fixes, and security fixes to the extensions used. ##### What do you need to do? Extension authors should review their extensions and consider removing any custom logic previously implemented to notify users of new releases. ### [`v2.61.0`](https://github.com/cli/cli/releases/tag/v2.61.0): GitHub CLI 2.61.0 [Compare Source](cli/cli@v2.60.1...v2.61.0) #### Ensure users understand consequences before making repository visibility changes In `v2.61.0`, `gh repo edit` command has been enhanced to inform users about [consequences of changing visibility](https://gh.io/setting-repository-visibility) and ensure users are intentional before making irreversible changes: 1. Interactive `gh repo edit` visibility change requires confirmation when changing from `public`, `private`, or `internal` 2. Non-interactive `gh repo edit --visibility` change requires new `--accept-visibility-change-consequences` flag to confirm 3. New content to inform users of consequences - Incorporate [GitHub Docs content](https://gh.io/setting-repository-visibility) into help usage and interactive `gh repo edit` experience - Expanded help usage to call out most concerning consequences - Display repository star and watcher counts to understand impact before confirming #### What's Changed - Add acceptance test for `project` command by [@jtmcg](https://github.com/jtmcg) in cli/cli#9816 - Add comprehensive testscript for `gh ruleset` by [@andyfeller](https://github.com/andyfeller) in cli/cli#9815 - Add comprehensive testscript for gh ext commandset by [@andyfeller](https://github.com/andyfeller) in cli/cli#9810 - Require visibility confirmation in `gh repo edit` by [@andyfeller](https://github.com/andyfeller) in cli/cli#9845 - Clean up skipped online tests for `gh attestation verify` by [@malancas](https://github.com/malancas) in cli/cli#9838 - `gh attestation verify` should only verify provenance attestations by default by [@malancas](https://github.com/malancas) in cli/cli#9825 - Set `dnf5` commands as default by [@its-miroma](https://github.com/its-miroma) in cli/cli#9844 - Fix verbiage for deleting workflow runs by [@akx](https://github.com/akx) in cli/cli#9876 - Bump github.com/creack/pty from 1.1.23 to 1.1.24 by [@dependabot](https://github.com/dependabot) in cli/cli#9862 - `gh attestation verify` policy enforcement refactor by [@malancas](https://github.com/malancas) in cli/cli#9848 - Simplify Sigstore verification result handling in `gh attestation verify` by [@malancas](https://github.com/malancas) in cli/cli#9877 - Print empty array for `gh cache list` when `--json` is provided by [@williammartin](https://github.com/williammartin) in cli/cli#9883 - Bump actions/attest-build-provenance from 1.4.3 to 1.4.4 by [@dependabot](https://github.com/dependabot) in cli/cli#9884 - Create the automatic key when specified with -i by [@cmbrose](https://github.com/cmbrose) in cli/cli#9881 - fix: `gh pr create -w` ignore template flag by [@nilvng](https://github.com/nilvng) in cli/cli#9863 #### New Contributors - [@akx](https://github.com/akx) made their first contribution in cli/cli#9876 - [@nilvng](https://github.com/nilvng) made their first contribution in cli/cli#9863 **Full Changelog**: cli/cli@v2.60.1...v2.61.0 ### [`v2.60.1`](https://github.com/cli/cli/releases/tag/v2.60.1): GitHub CLI 2.60.1 [Compare Source](cli/cli@v2.60.0...v2.60.1) This is a small patch release to fix installing `gh` via `go install` which was broken with v2.60.0. #### What's Changed - Update testscript to use hard fork by [@williammartin](https://github.com/williammartin) in cli/cli#9821 **Full Changelog**: cli/cli@v2.60.0...v2.60.1 ### [`v2.60.0`](https://github.com/cli/cli/releases/tag/v2.60.0): GitHub CLI 2.60.0 [Compare Source](cli/cli@v2.59.0...v2.60.0) #### What's Changed - Add ArchivedAt field by [@tsukasaI](https://github.com/tsukasaI) in cli/cli#9790 - Include startedAt, completedAt in run steps data by [@andyfeller](https://github.com/andyfeller) in cli/cli#9774 - Adjust environment help for host and tokens by [@williammartin](https://github.com/williammartin) in cli/cli#9809 - Add handling of empty titles for Issues and MRs by [@jtmcg](https://github.com/jtmcg) in cli/cli#9701 - `LiveSigstoreVerifier.Verify` should error if no attestations are present by [@phillmv](https://github.com/phillmv) in cli/cli#9742 - `gh at verify` retries fetching attestations if it receives a 5xx by [@phillmv](https://github.com/phillmv) in cli/cli#9797 - Prevent local extension installations with invalid names and conflicts with core commands and other extensions by [@BagToad](https://github.com/BagToad) in cli/cli#9794 - Rewrite a sentence in CONTRIBUTING.md by [@muzimuzhi](https://github.com/muzimuzhi) in cli/cli#9772 - Use new GitHub preview terms in `working-with-us.md` by [@BagToad](https://github.com/BagToad) in cli/cli#9800 - Use new GitHub previews terminology in attestation commands' help docs by [@BagToad](https://github.com/BagToad) in cli/cli#9799 - Clarify in README that `gh` is supported on GitHub Enterprise Cloud by [@BagToad](https://github.com/BagToad) in cli/cli#9805 - build(deps): bump github.com/gabriel-vasile/mimetype from 1.4.5 to 1.4.6 by [@dependabot](https://github.com/dependabot) in cli/cli#9752 ##### Acceptance Test Changes - Add acceptance tests for `workflow`, `run`, and `cache` commands by [@BagToad](https://github.com/BagToad) in cli/cli#9766 - Add basic `api` acceptance tests by [@BagToad](https://github.com/BagToad) in cli/cli#9770 - Add acceptance tests for `release` commands by [@BagToad](https://github.com/BagToad) in cli/cli#9771 - Add acceptance tests for `org` and `ssh-key` commands by [@BagToad](https://github.com/BagToad) in cli/cli#9812 - Add acceptance tests for `gh auth` commands by [@jtmcg](https://github.com/jtmcg) in cli/cli#9787 - Add acceptance tests for `repo` commands by [@jtmcg](https://github.com/jtmcg) in cli/cli#9783 - Add acceptance tests for `search` command by [@BagToad](https://github.com/BagToad) in cli/cli#9786 - Add acceptance tests for `variable` commands by [@andyfeller](https://github.com/andyfeller) in cli/cli#978 - Add testscripts for gpg-key and label commands by [@williammartin](https://github.com/williammartin) in cli/cli#9811 - Use forked testscript for token redaction by [@williammartin](https://github.com/williammartin) in cli/cli#9804 - Add acceptance tests for `secret` commands by [@andyfeller](https://github.com/andyfeller) in cli/cli#9782 - Note token redaction in Acceptance test README by [@williammartin](https://github.com/williammartin) in cli/cli#9813 #### New Contributors - [@tsukasaI](https://github.com/tsukasaI) made their first contribution in cli/cli#9790 **Full Changelog**: cli/cli@v2.59.0...v2.60.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

malancas and others added 8 commits October 24, 2024 11:40

set provenance predicate type as default for predicate-type flag

4d57c79

Signed-off-by: Meredith Lancaster <[email protected]>

Merge branch 'trunk' into verify-provenance-predicate-by-default

c552224

update documentation to indclude predicate-type information

e8013c0

Signed-off-by: Meredith Lancaster <[email protected]>

Merge branch 'verify-provenance-predicate-by-default' of github.com:m…

84f3265

…alancas/cli into verify-provenance-predicate-by-default

predicate-type is no longer empty

fa65364

Signed-off-by: Meredith Lancaster <[email protected]>

message formatting

28fa42a

Signed-off-by: Meredith Lancaster <[email protected]>

update tests to include predicate type

cef335c

Signed-off-by: Meredith Lancaster <[email protected]>

add predicate type to integration tests

9ddaf13

Signed-off-by: Meredith Lancaster <[email protected]>

malancas marked this pull request as ready for review October 25, 2024 21:37

malancas requested a review from a team as a code owner October 25, 2024 21:37

cliAutomation added the external pull request originating outside of the CLI core team label Oct 25, 2024

phillmv reviewed Oct 28, 2024

View reviewed changes

malancas and others added 5 commits October 28, 2024 15:37

doc updates

f8f3502

Signed-off-by: Meredith Lancaster <[email protected]>

organize

7598c4a

Signed-off-by: Meredith Lancaster <[email protected]>

Update pkg/cmd/attestation/verification/attestation.go

efa6faf

Co-authored-by: Phill MV <[email protected]>

fix references

7bfddec

Signed-off-by: Meredith Lancaster <[email protected]>

update references

15d7e33

Signed-off-by: Meredith Lancaster <[email protected]>

phillmv reviewed Oct 29, 2024

View reviewed changes

pkg/cmd/attestation/verify/verify.go Outdated Show resolved Hide resolved

malancas and others added 2 commits October 29, 2024 11:53

Update pkg/cmd/attestation/verify/verify.go

2714508

Co-authored-by: Phill MV <[email protected]>

bold all flags in docs

384057c

Signed-off-by: Meredith Lancaster <[email protected]>

kommendorkapten approved these changes Oct 30, 2024

View reviewed changes

phillmv approved these changes Oct 30, 2024

View reviewed changes

malancas merged commit 1652051 into cli:trunk Oct 30, 2024

malancas deleted the verify-provenance-predicate-by-default branch May 28, 2025 22:56

gh attestation verify should only verify provenance attestations by default #9825

gh attestation verify should only verify provenance attestations by default #9825

Uh oh!

Conversation

malancas commented Oct 25, 2024 • edited by phillmv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cliAutomation commented Oct 25, 2024

Uh oh!

phillmv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malancas Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phillmv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`gh attestation verify` should only verify provenance attestations by default #9825

`gh attestation verify` should only verify provenance attestations by default #9825

malancas commented Oct 25, 2024 •

edited by phillmv

Loading

malancas Oct 29, 2024 •

edited

Loading