Set bundleVerified to true after Rekor verification (Resolves #3740)

[maxlambrecht]

Summary

This PR addresses an issue where the bundleVerified flag was not set to true after a successful online Rekor verification. The change ensures that bundleVerified accurately reflects the verification status when Rekor lookup is used.

Resolves #3740

Release Note

Documentation

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.83%. Comparing base (2ef6022) to head (b62e437).
Report is 140 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3745      +/-   ##
==========================================
- Coverage   40.10%   36.83%   -3.27%     
==========================================
  Files         155      200      +45     
  Lines       10044    12233    +2189     
==========================================
+ Hits         4028     4506     +478     
- Misses       5530     7177    +1647     
- Partials      486      550      +64     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[haydentherapper]

Thanks! Can you add a test for this? I think you just need to check what's returned by a code block like

cosign/pkg/cosign/verify_test.go

Lines 553 to 556 in 65969ae

	if _, err := VerifyImageSignature(context.TODO(), ociSig, v1.Hash{}, &CheckOpts{
	SigVerifier: sv,
	RekorClient: mClient,
	Identities: []Identity{{Subject: "[email protected]", Issuer: "oidc-issuer"}},

.

If adding this test isn't straightforward, that's fine, this is a simple enough change.

[maxlambrecht]

Thanks! Can you add a test for this? I think you just need to check what's returned by a code block like

cosign/pkg/cosign/verify_test.go

Lines 553 to 556 in 65969ae

	if _, err := VerifyImageSignature(context.TODO(), ociSig, v1.Hash{}, &CheckOpts{
	SigVerifier: sv,
	RekorClient: mClient,
	Identities: []Identity{{Subject: "[email protected]", Issuer: "oidc-issuer"}},

.

If adding this test isn't straightforward, that's fine, this is a simple enough change.

It wasn't straightforward, but I managed to add a comprehensive test covering several scenarios. I refactored the existing test, which only covered a failing validation, and combined it with other cases to ensure thorough coverage.

Let me know if you need any further adjustments or additional scenarios covered.

[haydentherapper]

Thank you, and thanks for the tests too!

[mtrmac]

I have no particular opinion on the desired semantics of the “bundleVerified” return value [my general position is that verification should be provided a policy and, apart from debugging-only output, return a single yes/no outcome, not any details to be further interpreted] — but AFAICS this value is consumed in

cosign/cmd/cosign/cli/verify/verify.go

Line 363 in 3d622d1

ui.Infof(ctx, " - Existence of the claims in the transparency log was verified offline")

, which claims the off-line validation has succeeded.

With this PR, bundleVerified is set to true if off-line validation did not succeed, but an on-line Rekor client check did.

I don’t know which part should be adjusted, but right now the codebase seems to be inconsistent.

[haydentherapper]

I agree, unfortunately we have inconsistencies in the codebase between what verification steps were taken and the debug output.

Even bundleVerified is misleading because we mean that we've verified Rekor, not the "bundle" aka the sig, cert and inclusion proof.

The plan is to clean all of this up with the adoption of sigstore-go as the internal API for Cosign.