Update verification results printing #9937

malancas · 2024-11-19T21:50:24Z

Improve how we filter attestations shown as verification results to the user cc #9850

Signed-off-by: Meredith Lancaster <malancas@github.com>

phillmv

I still need to read over the tests but here is a prelim comment

pkg/cmd/attestation/verification/extensions.go

Signed-off-by: Meredith Lancaster <malancas@github.com>

phillmv

I approve of the shape of this PR and I have only not clicked Approve because I think we should improve some comments (see my feedback)

pkg/cmd/attestation/verify/attestation_integration_test.go

phillmv · 2024-11-21T21:53:11Z

pkg/cmd/attestation/verify/attestation_integration_test.go

+		results, errMsg, err := verifyAttestations(*a, attestations, mockSgVerifier, ec)
+		require.NoError(t, err)
+		require.Zero(t, errMsg)
+		require.Len(t, results, 2)


ideally i'd like to check that the rwfResult is specifically the one being excluded. can we do an array comparison for sgjAttestation[0] and sgjAttestation[1]?

phillmv · 2024-11-21T21:56:18Z

pkg/cmd/attestation/verification/mock_verifier.go

+	return &MockSigstoreVerifier{t, mockResults}
+}
+
+func NewDefaultMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier {


food for thought / not high priority suggestion: considering the nature of this change i'd invert this: NewMockSigstoreVerifierWithMockResults so we don't modify every other usage of it / i feel like it's harder to reason what the Default Mock is.

Agreed, we can definitely iterate on this one

phillmv · 2024-11-21T21:58:48Z

pkg/cmd/attestation/verification/extensions.go

-			// if at least one attestation is verified, we're good as verification
-			// is defined as successful if at least one attestation is verified
-			return nil
+		if err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec.Certificate); err != nil {


yesterday we had a conversation where we realized it's not obvious WHY we verifyCertExtensions separately from the CertificateIdentity provided in sigstore-go (and frankly, maybe we should just upstream how we've done it here) and

after some mild effort,

we discovered it's so we can support case insensitivity around repo & owner names.

given that conversation let's add a wee comment to func verifyCertExtensiosn denoting that - "this func exists so we can do case insensitive comparisons"

Co-authored-by: Phill MV <phillmv@github.com>

Signed-off-by: Meredith Lancaster <malancas@github.com>

cliAutomation · 2024-11-21T22:31:48Z

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.

Signed-off-by: Meredith Lancaster <malancas@github.com>

phillmv

Nice!

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.62.0` -> `v2.63.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.63.0`](https://github.com/cli/cli/releases/tag/v2.63.0): GitHub CLI 2.63.0 [Compare Source](cli/cli@v2.62.0...v2.63.0) #### What's Changed - Support bare repo creation by [@williammartin](https://github.com/williammartin) in cli/cli#9905 - Refactor the `getAttestations` functions by [@malancas](https://github.com/malancas) in cli/cli#9892 - Added a section on manual verification of the relases. by [@kommendorkapten](https://github.com/kommendorkapten) in cli/cli#9936 - Adding option to return `baseRefOid` in `pr view` by [@daliusd](https://github.com/daliusd) in cli/cli#9938 - Update verification results printing by [@malancas](https://github.com/malancas) in cli/cli#9937 - Fix some multiline command documentation to use `heredoc` strings by [@BagToad](https://github.com/BagToad) in cli/cli#9948 - Print friendly error when `release create` fails due to missing `workflow` OAuth scope by [@BagToad](https://github.com/BagToad) in cli/cli#9791 **Full Changelog**: cli/cli@v2.62.0...v2.63.0 #### Security - A security vulnerability has been identified in the GitHub CLI that could leak authentication tokens when cloning repositories containing git submodules hosted outside of GitHub.com and ghe.com. For more information, see GHSA-jwcm-9g39-pmcw #### New Contributors - [@daliusd](https://github.com/daliusd) made their first contribution in cli/cli#9938 </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 3 commits November 19, 2024 14:38

verify cert extensions function should return filtered result list

f48e6b5

Signed-off-by: Meredith Lancaster <malancas@github.com>

wrap sigstore and cert ext verification into a single function

b5788f2

Signed-off-by: Meredith Lancaster <malancas@github.com>

clean up

3e6861e

Signed-off-by: Meredith Lancaster <malancas@github.com>

phillmv reviewed Nov 20, 2024

View reviewed changes

pkg/cmd/attestation/verification/extensions.go Outdated Show resolved Hide resolved

pkg/cmd/attestation/verification/extensions.go Outdated Show resolved Hide resolved

pkg/cmd/attestation/verification/extensions.go Outdated Show resolved Hide resolved

malancas and others added 9 commits November 20, 2024 12:46

update test

4671b8d

Signed-off-by: Meredith Lancaster <malancas@github.com>

update test

ff8844a

Signed-off-by: Meredith Lancaster <malancas@github.com>

naming

5601149

Signed-off-by: Meredith Lancaster <malancas@github.com>

update test with new test bundle

19afe45

Signed-off-by: Meredith Lancaster <malancas@github.com>

add more testing testing fixtures

4d277df

Signed-off-by: Meredith Lancaster <malancas@github.com>

undo change

7a271b0

Signed-off-by: Meredith Lancaster <malancas@github.com>

remove unused test file

28565dc

Signed-off-by: Meredith Lancaster <malancas@github.com>

pr feedback

2d41225

Signed-off-by: Meredith Lancaster <malancas@github.com>

Merge branch 'trunk' into verify-result-processing

27a268c

phillmv reviewed Nov 21, 2024

View reviewed changes

malancas and others added 2 commits November 21, 2024 15:20

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

fed3c81

Co-authored-by: Phill MV <phillmv@github.com>

pr feedback

0fd09eb

Signed-off-by: Meredith Lancaster <malancas@github.com>

malancas marked this pull request as ready for review November 21, 2024 22:31

malancas requested a review from a team as a code owner November 21, 2024 22:31

cliAutomation added the external pull request originating outside of the CLI core team label Nov 21, 2024

pr feedback

f92d703

Signed-off-by: Meredith Lancaster <malancas@github.com>

phillmv approved these changes Nov 22, 2024

View reviewed changes

phillmv merged commit f84c1c6 into cli:trunk Nov 22, 2024

malancas deleted the verify-result-processing branch December 3, 2024 20:32

falcorocks mentioned this pull request Dec 5, 2024

v2.63.0 seems to have broken attestation verify --bundle-from-oci flag #10017

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update verification results printing #9937

Update verification results printing #9937

Uh oh!

malancas commented Nov 19, 2024 •

edited by phillmv

Loading

Uh oh!

phillmv left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phillmv left a comment

Uh oh!

Uh oh!

phillmv Nov 21, 2024

Uh oh!

phillmv Nov 21, 2024

Uh oh!

malancas Nov 21, 2024

Uh oh!

phillmv Nov 21, 2024

Uh oh!

cliAutomation commented Nov 21, 2024

Uh oh!

phillmv left a comment

Uh oh!

Uh oh!

Update verification results printing #9937

Update verification results printing #9937

Uh oh!

Conversation

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

Uh oh!

Uh oh!

phillmv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phillmv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phillmv Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

phillmv Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

malancas Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

phillmv Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

cliAutomation commented Nov 21, 2024

Uh oh!

phillmv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

malancas commented Nov 19, 2024 •

edited by phillmv

Loading