Improve signature and attestation detail output for sigstore bundles#3162
Open
simonbaird wants to merge 2 commits intoconforma:mainfrom
Open
Improve signature and attestation detail output for sigstore bundles#3162simonbaird wants to merge 2 commits intoconforma:mainfrom
simonbaird wants to merge 2 commits intoconforma:mainfrom
Conversation
Contributor
Review Summary by QodoHandle v3 sigstore bundle signatures and attestations filtering
WalkthroughsDescription• Handle v3 sigstore bundle signatures and attestations separately - Filter image signature attestations from provenance attestations - Extract signature details from bundle image signature attestations • Add helper functions for bundle signature processing - isImageSignatureAttestation() identifies signature type by predicate - extractSignaturesFromBundle() extracts signatures from DSSE envelopes • Improve code clarity with detailed comments explaining v3 bundle handling • Minor code style fix for octal file permission notation Diagramflowchart LR
A["v3 Bundle Signatures"] --> B["VerifyImageAttestations"]
B --> C["Filter by Predicate Type"]
C --> D["Image Signatures"]
C --> E["Provenance Attestations"]
D --> F["extractSignaturesFromBundle"]
E --> G["parseAttestationsFromBundles"]
F --> H["EntitySignature List"]
G --> I["Attestation List"]
File Changes1. internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
|
Contributor
Code Review by Qodo
1. Missing predicate constant
|
📝 WalkthroughWalkthroughValidateImageSignature gains bundle-aware processing: when bundles exist it enables NewBundleFormat and routes verification through VerifyImageAttestations (Cosign in‑toto verifier), extracts image signatures from bundles, and preserves the legacy non-bundle signature path. Changes
Sequence Diagram(s)sequenceDiagram
participant Validator as ValidateImageSignature
participant Verifier as VerifyImageAttestations
participant Cosign as Cosign In-toto Verifier
participant Bundle as Bundle Parser
participant SigBuilder as extractSignaturesFromBundle
Validator->>Verifier: Request verification (NewBundleFormat when bundles present)
Verifier->>Cosign: Verify attestations (in‑toto)
Cosign-->>Verifier: Verified attestations (DSSE envelopes)
Verifier->>Bundle: Provide bundle attestations
Bundle-->>Verifier: Filter attestations (skip image-signature vs others)
alt image-signature attestations present
Bundle->>SigBuilder: extractSignaturesFromBundle(DSSE envelope)
SigBuilder-->>Validator: EntitySignature objects
else other attestations
Verifier-->>Validator: Parsed attestations (provenance, etc.)
end
Validator->>Validator: Fall back to legacy non-bundle path if applicable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Member
Author
|
@SequeI FYI, wdyt? |
simonbaird
commented
Mar 4, 2026
| { | ||
| "keyid": "", | ||
| "sig": "" | ||
| "sig": "MEUCIBZc+dmgTn8SCx30h9yvCOjsBwj1+aZX0gW53c7TeyuSAiEAp4zWGNHMrjql9NFl/fCmFXnJkgDkOqbN5n7H7mw6aqI=" |
Member
Author
There was a problem hiding this comment.
See line 366 in this file for the equivalent output for v2 style sigs.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go (1)
164-205:⚠️ Potential issue | 🟠 MajorCache bundle detection once per call; avoid re-querying in the loop.
Line 164 already decides bundle mode, but Line 184 calls
a.hasBundles(ctx)again for every signature. SincehasBundlesperforms a remote lookup, this adds unnecessary network I/O and can flip behavior mid-loop on transient errors.Suggested fix
func (a *ApplicationSnapshotImage) ValidateImageSignature(ctx context.Context) error { opts := a.checkOpts client := oci.NewClient(ctx) + useBundles := a.hasBundles(ctx) var sigs []cosignOCI.Signature var err error - if a.hasBundles(ctx) { + if useBundles { opts.NewBundleFormat = true opts.ClaimVerifier = cosign.IntotoSubjectClaimVerifier sigs, _, err = client.VerifyImageAttestations(a.reference, &opts) } else { opts.ClaimVerifier = cosign.SimpleClaimVerifier sigs, _, err = client.VerifyImageSignatures(a.reference, &opts) } @@ for _, s := range sigs { - if a.hasBundles(ctx) { + if useBundles { ... } else { ... } }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/evaluation_target/application_snapshot_image/application_snapshot_image.go` around lines 164 - 205, Compute and cache the bundle-mode result once (e.g., isBundle := a.hasBundles(ctx)) before calling client.VerifyImageAttestations/VerifyImageSignatures and use that cached boolean inside the for _, s := range sigs loop instead of calling a.hasBundles(ctx) repeatedly; update the initial branch that chooses VerifyImageAttestations vs VerifyImageSignatures to use the cached value and remove the extra remote lookups in the loop (ensure any error handling for the initial hasBundles call is preserved).
🤖 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/evaluation_target/application_snapshot_image/application_snapshot_image.go`:
- Around line 534-553: isImageSignatureAttestation currently calls
attestation.ProvenanceFromBundlePayload which performs full signature/material
extraction and can fail even when predicateType indicates an image-signature;
change it to parse only the predicate type from the signature payload bytes
returned by sig.Payload() (e.g., decode the JSON payload and read the
"predicateType" field) and base the boolean result solely on that value; remove
or avoid calling attestation.ProvenanceFromBundlePayload in
isImageSignatureAttestation so metadata extraction errors do not cause false
negatives, but keep debug logging for any payload parse errors.
---
Outside diff comments:
In
`@internal/evaluation_target/application_snapshot_image/application_snapshot_image.go`:
- Around line 164-205: Compute and cache the bundle-mode result once (e.g.,
isBundle := a.hasBundles(ctx)) before calling
client.VerifyImageAttestations/VerifyImageSignatures and use that cached boolean
inside the for _, s := range sigs loop instead of calling a.hasBundles(ctx)
repeatedly; update the initial branch that chooses VerifyImageAttestations vs
VerifyImageSignatures to use the cached value and remove the extra remote
lookups in the loop (ensure any error handling for the initial hasBundles call
is preserved).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1eb6f5b7-2584-4200-8e07-a21f30521173
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
Show resolved
Hide resolved
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
Outdated
Show resolved
Hide resolved
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
Show resolved
Hide resolved
simonbaird
force-pushed
the
v3-sig-output
branch
2 times, most recently
from
c2b7dd7 to
a7ce061
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go (1)
541-549:⚠️ Potential issue | 🟠 MajorDecouple image-signature type detection from full attestation material extraction.
isImageSignatureAttestationcurrently depends onattestation.ProvenanceFromBundlePayload, so any extraction failure can misclassify a valid image-signature attestation. Parse onlypredicateTypefrom the DSSE payload for classification.Proposed fix
import ( "bytes" "context" + "encoding/base64" "encoding/json" @@ func isImageSignatureAttestation(sig cosignOCI.Signature) bool { payload, err := sig.Payload() if err != nil { log.Debugf("Cannot read signature payload: %v", err) return false } - att, err := attestation.ProvenanceFromBundlePayload(sig, payload) - if err != nil { - log.Debugf("Cannot parse bundle attestation: %v", err) - return false - } - - predicateType := att.PredicateType() + var envelope struct { + Payload string `json:"payload"` + } + if err := json.Unmarshal(payload, &envelope); err != nil || envelope.Payload == "" { + log.Debugf("Cannot parse bundle envelope payload: %v", err) + return false + } + + decoded, err := base64.StdEncoding.DecodeString(envelope.Payload) + if err != nil { + log.Debugf("Cannot decode bundle payload: %v", err) + return false + } + + var stmt struct { + PredicateType string `json:"predicateType"` + } + if err := json.Unmarshal(decoded, &stmt); err != nil { + log.Debugf("Cannot parse in-toto statement: %v", err) + return false + } + + predicateType := stmt.PredicateType // Image signature attestations use the cosign/sign predicate type isImageSig := predicateType == "https://sigstore.dev/cosign/sign/v1" log.Debugf("Attestation predicateType: %s, isImageSignature: %t", predicateType, isImageSig) return isImageSig }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/evaluation_target/application_snapshot_image/application_snapshot_image.go` around lines 541 - 549, The image-signature detection currently calls attestation.ProvenanceFromBundlePayload and uses att.PredicateType(), which misclassifies when full attestation parsing fails; change isImageSignatureAttestation logic to extract only the DSSE envelope's predicateType from the raw payload (do not call ProvenanceFromBundlePayload) and compare that string to "https://sigstore.dev/cosign/sign/v1" (adjust where isImageSig/isImageSignatureAttestation is computed), falling back to full parsing only when predicateType indicates a non-image attestation; this keeps attestation.ProvenanceFromBundlePayload for when you need full material but ensures classification is based solely on the DSSE predicateType.
🤖 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/evaluation_target/application_snapshot_image/application_snapshot_image.go`:
- Around line 184-205: Compute the bundle-mode once before iterating signatures
instead of calling a.hasBundles(ctx) inside the loop: call useBundles :=
a.hasBundles(ctx) (or if hasBundles returns (bool, error) call useBundles, err
:= a.hasBundles(ctx) and handle the error) and then replace the in-loop
conditionals that currently call a.hasBundles(ctx) with checks against
useBundles for both verification and per-signature handling (the blocks using
isImageSignatureAttestation(s), extractSignaturesFromBundle(s) and appending to
a.signatures should use useBundles). This avoids repeated remote lookups and
transient flips in behavior.
---
Duplicate comments:
In
`@internal/evaluation_target/application_snapshot_image/application_snapshot_image.go`:
- Around line 541-549: The image-signature detection currently calls
attestation.ProvenanceFromBundlePayload and uses att.PredicateType(), which
misclassifies when full attestation parsing fails; change
isImageSignatureAttestation logic to extract only the DSSE envelope's
predicateType from the raw payload (do not call ProvenanceFromBundlePayload) and
compare that string to "https://sigstore.dev/cosign/sign/v1" (adjust where
isImageSig/isImageSignatureAttestation is computed), falling back to full
parsing only when predicateType indicates a non-image attestation; this keeps
attestation.ProvenanceFromBundlePayload for when you need full material but
ensures classification is based solely on the DSSE predicateType.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb182018-0a76-4dc2-8b5e-bcfe5265c33f
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
Outdated
Show resolved
Hide resolved
Member
Author
|
There's some work to do here. Moving to draft. |
If we compare the output for a cosign v2 signed image versus a cosign v3 signed image, there is a lot of detail missing. I'm expecting we will be able to extract the signature info from the bundle, but for now Claude and I are having some difficulty. I think we'll need to gain a better understanding of the way the new sigstore bundles work and try again in the future. For now I think it's okay if we just show the sig field and add some todos in the code. I suspect no one is actually consuming this data, so it won't have an immediate impact if it's missing. Ref: https://issues.redhat.com/browse/EC-1690 Co-authored-by: Claude Code <noreply@anthropic.com>
In v3 signature bundles, the signature is actually just another type of attestation. So when we list attestations (in v3) we see both the sig and the provenance. Similarly, when we list the image signatures we see the image signature and the attestation signature. In the detailed output for ec validate image we show details about the signatures and attestations. This changes attempts to avoid listing provenance atts in the image sig list, and imag sigs in the provenance att list. As mentioned in the comments, there might be other ways to do this. Ref: https://issues.redhat.com/browse/EC-1690 Co-authored-by: Claude Code <noreply@anthropic.com>
simonbaird
force-pushed
the
v3-sig-output
branch
from
a7ce061 to
8e59a62
Compare
Contributor
Review Summary by QodoExtract and filter v3 sigstore bundle signatures and attestations
WalkthroughsDescription• Handle v3 sigstore bundle signature and attestation extraction separately • Filter image signature attestations from provenance attestations in v3 bundles • Extract signature details from bundle image signature attestations • Add helper functions for predicate type detection and filtering Diagramflowchart LR
A["v3 Bundle Processing"] --> B["ValidateImageSignature"]
A --> C["ValidateAttestationSignature"]
B --> D["Extract Image Sigs<br/>from Bundle"]
C --> E["Filter & Extract<br/>Provenance Atts"]
D --> F["isImageSignatureAttestation<br/>Check"]
E --> F
F --> G["Separate Output:<br/>Signatures vs Attestations"]
File Changes1. internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
|
Contributor
Code Review by Qodo
1. Bundle signature filtering broken
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go (1)
533-568:⚠️ Potential issue | 🔴 CriticalPredicate type extraction parses wrong JSON level.
The DSSE envelope structure returned by
sig.Payload()haspredicateTypeinside the base64-decodedpayloadfield, not at the top level. The current implementation will always return an empty string, causingisImageSignatureAttestationto returnfalsefor all attestations.See
parseAttestationsFromBundles(lines 310-321) for the correct structure handling.Proposed fix
+import "encoding/base64" + // extractPredicateType extracts the predicateType field from a JSON -// payload lazily, i.e. without unmarshalling all the other fields +// payload lazily from the base64-encoded in-toto statement inside the DSSE envelope func extractPredicateType(payload []byte) (string, error) { - var attestation struct { - PredicateType string `json:"predicateType"` + var envelope struct { + Payload string `json:"payload"` } - if err := json.Unmarshal(payload, &attestation); err != nil { + if err := json.Unmarshal(payload, &envelope); err != nil { return "", err } - return attestation.PredicateType, nil + if envelope.Payload == "" { + return "", nil + } + + decoded, err := base64.StdEncoding.DecodeString(envelope.Payload) + if err != nil { + return "", fmt.Errorf("decode base64 payload: %w", err) + } + + var stmt struct { + PredicateType string `json:"predicateType"` + } + if err := json.Unmarshal(decoded, &stmt); err != nil { + return "", fmt.Errorf("parse in-toto statement: %w", err) + } + return stmt.PredicateType, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/evaluation_target/application_snapshot_image/application_snapshot_image.go` around lines 533 - 568, The predicate extraction currently unmarshals the top-level DSSE envelope instead of the inner base64-encoded payload; update extractPredicateType to first unmarshal the DSSE envelope (matching the structure returned by sig.Payload(), e.g. a struct with a "payload" string), base64-decode that envelope.payload, then unmarshal the decoded bytes into a struct containing PredicateType (`json:"predicateType"`) and return it; keep hasPredicateType and isImageSignatureAttestation (which calls sig.Payload() and uses cosignSignPredicateType) unchanged aside from relying on the corrected extractPredicateType implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@internal/evaluation_target/application_snapshot_image/application_snapshot_image.go`:
- Around line 533-568: The predicate extraction currently unmarshals the
top-level DSSE envelope instead of the inner base64-encoded payload; update
extractPredicateType to first unmarshal the DSSE envelope (matching the
structure returned by sig.Payload(), e.g. a struct with a "payload" string),
base64-decode that envelope.payload, then unmarshal the decoded bytes into a
struct containing PredicateType (`json:"predicateType"`) and return it; keep
hasPredicateType and isImageSignatureAttestation (which calls sig.Payload() and
uses cosignSignPredicateType) unchanged aside from relying on the corrected
extractPredicateType implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59cb6a2c-0b6f-4d4f-bb84-1a109826fd61
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
Comment on lines
+192
to
+195
| if !isImageSignatureAttestation(s) { | ||
| log.Debugf("Skipping non-image signature attestation") | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
1. Bundle signature filtering broken 🐞 Bug ✓ Correctness
isImageSignatureAttestation() looks for predicateType directly in sig.Payload(), but for bundles sig.Payload() is a DSSE envelope and predicateType exists only in the base64-decoded embedded statement. This likely results in skipping all bundle image signatures (empty image.signatures) and incorrectly including cosign/sign/v1 entries in attestations.
Agent Prompt
### Issue description
`isImageSignatureAttestation()` currently checks for `predicateType` by JSON-unmarshalling the raw bytes returned from `sig.Payload()`. For bundle entries, `sig.Payload()` is a DSSE envelope JSON containing `payloadType` and a base64-encoded `payload` (the embedded in-toto statement). The DSSE envelope itself does **not** contain `predicateType`, so the check misclassifies bundle entries and breaks signature/attestation filtering.
### Issue Context
- Bundle parsing already assumes `sig.Payload()` is DSSE JSON and uses `attestation.ProvenanceFromBundlePayload()` which base64-decodes the embedded statement.
- Filtering should follow the same model: decode the DSSE envelope, base64-decode its `payload`, then read `predicateType` from the embedded statement.
### Fix Focus Areas
- internal/evaluation_target/application_snapshot_image/application_snapshot_image.go[533-568]
- internal/evaluation_target/application_snapshot_image/application_snapshot_image.go[184-206]
- internal/evaluation_target/application_snapshot_image/application_snapshot_image.go[289-304]
- internal/evaluation_target/application_snapshot_image/application_snapshot_image_test.go[1046-1078]
### Suggested approach
1. Replace `extractPredicateType(payload []byte)` usage for bundle filtering with logic that:
- Unmarshals `sig.Payload()` into `cosign.AttestationPayload` (or DSSE envelope struct).
- Base64-decodes `payload.PayLoad`.
- Extracts `predicateType` from the decoded embedded JSON (your existing `extractPredicateType` can be used *on the decoded payload bytes*).
2. If the DSSE unmarshal/base64 decode fails, return false and emit a debug log that indicates the payload isn’t a DSSE envelope / can’t be decoded.
3. Add a unit test ensuring `isImageSignatureAttestation()` returns true for a bundle DSSE signature whose embedded statement’s predicateType is `https://sigstore.dev/cosign/sign/v1` (you can reuse `createBundleDSSESignature`).
4. Optionally add a higher-level test to ensure ValidateImageSignature(bundle path) populates `a.signatures` (may require controllable bundle detection).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Ref: https://issues.redhat.com/browse/EC-1690