Sigstore verifier logic updates #9999

malancas · 2024-12-03T19:34:43Z

This just rearranges some logic for the sigstore verifier used by the gh attestation command set.

cc #9850

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

malancas · 2024-12-03T20:03:25Z

pkg/cmd/attestation/verification/sigstore.go


 type LiveSigstoreVerifier struct {
-	config SigstoreConfig
+	TrustedRoot  string


No need for these to be nested in another struct. We probably don't need the SigstoreConfig struct either since it only contains three fields of different types.

malancas · 2024-12-03T20:04:32Z

pkg/cmd/attestation/verification/sigstore.go

 }

-func (v *LiveSigstoreVerifier) chooseVerifier(b *bundle.Bundle) (*verify.SignedEntityVerifier, string, error) {
+func getBundleIssuer(b *bundle.Bundle) (string, error) {


This logic was originally inside of the chooseVerifier method. I moved it out so we could parse the bundle's issuer separately. This allowed me to clean up chooseVerifier a bit.

malancas · 2024-12-03T20:05:53Z

pkg/cmd/attestation/verification/sigstore.go

-					continue
-				}
+			// if the custom trusted root issuer is not set or doesn't match the given issuer, skip it
+			if len(lowestCert.Issuer.Organization) == 0 || lowestCert.Issuer.Organization[0] != issuer {


I flipped the logic checking if the cert's issuer matches the incoming issuer for an early exit in a given loop iteration.

williammartin · 2024-12-09T14:14:43Z

Is this waiting on something to be merged?

malancas · 2024-12-09T22:35:24Z

Is this waiting on something to be merged?

I'm going to open a pull request with some more integration tests across the command set, including some that will test the Sigstore verifier functionality. I'd like to merge that first so I can be more assured there are no regressions accidentally introduced in this PR. While we already have some integration tests for this functionality, I want to add some more that test the gh attestation verify command with various Sigstore verifier options in a script.

Once I open that PR, I'll add a link in this one saying it's blocked.

malancas · 2024-12-12T21:50:31Z

Is this waiting on something to be merged?

I'm going to open a pull request with some more integration tests across the command set, including some that will test the Sigstore verifier functionality. I'd like to merge that first so I can be more assured there are no regressions accidentally introduced in this PR. While we already have some integration tests for this functionality, I want to add some more that test the gh attestation verify command with various Sigstore verifier options in a script.

Once I open that PR, I'll add a link in this one saying it's blocked.

Actually, we have integration tests in pkg/cmd/attestation/verification/sigstore_integration_test.go that cover the public good, GitHub, and custom trust root use cases. So I'm going to merge this. I'll add more bash script integration tests in test/integration/attestation-cmd after #10051 is merged.

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.63.2` -> `v2.64.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.64.0`](https://github.com/cli/cli/releases/tag/v2.64.0): GitHub CLI 2.64.0 [Compare Source](cli/cli@v2.63.2...v2.64.0) #### What's Changed - docs: improve docs for browse command as of [#5352](cli/cli#5352) by [@ankddev](https://github.com/ankddev) in cli/cli#10025 - Open MR against gh-merge-base by [@heaths](https://github.com/heaths) in cli/cli#9712 - Add integration tests for `gh attestation verify` when the `bundle-from-oci` flag is specified by [@malancas](https://github.com/malancas) in cli/cli#10020 - `gh repo rename` help text clarifies new repo name should not include owner by [@BagToad](https://github.com/BagToad) in cli/cli#10044 - fix: list branches in square brackets in `gh run` and `gh codespace` by [@uday-rana](https://github.com/uday-rana) in cli/cli#10043 - Bump actions/attest-build-provenance from 1.4.4 to 2.1.0 by [@dependabot](https://github.com/dependabot) in cli/cli#10056 - Bump golang.org/x/crypto from 0.29.0 to 0.31.0 by [@dependabot](https://github.com/dependabot) in cli/cli#10070 - Improve documentation and error messaging for local extension installations without executables by [@BagToad](https://github.com/BagToad) in cli/cli#9933 - docs: better document auth scopes by [@ankddev](https://github.com/ankddev) in cli/cli#10026 - Sigstore verifier logic updates by [@malancas](https://github.com/malancas) in cli/cli#9999 - `gh pr merge --delete-branch` exits with error when merge requested via merge queue by [@BagToad](https://github.com/BagToad) in cli/cli#10074 - sundry `gh at inspect` improvements by [@phillmv](https://github.com/phillmv) in cli/cli#9954 - Support `pr view` for intra-org forks by [@williammartin](https://github.com/williammartin) in cli/cli#10078 - Print policy information before verifying attestations by [@malancas](https://github.com/malancas) in cli/cli#9891 - Improve error handling in apt setup script by [@jobegrabber](https://github.com/jobegrabber) in cli/cli#10055 - Use Windows compatible file name for downloaded attestations when running `gh attestation download` by [@malancas](https://github.com/malancas) in cli/cli#10051 - Bump github.com/cpuguy83/go-md2man/v2 from 2.0.5 to 2.0.6 by [@dependabot](https://github.com/dependabot) in cli/cli#10094 - Perform all `gh attestation verify` policy options configuration in the `newEnforcementCriteria()` function by [@malancas](https://github.com/malancas) in cli/cli#10012 #### New Contributors - [@ankddev](https://github.com/ankddev) made their first contribution in cli/cli#10025 - [@uday-rana](https://github.com/uday-rana) made their first contribution in cli/cli#10043 - [@jobegrabber](https://github.com/jobegrabber) made their first contribution in cli/cli#10055 **Full Changelog**: cli/cli@v2.63.2...v2.64.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 added 7 commits December 3, 2024 10:54

remove unneeded nesting

6a7243b

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

inverse logic for less nesting

1ffd225

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

more logic updating to remove nesting

d737d3b

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

get bundle issuer in another func

f0f86ec

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

remove duplicate err checking

2a6ee87

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

try switch statement

efca3bd

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

comment

3b043f6

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

malancas changed the title ~~Sigstore verifier cleanup~~ Sigstore verifier logic updates Dec 3, 2024

malancas commented Dec 3, 2024

View reviewed changes

malancas marked this pull request as ready for review December 3, 2024 20:07

malancas requested a review from a team as a code owner December 3, 2024 20:07

cliAutomation added the external pull request originating outside of the CLI core team label Dec 3, 2024

kommendorkapten approved these changes Dec 4, 2024

View reviewed changes

malancas merged commit 76ffe4f into cli:trunk Dec 12, 2024

malancas deleted the sigstore-verifier-cleanup branch December 12, 2024 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Sigstore verifier logic updates #9999

Sigstore verifier logic updates #9999

Uh oh!

malancas commented Dec 3, 2024 •

edited

Loading

Uh oh!

malancas Dec 3, 2024 •

edited

Loading

Uh oh!

malancas Dec 3, 2024

Uh oh!

malancas Dec 3, 2024

Uh oh!

williammartin commented Dec 9, 2024

Uh oh!

malancas commented Dec 9, 2024

Uh oh!

malancas commented Dec 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sigstore verifier logic updates #9999

Sigstore verifier logic updates #9999

Uh oh!

Conversation

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

Uh oh!

Uh oh!

malancas Dec 3, 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!

malancas Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

malancas Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

williammartin commented Dec 9, 2024

Uh oh!

malancas commented Dec 9, 2024

Uh oh!

malancas commented Dec 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

malancas commented Dec 3, 2024 •

edited

Loading

malancas Dec 3, 2024 •

edited

Loading