Add bundle auto-detection to Rego sigstore builtins#3269
Open
st3penta wants to merge 7 commits intoconforma:mainfrom
Open
Add bundle auto-detection to Rego sigstore builtins#3269st3penta wants to merge 7 commits intoconforma:mainfrom
st3penta wants to merge 7 commits intoconforma:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects OCI Sigstore bundles via a new Client.HasBundles call and branches verification into bundle-format (OCI referrer) or legacy paths; adds bundle-aware attestation parsing and protobuf bundle referrer creation, extends OCI client interface and tests, and updates acceptance BDD for bundle-format attestations. ChangesBundle Format Attestation Support
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant Eval as Evaluator
participant OCI as OCI Client
participant Verifier as Sigstore Verifier
participant Parser as Attestation Parser
CLI->>Eval: ValidateImage(ctx, image)
Eval->>OCI: HasBundles(ctx, imageRef)
alt Bundles present
OCI-->>Eval: true
Eval->>Verifier: VerifyImageAttestations (NewBundleFormat=true)
Verifier->>OCI: Fetch referrers / bundles
OCI-->>Verifier: bundles/referrers
Verifier->>Parser: attestationResult(attestations, useBundles=true)
Parser->>Parser: Parse DSSE envelope(s), validate payloadType
Parser-->>Verifier: provenance/attestation entries
Verifier-->>Eval: verification result
else No bundles or detection error
OCI-->>Eval: false / error
Eval->>Verifier: VerifyImageSignatures (legacy)
Verifier->>OCI: Fetch tag-based signatures
OCI-->>Verifier: signatures
Verifier->>Parser: attestationResult(signatures, useBundles=false)
Parser->>Parser: ProvenanceFromSignature
Parser-->>Verifier: provenance/attestation entries
Verifier-->>Eval: verification result
end
Eval-->>CLI: final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Contributor
Review Summary by QodoAdd bundle auto-detection to Rego sigstore builtins with fallback support
WalkthroughsDescription• Add HasBundles() method to oci.Client interface for testable bundle detection • Auto-detect Sigstore bundles in verify_attestation and verify_image Rego builtins - Uses bundle verification path with NewBundleFormat=true when bundles present - Falls back to legacy path when no bundles found or detection fails - Skips non-in-toto payload types silently • Refactor ApplicationSnapshotImage.hasBundles() to use new Client.HasBundles() method • Add comprehensive unit tests covering bundle and legacy verification paths • Add acceptance test scenario for bundle-format attestation verification Diagramflowchart LR
A["oci.Client Interface"] -->|HasBundles| B["Bundle Detection"]
B -->|Bundles Found| C["Bundle Verification Path"]
B -->|No Bundles/Error| D["Legacy Verification Path"]
C -->|verify_attestation| E["ProvenanceFromBundlePayload"]
C -->|verify_image| F["IntotoSubjectClaimVerifier"]
D -->|verify_attestation| G["ProvenanceFromSignature"]
D -->|verify_image| H["SimpleClaimVerifier"]
File Changes1. internal/utils/oci/client.go
|
Contributor
Code Review by Qodo
Context used✅ Tickets:
EC-1700 1. verify_image checks attestations
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+135
to
+152
| client := ecoci.NewClient(ctx) | ||
| useBundles, err := client.HasBundles(ctx, ref) | ||
| if err != nil { | ||
| logger.WithField("error", err).Debug("bundle detection failed, falling back to legacy path") | ||
| useBundles = false | ||
| } | ||
|
|
||
| var signatures []oci.Signature | ||
| if useBundles { | ||
| logger.Debug("bundles detected, using bundle verification path") | ||
| checkOpts.NewBundleFormat = true | ||
| checkOpts.ClaimVerifier = cosign.IntotoSubjectClaimVerifier | ||
| signatures, _, err = client.VerifyImageAttestations(ref, checkOpts) | ||
| } else { | ||
| checkOpts.ClaimVerifier = cosign.SimpleClaimVerifier | ||
| logger.Debug("verifying image signatures") | ||
| signatures, _, err = client.VerifyImageSignatures(ref, checkOpts) | ||
| } |
Contributor
There was a problem hiding this comment.
1. Verify_image checks attestations 🐞 Bug ⛨ Security
When bundles are detected, sigstoreVerifyImage() calls VerifyImageAttestations() with IntotoSubjectClaimVerifier and returns those results as image signatures. This can allow ec.sigstore.verify_image to succeed even if there are no image signatures, as long as attestations exist.
Agent Prompt
### Issue description
`ec.sigstore.verify_image` is intended to verify *image signatures*, but when `HasBundles` is true it verifies *attestations* instead (`VerifyImageAttestations`) and returns those as `signatures`.
### Issue Context
The acceptance helpers create distinct referrers for signatures vs attestations (different artifact types). Switching `verify_image` to verify attestations changes the security meaning of policies that rely on `verify_image` to assert an image was signed.
### Fix Focus Areas
- internal/rego/sigstore/sigstore.go[135-152]
### Suggested fix
- In the `useBundles` branch of `sigstoreVerifyImage`, keep `checkOpts.NewBundleFormat = true` but call `VerifyImageSignatures(ref, checkOpts)` (not `VerifyImageAttestations`).
- Keep `checkOpts.ClaimVerifier = cosign.SimpleClaimVerifier` for image signatures (the attestation-specific verifier should remain in `verify_attestation`).
- Update/extend unit tests so the bundle path asserts `VerifyImageSignatures` is used, and ensure the acceptance scenario still passes with referrer signatures present.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The Rego sigstore builtins need to detect whether an image has Sigstore bundles (OCI referrers) before verification, but cosign.GetBundles() is a package-level function that cannot be mocked through the existing FakeClient. This makes the bundle code path untestable in unit tests. Add HasBundles(ctx, ref) to the Client interface, wrapping the cosign.GetBundles() call and returning a boolean. Refactor ApplicationSnapshotImage.hasBundles() to delegate to the new method instead of calling cosign directly. Ref: https://issues.redhat.com/browse/EC-1700 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The verify_attestation Rego builtin only supported the legacy .att tag path for attestation verification. Images with attestations stored as OCI referrer bundles (cosign v3 --new-bundle-format) were invisible to Rego policies. Call HasBundles() before verification to auto-detect the format. When bundles are present, set NewBundleFormat=true and parse attestations via ProvenanceFromBundlePayload with DSSE envelope payloadType filtering. When no bundles are found or detection fails, fall back to the legacy path unchanged. Ref: https://issues.redhat.com/browse/EC-1700 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/rego/sigstore/sigstore_test.go (1)
557-560: ⚡ Quick winAssert legacy mode explicitly in fallback tests.
These fallback cases verify the call path, but they don’t assert that
checkOpts.NewBundleFormatis false. A regression that still enables bundle mode could slip through.Suggested test hardening
c.On( "VerifyImageAttestations", goodImage, mock.Anything, -).Return([]oci.Signature{goodSig}, false, nil) +).Return([]oci.Signature{goodSig}, false, nil).Run(func(args mock.Arguments) { + checkOpts := args.Get(1).(*cosign.CheckOpts) + require.False(t, checkOpts.NewBundleFormat) +}) c.On( "VerifyImageSignatures", goodImage, mock.Anything, -).Return([]oci.Signature{sig}, false, nil) +).Return([]oci.Signature{sig}, false, nil).Run(func(args mock.Arguments) { + checkOpts := args.Get(1).(*cosign.CheckOpts) + require.False(t, checkOpts.NewBundleFormat) +})As per coding guidelines "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 672-674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/rego/sigstore/sigstore_test.go` around lines 557 - 560, The fallback tests that mock HasBundles/VerifyImageAttestations need an explicit assertion that bundle mode was not used: capture the options passed into the mocked VerifyImageAttestations call (e.g., via mock.MatchedBy or the mock.Run callback on the VerifyImageAttestations expectation) and assert that the checkOpts.NewBundleFormat field is false; reference the mocked methods VerifyImageAttestations and HasBundles and the variables goodImage/goodSig so the assertion validates the fallback path rather than silently allowing bundle mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/utils/oci/client.go`:
- Around line 76-80: The HasBundles method is too coarse—change detection to be
artifact-type specific or add a fallback to the legacy verifier so mixed-format
images don't produce false negatives. Update the Client interface by replacing
or augmenting HasBundles(context.Context, name.Reference) (bool, error) with an
artifact-aware method (for example HasBundlesForType(ctx, ref, artifactType)
(bool, error) or HasBundles(ctx, ref) (map[string]bool, error)), implement the
corresponding logic in the OCI client, and then update callers
(internal/rego/sigstore/sigstore.go and
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go)
to query by artifact type (or consult the map) and, if the bundle path has no
matching artifact type, fall back to the legacy
VerifyImageSignatures/VerifyImageAttestations paths instead of assuming no
verification is possible.
---
Nitpick comments:
In `@internal/rego/sigstore/sigstore_test.go`:
- Around line 557-560: The fallback tests that mock
HasBundles/VerifyImageAttestations need an explicit assertion that bundle mode
was not used: capture the options passed into the mocked VerifyImageAttestations
call (e.g., via mock.MatchedBy or the mock.Run callback on the
VerifyImageAttestations expectation) and assert that the
checkOpts.NewBundleFormat field is false; reference the mocked methods
VerifyImageAttestations and HasBundles and the variables goodImage/goodSig so
the assertion validates the fallback path rather than silently allowing bundle
mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a2b8a858-e2fd-43e7-9d9f-6dc367b78bfd
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
acceptance/image/image.gofeatures/validate_image.featureinternal/evaluation_target/application_snapshot_image/application_snapshot_image.gointernal/evaluation_target/application_snapshot_image/application_snapshot_image_test.gointernal/image/validate_test.gointernal/rego/sigstore/sigstore.gointernal/rego/sigstore/sigstore_test.gointernal/utils/oci/client.gointernal/utils/oci/fake/client.go
Same bundle auto-detection as verify_attestation, applied to verify_image. When bundles are detected, switch from SimpleClaimVerifier + VerifyImageSignatures to IntotoSubjectClaimVerifier + VerifyImageAttestations with NewBundleFormat=true, matching the ApplicationSnapshotImage pattern. Legacy path unchanged. Ref: https://issues.redhat.com/browse/EC-1700 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a scenario that pushes attestations and signatures as OCI referrers (bundle format) and verifies them through the Rego sigstore builtins. Consolidate referrer signatures into the acceptance test output maps so snapshot template variables resolve for referrer-format entries. Ref: https://issues.redhat.com/browse/EC-1700 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/rego/sigstore/sigstore.go`:
- Around line 135-152: The repo-level HasBundles check (ecoci.NewClient ->
client.HasBundles) is being used as an exclusive switch causing attestations or
signatures to be skipped; change VerifyImageAttestations/VerifyImageSignatures
flow so you attempt the bundle verification (client.VerifyImageAttestations with
checkOpts.NewBundleFormat=true and
ClaimVerifier=cosign.IntotoSubjectClaimVerifier) and if it returns no matching
signatures for the specific ref/artifact, fall back to the legacy path (set
ClaimVerifier=cosign.SimpleClaimVerifier and call client.VerifyImageSignatures)
instead of relying solely on useBundles; ensure the logic inspects the returned
signatures slice/err from VerifyImageAttestations before deciding to skip
VerifyImageSignatures so artifact-specific bundles don’t suppress legacy
verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: b6926d90-9cea-472f-8ced-84fd70f94480
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
acceptance/image/image.gofeatures/validate_image.featureinternal/rego/sigstore/sigstore.gointernal/rego/sigstore/sigstore_test.go
✅ Files skipped from review due to trivial changes (1)
- features/validate_image.feature
Comment on lines
+135
to
+152
| client := ecoci.NewClient(ctx) | ||
| useBundles, err := client.HasBundles(ctx, ref) | ||
| if err != nil { | ||
| logger.WithField("error", err).Debug("bundle detection failed, falling back to legacy path") | ||
| useBundles = false | ||
| } | ||
|
|
||
| var signatures []oci.Signature | ||
| if useBundles { | ||
| logger.Debug("bundles detected, using bundle verification path") | ||
| checkOpts.NewBundleFormat = true | ||
| checkOpts.ClaimVerifier = cosign.IntotoSubjectClaimVerifier | ||
| signatures, _, err = client.VerifyImageAttestations(ref, checkOpts) | ||
| } else { | ||
| checkOpts.ClaimVerifier = cosign.SimpleClaimVerifier | ||
| logger.Debug("verifying image signatures") | ||
| signatures, _, err = client.VerifyImageSignatures(ref, checkOpts) | ||
| } |
There was a problem hiding this comment.
Make bundle selection artifact-specific before skipping the legacy verifier.
HasBundles(ctx, ref) is only a repo-level presence check, but these branches treat it as an exclusive switch for both builtins. In a mixed repo, a bundle attestation can force ec.sigstore.verify_image onto VerifyImageAttestations, and any unrelated bundle can keep ec.sigstore.verify_attestation from ever checking a valid legacy .att tag. That makes partial bundle rollouts return false negatives or validate the wrong artifact type.
Prefer trying the bundle path only when it yields matching results, then falling back to the legacy path, or extend detection so it distinguishes signature bundles from attestation bundles.
Also applies to: 220-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/rego/sigstore/sigstore.go` around lines 135 - 152, The repo-level
HasBundles check (ecoci.NewClient -> client.HasBundles) is being used as an
exclusive switch causing attestations or signatures to be skipped; change
VerifyImageAttestations/VerifyImageSignatures flow so you attempt the bundle
verification (client.VerifyImageAttestations with checkOpts.NewBundleFormat=true
and ClaimVerifier=cosign.IntotoSubjectClaimVerifier) and if it returns no
matching signatures for the specific ref/artifact, fall back to the legacy path
(set ClaimVerifier=cosign.SimpleClaimVerifier and call
client.VerifyImageSignatures) instead of relying solely on useBundles; ensure
the logic inspects the returned signatures slice/err from
VerifyImageAttestations before deciding to skip VerifyImageSignatures so
artifact-specific bundles don’t suppress legacy verification.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
acceptance/rekor/rekor.go (1)
185-196:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse base64, not hex, inside the checkpoint body
Checkpointnotes encode the Merkle root as base64 per RFC 6962 and C2SP checkpoint format, but these two blocks interpolaterootHashHex. That makes the acceptance stub non-conformant with the standard format and can break any bundle path that parses or validates the checkpoint body. KeepInclusionProof.RootHashas hex if needed, but build the checkpoint string from a base64-encodedrootHash.Suggested fix
rootHash := hasher.HashChildren(entryHash, entryUUID) rootHashHex := hex.EncodeToString(rootHash) + checkpointRootHash := base64.StdEncoding.EncodeToString(rootHash) - checkpoint := fmt.Sprintf("rekor.sigstore.dev - %s\n%d\n%s\n\n", logID, treeSize, rootHashHex) + checkpoint := fmt.Sprintf("rekor.sigstore.dev - %s\n%d\n%s\n\n", logID, treeSize, checkpointRootHash)Also applies to: 307-318
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@acceptance/rekor/rekor.go` around lines 185 - 196, The checkpoint string is built using rootHashHex but the C2SP/RFC6962 checkpoint body requires the Merkle root in base64; update the code that constructs checkpoint (the checkpoint variable) to use a base64-encoded root (e.g. base64.StdEncoding.EncodeToString(rootHashBytes)) instead of rootHashHex, while leaving InclusionProof.RootHash as the hex string if other code depends on it; adjust the creation of models.LogEntryAnon (and its Verification.InclusionProof.Checkpoint) to use that base64 root in the checkpoint string consistently (also apply the same change to the similar block around the other occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@acceptance/rekor/rekor.go`:
- Around line 185-196: The checkpoint string is built using rootHashHex but the
C2SP/RFC6962 checkpoint body requires the Merkle root in base64; update the code
that constructs checkpoint (the checkpoint variable) to use a base64-encoded
root (e.g. base64.StdEncoding.EncodeToString(rootHashBytes)) instead of
rootHashHex, while leaving InclusionProof.RootHash as the hex string if other
code depends on it; adjust the creation of models.LogEntryAnon (and its
Verification.InclusionProof.Checkpoint) to use that base64 root in the
checkpoint string consistently (also apply the same change to the similar block
around the other occurrence).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: c839f99f-b56e-4183-8d81-cc0b1fd96c05
📒 Files selected for processing (1)
acceptance/rekor/rekor.go
The stub Rekor functions overwrote Verification with only SignedEntryTimestamp, discarding the InclusionProof that computeLogEntry* had already set. MakeNewBundle requires InclusionProof for GenerateTransparencyLogEntry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The bundle-format attestation test was failing because HasBundles detection caused verify_image to take the bundle path. Switch to a legacy tag-based image signature and an attestation-only rego policy that only checks verify_attestation, avoiding the verify_image/bundle interaction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Both image signature and attestation are now attached as OCI referrers, matching realistic deployment scenarios. This tests both verify_image and verify_attestation through the bundle detection path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Rego sigstore builtins (verify_attestation and verify_image)
only supported the legacy .att tag path for signature and
attestation verification. Images with attestations stored as OCI
referrer bundles (cosign v3 --new-bundle-format) were invisible
to Rego policies.
Add HasBundles(ctx, ref) to the oci.Client interface so bundle
detection can be mocked in unit tests, then use it in both
builtins to auto-detect the format before verification. When
bundles are present, switch to the bundle verification path with
NewBundleFormat=true and the appropriate parser/verifier. When
no bundles are found or detection fails, fall back to the legacy
path unchanged. Refactor ApplicationSnapshotImage.hasBundles()
to delegate to the new interface method.
Add an acceptance test scenario that pushes attestations and
signatures as OCI referrers and verifies them through the Rego
builtins end-to-end.
Ref: https://issues.redhat.com/browse/EC-1700
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com