Improve signature and attestation detail output for v3 style sigstore bundles#3162
Improve signature and attestation detail output for v3 style sigstore bundles#3162simonbaird wants to merge 5 commits into
Conversation
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
|
Code Review by Qodo
1. Missing predicate constant
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughValidateImageSignature becomes bundle-aware: it detects Sigstore bundles, uses bundle-mode verification, extracts image signatures from DSSE envelopes into EntitySignature objects, skips image-signature attestations during attestation parsing, updates logging, adds tests, and tweaks input.json file mode. ChangesBundle-aware image signature validation
Sequence Diagram(s)sequenceDiagram
participant Validator as ValidateImageSignature
participant Verifier as VerifyImageAttestations
participant Cosign as Cosign In-toto Verifier
participant BundleParser as parseAttestationsFromBundles
participant SigExtractor as extractSignaturesFromBundle
Validator->>Verifier: request verification (opts.NewBundleFormat when bundles)
Verifier->>Cosign: verify attestations (in‑toto)
Cosign-->>Verifier: return verified DSSE envelopes
Verifier->>BundleParser: provide bundle entries
BundleParser-->>Verifier: filter entries (skip image-signature attestations)
alt image-signature attestations present
BundleParser->>SigExtractor: pass DSSE envelope
SigExtractor-->>Validator: EntitySignature objects (from DSSE signatures)
else other attestations
BundleParser-->>Validator: parsed attestations (provenance, etc.)
end
Validator->>Validator: fallback to legacy VerifyImageSignatures when not bundles
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 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)
Comment |
|
@SequeI FYI, wdyt? |
| { | ||
| "keyid": "", | ||
| "sig": "" | ||
| "sig": "MEUCIBZc+dmgTn8SCx30h9yvCOjsBwj1+aZX0gW53c7TeyuSAiEAp4zWGNHMrjql9NFl/fCmFXnJkgDkOqbN5n7H7mw6aqI=" |
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
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
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
|
There's some work to do here. Moving to draft. |
simonbaird
force-pushed
the
v3-sig-output
branch
from
a7ce061 to
8e59a62
Compare
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
|
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
st3penta
left a comment
st3penta
left a comment
There was a problem hiding this comment.
To be honest, this PR seems a bit experimental and with several TODOs left.
Maybe we should aim to gain a better understanding of the changes in cosign v3 before merging this?
It would be nice to have an acceptance test specifically for these changes.
Maybe add some references about the changes in the v3 in the PR description?
I also find the naming a bit confusing, but it might be me.
|
I'm planning to come back to this and fix it up a bit. Moving to draft in the meantime. |
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>
ReviewFindingsLow
Info
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsMedium
Low
Info
|
fullsend-ai-review
Bot
added
the
requires-manual-review
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
removed
the
requires-manual-review
fullsend-ai-review
Bot
added
the
ready-for-merge
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>
(Suggested by @fullsend-ai-review.) Ref: https://issues.redhat.com/browse/EC-1690 Co-authored-by: Claude Code <noreply@anthropic.com>
(Suggested by @fullsend-ai-review.) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
simonbaird
changed the title
fullsend-ai-review
Bot
added
requires-manual-review
Co-authored-by: Claude Code <noreply@anthropic.com>
|
I think it's probably good enough to merge now. |
fullsend-ai-review
Bot
added
ready-for-merge
2 participants
Ref: https://issues.redhat.com/browse/EC-1690