Add --skip-att-sig-check flag#3267
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:
📝 WalkthroughWalkthroughAdds a configurable ChangesAttestation signature verification skip flag
🎯 3 (Moderate) | ⏱️ ~25 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 |
|
Making it a draft because it has no Jira (yet anyway). |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
simonbaird
force-pushed
the
skip-att-sig-check
branch
from
8d30082 to
a77249d
Compare
|
This isn't in the sprint currently, so maybe review it only after you reviewed everything else. |
simonbaird
force-pushed
the
skip-att-sig-check
branch
from
a77249d to
767ffe0
Compare
![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/evaluation_target/application_snapshot_image/application_snapshot_image.go (1)
280-314: ⚡ Quick winDeduplicate attestation parsing to avoid behavior drift
Line 280 through Line 314 duplicates the non-bundle parsing path from
ValidateAttestationSignature. A future change in one path can silently desync the other and produce inconsistent attestation handling.🤖 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 `@internal/evaluation_target/application_snapshot_image/application_snapshot_image.go` around lines 280 - 314, This block duplicates the non-bundle attestation parsing logic found in ValidateAttestationSignature; extract the parsing/appending logic into a single helper (or call the existing ValidateAttestationSignature parsing routine) so both paths use the same code path: for each signature from layers.Get() call the shared function that runs ProvenanceFromSignature, switches on PredicateType and converts to SLSA v0.2/v1 or generic attestation via SLSAProvenanceFromSignature, SLSAProvenanceFromSignatureV1 and appends the result to a.attestations; remove the duplicated switch in the current loop and replace it with a call to that helper to avoid divergence.
🤖 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.
Inline comments:
In `@internal/image/validate.go`:
- Around line 87-90: The attestation fetch error is being suppressed in the
validation path; change the behavior in the block that calls
a.FetchAttestationsWithoutVerification(ctx) so that instead of only logging and
continuing (the log.Debugf("Failed to fetch attestations without verification:
%v", err) path) you return or propagate a wrapped error to abort this
component's validation; locate the call to
a.FetchAttestationsWithoutVerification and replace the silent-continue with an
immediate return of the error (or fmt.Errorf/Wrap with context) so callers see
the registry fetch failure.
---
Nitpick comments:
In
`@internal/evaluation_target/application_snapshot_image/application_snapshot_image.go`:
- Around line 280-314: This block duplicates the non-bundle attestation parsing
logic found in ValidateAttestationSignature; extract the parsing/appending logic
into a single helper (or call the existing ValidateAttestationSignature parsing
routine) so both paths use the same code path: for each signature from
layers.Get() call the shared function that runs ProvenanceFromSignature,
switches on PredicateType and converts to SLSA v0.2/v1 or generic attestation
via SLSAProvenanceFromSignature, SLSAProvenanceFromSignatureV1 and appends the
result to a.attestations; remove the duplicated switch in the current loop and
replace it with a call to that helper to avoid divergence.
🪄 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: 1da8673f-6dfe-44f1-a84d-a6396b58f8cb
📒 Files selected for processing (5)
cmd/validate/image.godocs/modules/ROOT/pages/ec_validate_image.adocinternal/evaluation_target/application_snapshot_image/application_snapshot_image.gointernal/image/validate.gointernal/policy/policy.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/evaluation_target/application_snapshot_image/application_snapshot_image_test.go (1)
1158-1231: ⚡ Quick winAdd malformed DSSE payload coverage for the unverified parsing path.
This test only covers happy paths. Since
--skip-att-sig-checkrelies on parsing unverified attestations, add at least one malformed DSSE/base64/statement case to lock down failure behavior and prevent regressions in this security-sensitive path.internal/image/validate_test.go (1)
746-750: ⚡ Quick winAssert
VerifyImageAttestationscall behavior for skip vs non-skip.The test checks output shaping, but not whether attestation verification was actually bypassed. Add explicit mock call assertions so the flag contract is enforced (called when false, not called when true).
Suggested assertion pattern
output, err := ValidateImage(ctx, comp, snap, updatedPolicy, evaluators, false) require.NoError(t, err) require.NotNil(t, output) + + if tt.skipAttSigCheck { + fakeClient.AssertNotCalled(t, "VerifyImageAttestations", refNoTag, mock.Anything) + } else { + fakeClient.AssertCalled(t, "VerifyImageAttestations", refNoTag, mock.Anything) + }Also applies to: 783-826
🤖 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 `@internal/image/validate_test.go` around lines 746 - 750, Add explicit mock expectations in the test around the VerifyImageAttestations (and similarly VerifyImageSignatures) calls so the skip flag contract is enforced: when the skip-attestations flag is false assert fakeClient.AssertCalled/AssertNumberOfCalls for VerifyImageAttestations(refNoTag, mock.Anything) (and for VerifyImageSignatures when relevant), and when the skip flag is true assert fakeClient.AssertNotCalled for VerifyImageAttestations; apply the same pattern for the other test variants referenced (around the blocks using fakeClient.On("Head", ref)... and the other cases in the 783-826 range) to ensure attestations are invoked only when not skipped.
🤖 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.
Inline comments:
In `@internal/image/validate_test.go`:
- Around line 712-718: This file fails gci import/format checks; run the
repository formatting/import toolchain to reorder and format imports and code in
the test file containing TestValidateImageSkipAttSigCheck and the tests struct,
then re-run gci/static checks to ensure the file is properly formatted before
committing.
---
Nitpick comments:
In `@internal/image/validate_test.go`:
- Around line 746-750: Add explicit mock expectations in the test around the
VerifyImageAttestations (and similarly VerifyImageSignatures) calls so the skip
flag contract is enforced: when the skip-attestations flag is false assert
fakeClient.AssertCalled/AssertNumberOfCalls for
VerifyImageAttestations(refNoTag, mock.Anything) (and for VerifyImageSignatures
when relevant), and when the skip flag is true assert fakeClient.AssertNotCalled
for VerifyImageAttestations; apply the same pattern for the other test variants
referenced (around the blocks using fakeClient.On("Head", ref)... and the other
cases in the 783-826 range) to ensure attestations are invoked only when not
skipped.
🪄 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: 8c50c02f-863b-4993-9b4a-6ed41ef86795
📒 Files selected for processing (4)
internal/evaluation_target/application_snapshot_image/application_snapshot_image.gointernal/evaluation_target/application_snapshot_image/application_snapshot_image_test.gointernal/image/validate_test.gointernal/policy/policy_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/evaluation_target/application_snapshot_image/application_snapshot_image.go
simonbaird
force-pushed
the
skip-att-sig-check
branch
2 times, most recently
from
87f5671 to
bc7ed38
Compare
|
Doing some extra work to get the Codecov check green. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/image/validate.go`:
- Line 89: The file uses fmt.Errorf in the call to
out.SetAttestationSignatureCheckFromError but does not import the fmt package;
add "fmt" to the import block in internal/image/validate.go so fmt.Errorf can be
resolved (locate the use at
out.SetAttestationSignatureCheckFromError(fmt.Errorf(...)) and update the
imports accordingly).
🪄 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: 5089e64a-4278-4072-8ad6-b64e2ae89fee
📒 Files selected for processing (1)
internal/image/validate.go
|
Got some changes in progress. Will move back to draft while I'm working on them. |
We're going this already for make feature_%. Use the same technique. Co-authored-by: Claude Code <noreply@anthropic.com>
simonbaird
force-pushed
the
skip-att-sig-check
branch
from
5b6e064 to
c1a78bc
Compare
|
Claude says all the suggestions have been addressed. It might be true.. 😅 |
simonbaird
force-pushed
the
skip-att-sig-check
branch
from
c1a78bc to
79ada71
Compare
ReviewFindingsLow
Info
Previous runReviewFindingsLow
Info
Previous run (2)ReviewFindingsMedium
Low
Info
Previous run (3)ReviewFindingsLow
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
ready-for-merge
simonbaird
force-pushed
the
skip-att-sig-check
branch
from
79ada71 to
e18ba05
Compare
fullsend-ai-review
Bot
added
requires-manual-review
|
Claude and I agree that the Covscan number is close enough. |
|
The [nil-deref] is being fixed. Here are my local agents take on the other items:
|
Implement --skip-att-sig-check flag to skip attestation signature validation checks, mirroring the existing --skip-image-sig-check flag. When enabled, attestation signature verification is bypassed during image validation. Why? Often I'm debugging/troubleshooting something and I get given an image ref to look at. We can use cosign download attestation to inspect the attestation, which is very useful, but if we want to try running Conforma against it, we must either guess, find, or ask to be provided with the public key. Sometimes that's not so difficult, but other times it may be very difficult or even impossible. (Consider for example if the image was built on an ephemeral cluster and the signing secret used is gone forever.) Now we can use --skip-image-sig-check and --skip-att-sig-check and carry on with the debugging. Note that we added the --skip-image-sig-check recently for other reasons, see https://redhat.atlassian.net/browse/EC-1647. The --skip-att-sig-check is a little more complicated because we needed to add a new function that can download the attestation without verifying it. The argument against this is that it may encourage less secure practices, but I would say it's acceptable because we're not changing the default behavior, which is always to require signature verification. Ref: https://redhat.atlassian.net/browse/EC-1815 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
simonbaird
force-pushed
the
skip-att-sig-check
branch
from
e18ba05 to
c3eb9ee
Compare
fullsend-ai-review
Bot
added
ready-for-merge
| } else { | ||
| out.SetImageSignatureCheckFromError(a.ValidateImageSignature(ctx)) | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] cryptographic-verification-bypass
The new --skip-att-sig-check flag allows fetching and processing attestations without cryptographic signature verification. Downstream consumers of the validation output have no programmatic way to distinguish verified from unverified attestations.
Suggested fix: Consider adding an output field indicating signature verification was skipped so policy rules can detect and handle unverified attestations.
| @@ -76,14 +77,27 @@ func ValidateImage(ctx context.Context, comp app.SnapshotComponent, snap *app.Sn | |||
|
|
|||
| // Handle image signature validation | |||
| if p.SkipImageSigCheck() { | |||
There was a problem hiding this comment.
[low] behavioral-change
The existing --skip-image-sig-check log message was changed from log.Debug to log.Warn, which is a behavioral change for current users who filter on log levels.
| // Extract the signatures from the attestations here in order to also validate that | ||
| // the signatures do exist in the expected format. | ||
| for _, sig := range layers { | ||
| return a.parseAttestationsFromSignatures(layers) |
There was a problem hiding this comment.
[low] test-adequacy
FetchAttestationsWithoutVerification has integration-level test coverage but no dedicated unit test exercising error paths and bundle detection independently.
Suggested fix: Add a unit test for FetchAttestationsWithoutVerification that mocks the OCI client to cover error paths and bundle detection.
fullsend-ai-review
Bot
added
ready-for-merge
2 participants
Implement --skip-att-sig-check flag to skip attestation signature validation checks, mirroring the existing --skip-image-sig-check flag. When enabled, attestation signature verification is bypassed during image validation.
Why? Often I'm debugging/troubleshooting something and I get given an image ref to look at. We can use cosign download attestation to inspect the attestation, which is very useful, but if we want to try running Conforma against it, we must either guess, find, or ask to be provided with the public key. Sometimes that's not so difficult, but other times it may be very difficult or even impossible. (Consider for example if the image was built on an ephemeral cluster and the signing secret used is gone forever.)
Now we can use --skip-image-sig-check and --skip-att-sig-check and carry on with the debugging. Note that we added the --skip-image-sig-check recently for other reasons, see https://redhat.atlassian.net/browse/EC-1647. The
--skip-att-sig-check is perhaps a little more complicated because we need to add a function that can download the attestation without verifying it.
The argument against this is that it may encourage less secure practices, but I would say it's acceptable because we're not changing the default behavior, which is always to require signature verification.
Ref: https://redhat.atlassian.net/browse/EC-1815