Add --skip-att-sig-check flag#3267
Open
simonbaird wants to merge 2 commits into
Open
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR introduces a new 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 |
Member
Author
|
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.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
simonbaird
force-pushed
the
skip-att-sig-check
branch
from
8d30082 to
a77249d
Compare
Member
Author
|
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
from
a80cb60 to
87f5671
Compare
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>
Also refactor some duplicated code Co-authored-by: Claude Code <noreply@anthropic.com>
simonbaird
force-pushed
the
skip-att-sig-check
branch
from
87f5671 to
bc7ed38
Compare
Member
Author
|
Doing some extra work to get the Codecov check green. |
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.
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