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.
... 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
|
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
|
Doing some extra work to get the Codecov check green. |
| // Handle attestation signature validation | ||
| if p.SkipAttSigCheck() { | ||
| log.Debug("Attestation signature check skipped, fetching attestations without verification") | ||
| if err := a.FetchAttestationsWithoutVerification(ctx); err != nil { | ||
| log.Debugf("Failed to fetch attestations without verification: %v", err) | ||
| // Continue with validation even if attestation fetch fails | ||
| } | ||
| } else { | ||
| out.SetAttestationSignatureCheckFromError(a.ValidateAttestationSignature(ctx)) | ||
| if !out.AttestationSignatureCheck.Passed { | ||
| return out, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
When this path is taken, AttestationSignatureCheck in the output is {passed: false, result: null} -- indistinguishable from "never reached." An explicit "skipped" result would make the output unambiguous for downstream consumers and audit trails. (Same gap exists for --skip-image-sig-check, but out of scope here.)
Also: combining both skip flags silently disables all crypto verification with no indication in the output. A Warn log when both are active would help catch accidental misuse.
| return out, nil | ||
| // Handle attestation signature validation | ||
| if p.SkipAttSigCheck() { | ||
| log.Debug("Attestation signature check skipped, fetching attestations without verification") |
There was a problem hiding this comment.
Skipping a security check is operationally significant -- if this flag is accidentally left in a CI config, operators should be able to spot it without enabling Debug verbosity. Consider log.Warn here.
| } | ||
|
|
||
| remoteOpts := oci.CreateRemoteOptions(ctx) | ||
| signedEntity, err := ociremote.SignedEntity(a.reference, ociremote.WithRemoteOptions(remoteOpts...)) |
There was a problem hiding this comment.
This calls ociremote.SignedEntity directly, bypassing the oci.Client abstraction every other OCI operation in this file uses. That import was deliberately removed in #3269 to centralize OCI access. Adding a method to oci.Client would keep the pattern consistent and make this testable with fake.FakeClient.
| func (a *ApplicationSnapshotImage) FetchAttestationsWithoutVerification(ctx context.Context) error { | ||
| if trace.IsEnabled() { | ||
| region := trace.StartRegion(ctx, "ec:fetch-attestations-without-verification") | ||
| defer region.End() | ||
| } | ||
|
|
||
| remoteOpts := oci.CreateRemoteOptions(ctx) | ||
| signedEntity, err := ociremote.SignedEntity(a.reference, ociremote.WithRemoteOptions(remoteOpts...)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to fetch signed entity: %w", err) | ||
| } | ||
|
|
||
| layers, err := signedEntity.Attestations() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to fetch attestations: %w", err) | ||
| } | ||
|
|
||
| // Check if using bundles | ||
| useBundles := a.hasBundles(ctx) | ||
| if useBundles { | ||
| sigs, err := layers.Get() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get attestation signatures: %w", err) | ||
| } | ||
| return a.parseAttestationsFromBundles(sigs) | ||
| } | ||
|
|
||
| sigs, err := layers.Get() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get attestation signatures: %w", err) | ||
| } | ||
|
|
||
| return a.parseAttestationsFromSignatures(sigs) | ||
| } |
There was a problem hiding this comment.
This method has four error paths and a bundle/non-bundle branch, none tested at the unit level. The integration test does not mock this path either, so the fetch silently fails there. Routing through oci.Client (per the comment above) would make it straightforward to test with the existing fake infrastructure.
| func TestValidateImageSkipAttSigCheck(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| skipAttSigCheck bool | ||
| expectAttSigResult bool | ||
| expectImageSigCheckCall bool | ||
| }{ | ||
| { | ||
| name: "skip attestation signature check disabled (default)", | ||
| skipAttSigCheck: false, | ||
| expectAttSigResult: true, | ||
| expectImageSigCheckCall: true, | ||
| }, | ||
| { | ||
| name: "skip attestation signature check enabled", | ||
| skipAttSigCheck: true, | ||
| expectAttSigResult: false, | ||
| expectImageSigCheckCall: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
No mocks are set up for ociremote.SignedEntity, so FetchAttestationsWithoutVerification always fails silently in the skip-enabled case. This validates "error was swallowed" rather than "attestations were fetched without verification" -- a regression that ignores the flag entirely would still pass.
| Scenario: happy day with skip-att-sig-check flag | ||
| Given a key pair named "known" | ||
| Given an image named "acceptance/ec-happy-day" | ||
| Given a valid image signature of "acceptance/ec-happy-day" image signed by the "known" key | ||
| Given a valid attestation of "acceptance/ec-happy-day" signed by the "known" key | ||
| Given a git repository named "happy-day-policy" with | ||
| | main.rego | examples/happy_day.rego | | ||
| Given policy configuration named "ec-policy" with specification | ||
| """ | ||
| { | ||
| "sources": [ | ||
| { | ||
| "policy": [ | ||
| "git::https://${GITHOST}/git/happy-day-policy.git" | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| """ | ||
| When ec command is run with "validate image --image ${REGISTRY}/acceptance/ec-happy-day --policy acceptance/ec-policy --public-key ${known_PUBLIC_KEY} --skip-att-sig-check --rekor-url ${REKOR} --show-successes --output json" | ||
| Then the exit status should be 0 | ||
| # The only difference to the happy day scenario is that | ||
| # builtin.attestation.signature_check is not found in the success output | ||
| Then the output should match the snapshot | ||
|
|
||
| Scenario: happy day with git config and yaml | ||
| Given a key pair named "known" | ||
| Given an image named "acceptance/ec-happy-day" |
There was a problem hiding this comment.
The attestation here is signed with the correct key, so the skip flag is redundant. The --skip-image-sig-check has a companion scenario at line 214 with an invalid signature that proves the flag is needed. An equivalent scenario with a wrong attestation key would catch a regression that silently ignores this flag.
There was a problem hiding this comment.
Yeah, I was being lazy here main to satisfy Codecov stats. 😅
| sigs, err := layers.Get() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get attestation signatures: %w", err) | ||
| } | ||
| return a.parseAttestationsFromBundles(sigs) | ||
| } | ||
|
|
||
| sigs, err := layers.Get() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get attestation signatures: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
layers.Get() is identical in both branches -- could hoist it above the conditional.
sigs, err := layers.Get()
if err != nil {
return fmt.Errorf("failed to get attestation signatures: %%w", err)
}
if useBundles {
return a.parseAttestationsFromBundles(sigs)
}
return a.parseAttestationsFromSignatures(sigs)| name string | ||
| skipAttSigCheck bool | ||
| expectAttSigResult bool | ||
| expectImageSigCheckCall bool |
There was a problem hiding this comment.
expectImageSigCheckCall is populated in both cases but never asserted. Remove it or add the intended check.
| return a.parseAttestationsFromSignatures(sigs) | ||
| } | ||
|
|
||
| func (a *ApplicationSnapshotImage) parseAttestationsFromSignatures(sigs []cosignOCI.Signature) error { |
There was a problem hiding this comment.
ValidateAttestationSyntax docstring references ValidateAttestationSignature as the required prefill, but this function is now also a valid prefill path. Worth updating the docstring to mention both.
| expectAttSigResult: false, | ||
| expectImageSigCheckCall: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Consider adding a case with both --skip-image-sig-check and --skip-att-sig-check enabled -- that is the expected real-world debugging usage per the PR description.
Co-authored-by: Stefano Pentassuglia <spentass@redhat.com>
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
| log.Debug("Attestation signature check skipped, fetching attestations without verification") | ||
| if err := a.FetchAttestationsWithoutVerification(ctx); err != nil { | ||
| log.Warnf("Failed to fetch attestations without verification: %v", err) | ||
| out.SetAttestationSignatureCheckFromError(fmt.Errorf("failed to fetch attestations (signature check skipped): %w", err)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
p = Path("internal/image/validate.go").read_text()
imports = p.split("import (", 1)[1].split(")", 1)[0]
print("fmt.Errorf used:", "fmt.Errorf(" in p)
print('fmt imported:', '"fmt"' in imports)
if "fmt.Errorf(" in p and '"fmt"' not in imports:
raise SystemExit("verification_failed: fmt.Errorf is used without importing fmt")
PYRepository: conforma/cli
Length of output: 178
Add the missing fmt import before merging.
Line 89 uses fmt.Errorf, but this file does not import fmt, which prevents the package from compiling.
Proposed fix
import (
"context"
"encoding/json"
+ "fmt"
"runtime/trace"
"sort"
"time"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| out.SetAttestationSignatureCheckFromError(fmt.Errorf("failed to fetch attestations (signature check skipped): %w", err)) | |
| import ( | |
| "context" | |
| "encoding/json" | |
| "fmt" | |
| "runtime/trace" | |
| "sort" | |
| "time" |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 89-89: : # github.com/conforma/cli/internal/image [github.com/conforma/cli/internal/image.test]
internal/image/validate.go:89:46: undefined: fmt
(typecheck)
🤖 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.go` at 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).
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