Use ValidateVSAAndComparePolicy for ec validate image VSA skip

[st3penta]

IsValidVSA only checked Found && !Expired, so forged VSAs could skip
the entire validation pipeline (EC-1842). ValidateVSAAndComparePolicy
already handles this for ec validate vsa: it verifies signatures,
checks predicate status, and compares policy. Wire it into the image
path too.

Add --vsa-public-key to ec validate image, required when --vsa-upload
is set. Change ValidateImageWithVSACheck to take *vsa.VSAValidationConfig
instead of *vsa.VSAChecker + time.Duration.

Breaking change: --vsa-upload users need to also pass --vsa-public-key.
Default Konflux pipelines are unaffected.

Ref: https://issues.redhat.com/browse/EC-1998

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The validate image command now requires a VSA public key for VSA uploads. VSA-aware image validation uses VSAValidationConfig for signature, predicate, policy, expiration, and effective-time checks, with fallback to normal validation when VSA validation does not pass.

Changes

VSA validation flow

Layer / File(s)	Summary
CLI VSA configuration cmd/validate/image.go, cmd/validate/image_test.go	The command adds --vsa-public-key, stores the key path, rejects VSA uploads without it, and supplies complete VSA settings. Tests cover the required option and updated VSA upload invocations.
Image validation and fallback internal/image/validate.go, internal/image/validate_test.go	ValidateImageWithVSACheck accepts VSAValidationConfig, performs comprehensive VSA validation, skips normal validation after success, and falls back after failed, missing, or unavailable VSA data. Tests cover these outcomes.
Acceptance scenarios and documentation features/vsa.feature, docs/modules/ROOT/pages/ec_validate_image.adoc	The documentation and VSA scenarios include the required public key across storage, expiration, backend, and failure cases.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant ValidateImageWithVSACheck
  participant UploadBackedRetriever
  participant ValidateVSAAndComparePolicy
  participant ImageValidation
  CLI->>ValidateImageWithVSACheck: pass VSAValidationConfig
  ValidateImageWithVSACheck->>UploadBackedRetriever: retrieve VSA envelope
  UploadBackedRetriever-->>ValidateImageWithVSACheck: return envelope or retrieval error
  ValidateImageWithVSACheck->>ValidateVSAAndComparePolicy: validate VSA
  ValidateVSAAndComparePolicy-->>ValidateImageWithVSACheck: return validation result
  ValidateImageWithVSACheck->>ImageValidation: continue when VSA validation fails or is unavailable

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: using ValidateVSAAndComparePolicy to control VSA-based validation skipping for ec validate image.
Description check	✅ Passed	The description explains what and why, documents the breaking change, and includes a related Jira ticket reference.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:00 AM UTC · Ended 9:09 AM UTC

Commit: 87c4a29 · View workflow run →

[qodo-for-conforma]

PR Summary by Qodo

Harden ec validate image VSA skip path with full VSA verification

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Replace weak VSA skip check with signature, predicate, and policy-equivalence validation.
• Add --vsa-public-key and require it when --vsa-upload is used.
• Update unit tests to cover the new flag requirement and skip-path behavior.

Diagram

graph TD
  A(["ec validate image"]) --> B["Build VSAValidationConfig"] --> C["ValidateImageWithVSACheck"] --> D["ValidateVSAAndComparePolicy"] --> E{"VSA passed?"}
  E -->|"yes"| F["Skip full validation"]
  E -->|"no / error"| G["Run full image validation"]

  subgraph Legend
    direction LR
    _cli(["CLI entry"]) ~~~ _step["Processing step"] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make signature verification optional via explicit insecure flag

• ➕ Avoids breaking change for existing --vsa-upload users who don’t have a public key handy
• ➕ Keeps current behavior available for local/dev workflows
• ➖ Reintroduces a foot-gun on a security boundary (skip path)
• ➖ More surface area to document and test; higher chance of misconfiguration

2. Call `VSAChecker.CheckExistingVSAWithVerification` directly from image path

• ➕ Avoids duplicating/propagating VSAValidationConfig fields that image doesn’t strictly need
• ➖ Diverges from the already-established validate vsa implementation path
• ➖ Harder to keep policy-equivalence logic consistent across commands

Recommendation: Current approach is the best default: it reuses the existing ValidateVSAAndComparePolicy logic (already used by ec validate vsa) to harden a security-sensitive skip path. Requiring --vsa-public-key when --vsa-upload is set is an appropriate breaking change because skipping validation without signature verification is unsafe; if a softer migration is needed later, consider an explicit opt-in insecure flag rather than implicit behavior.

Files changed (4) +158 / -100

Bug fix (2) +32 / -16

image.goRequire VSA public key and wire full VSA validation into skip path +17/-3

Require VSA public key and wire full VSA validation into skip path

• Adds '--vsa-public-key' flag and enforces it when '--vsa-upload' is provided. Replaces the old VSA checker/expiration parameters with a 'VSAValidationConfig' and routes the skip decision through full VSA validation (signature, predicate, and policy equivalence).

cmd/validate/image.go

validate.goReplace 'IsValidVSA' skip check with 'ValidateVSAAndComparePolicy' +15/-13

Replace 'IsValidVSA' skip check with 'ValidateVSAAndComparePolicy'

• Changes 'ValidateImageWithVSACheck' to accept '*vsa.VSAValidationConfig' and use 'ValidateVSAAndComparePolicy' to decide whether to skip validation. Improves logging to surface signature verification and predicate outcomes when skipping or falling back.

internal/image/validate.go

Tests (2) +126 / -84

image_test.goUpdate CLI tests for new '--vsa-public-key' requirement +45/-0

Update CLI tests for new '--vsa-public-key' requirement

• Updates existing VSA upload/format tests to pass the new flag. Adds a dedicated test asserting '--vsa-public-key' is required when '--vsa-upload' is set.

cmd/validate/image_test.go

validate_test.goRewrite VSA skip-path unit tests around 'VSAValidationConfig' results +81/-84

Rewrite VSA skip-path unit tests around 'VSAValidationConfig' results

• Replaces the old mock checker approach with a mock retriever feeding DSSE envelopes representing passing/failing predicates. Adds coverage for skip-on-pass and fallback behavior when VSA retrieval/validation fails.

internal/image/validate_test.go

[qodo-for-conforma]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. validate_test.go build tag not first ✗ Dismissed 📜 Skill insight ≡ Correctness

Description

internal/image/validate_test.go is a modified _test.go file but its first line is a copyright
header, not a //go:build ... tag, violating the required build-tag-on-first-line convention.

Code

internal/image/validate_test.go[R430-433]

+type mockVSARetriever struct {
+	envelope *ssldsse.Envelope
+	err      error
+}

Relevance

●●● Strong

Build-tag-first-line compliance for unit tests; repository has recent unit-tag related enforcement
changes.

PR-#2698

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 3743 requires the first line of any new or modified Go test file to be a build tag.
In internal/image/validate_test.go, the first line is a copyright comment and the `//go:build
unit` tag appears later, so it does not meet the rule.

internal/image/validate_test.go[1-20]
Skill: pr-checklist

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Go compliance requires that any modified `*_test.go` file has a build tag on the **first line** (e.g., `//go:build unit`). `internal/image/validate_test.go` currently has `//go:build unit` at line 17, after the license header.

## Issue Context
The file was modified in this PR (new VSA config/envelope helpers and updated tests), so it must conform to the build-tag-on-first-line rule.

## Fix Focus Areas
- internal/image/validate_test.go[1-20]
- internal/image/validate_test.go[430-447]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. image_test.go build tag not first ✗ Dismissed 📜 Skill insight ≡ Correctness

Description

cmd/validate/image_test.go is a modified _test.go file but its first line is a copyright header,
not a //go:build ... tag, so it will fail the required build-tag-on-first-line convention.

Code

cmd/validate/image_test.go[R1547-1550]

+func TestValidateImageCommand_VSAPublicKeyRequired(t *testing.T) {
+	// --vsa-public-key is required when --vsa-upload is set
+	validateImageCmd := validateImageCmd(happyValidator())
+	cmd := setUpCobra(validateImageCmd)

Relevance

●●● Strong

Build-tag-first-line is a hard compliance rule; test file hygiene changes are typically accepted.

PR-#2678
PR-#2698

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 3743 requires the first line of any new or modified Go test file to be a build tag.
In cmd/validate/image_test.go, the first line is a copyright comment and the //go:build unit tag
appears later, so it does not meet the rule.

cmd/validate/image_test.go[1-20]
Skill: pr-checklist

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Go compliance requires that any modified `*_test.go` file has a build tag on the **first line** (e.g., `//go:build unit`). `cmd/validate/image_test.go` currently has `//go:build unit` at line 17, after the license header.

## Issue Context
The file was modified in this PR (new test added), so it must conform to the build-tag-on-first-line rule.

## Fix Focus Areas
- cmd/validate/image_test.go[1-20]
- cmd/validate/image_test.go[1547-1550]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Nil vsaConfig panic ✗ Dismissed 🐞 Bug ☼ Reliability

Description

ValidateImageWithVSACheck dereferences vsaConfig.VSAExpiration in trace logging before any nil
check, so a nil vsaConfig will panic when tracing is enabled. The downstream validator already
returns a clean error for nil validation config, but this dereference prevents that path.

Code

internal/image/validate.go[180]

+		trace.Logf(ctx, "", "image=%q vsa-expiration=%v", comp.ContainerImage, vsaConfig.VSAExpiration)

Relevance

●●● Strong

Deterministic nil-deref panic risk; similar reliability/panic-hardening changes have been accepted.

PR-#3386

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new trace log line directly dereferences vsaConfig, while the validator explicitly treats nil
config as an error—showing the deref is unnecessary and creates a panic risk.

internal/image/validate.go[173-184]
internal/validate/vsa/validator.go[42-49]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`internal/image.ValidateImageWithVSACheck` uses `vsaConfig.VSAExpiration` in a trace log message without checking `vsaConfig != nil`. If a caller ever passes nil (even accidentally), tracing-enabled runs will panic.

## Issue Context
`vsa.ValidateVSAAndComparePolicy` already returns an error when passed nil config, so `ValidateImageWithVSACheck` can fail gracefully if it validates `vsaConfig` first.

## Fix Focus Areas
- internal/image/validate.go[176-185]

### Suggested approach
- Add an early guard:
 - if `vsaConfig == nil`, return `ValidateImage(...)` (fallback) or return a clear error (depending on intended contract).
- Move the trace.Logf to after the nil guard, or log a nil-safe value.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Attestation time disables skip ✓ Resolved 🐞 Bug ≡ Correctness

Description

ec validate image accepts --effective-time=attestation, but the new VSA skip path forwards that
string to vsa.ValidateVSAAndComparePolicy, whose effective-time parser only supports now or
RFC3339. As a result, policy-equivalence checking never passes for attestation, so VSA-based
skipping is effectively disabled (and logs will misleadingly report “invalid effective time”).

Code

cmd/validate/image.go[R379-382]

+								VSAExpiration: data.vsaExpiration,
+								PublicKeyPath: data.vsaPublicKey,
+								PolicySpec:    data.policy.Spec(),
+								EffectiveTime: data.effectiveTime,

Relevance

●●● Strong

VSA skip path correctness bug; team historically accepts VSA validation/flag logic fixes.

PR-#2690
PR-#3080

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The validate-image CLI explicitly supports attestation as an --effective-time value, but VSA
validation’s ParseEffectiveTime does not; since ValidateVSAAndComparePolicy calls that parser
during policy comparison, an attestation value prevents a passing result and therefore prevents
skipping.

cmd/validate/image.go[556-561]
cmd/validate/image.go[371-384]
internal/validate/vsa/validator.go[123-135]
internal/validate/vsa/validator.go[249-257]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`validate image` supports `--effective-time` values like `attestation`, but the VSA policy-equivalence path only parses `now` or RFC3339. When `attestation` is used, `ValidateVSAAndComparePolicy` treats it as invalid and returns a non-passing result, preventing the intended “skip validation if VSA is valid & policy-equivalent” optimization.

## Issue Context
- `cmd/validate/image.go` documents and uses `attestation` for effective time.
- `internal/validate/vsa/validator.go` has its own `ParseEffectiveTime` with different semantics.

## Fix Focus Areas
- cmd/validate/image.go[371-384]
- cmd/validate/image.go[556-561]
- internal/validate/vsa/validator.go[123-135]
- internal/validate/vsa/validator.go[249-257]

### Suggested approach
- Reuse `internal/policy.ParseEffectiveTime(...)` semantics (supports `now`, `attestation`, RFC3339, and YYYY-MM-DD) or otherwise normalize `data.effectiveTime` before passing into VSA validation.
- If `attestation` cannot be resolved safely in the skip path, handle it explicitly (e.g., skip policy comparison with a clear reason code/message) instead of treating it as “invalid effective time”.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

✅ Compliance rules (platform): 36 rules

✅ Skills: build-and-lint, pr-checklist, write-tests

Tip of the day

💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[coderabbitai]

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_test.go`:
- Around line 451-457: Update createPassingVSAEnvelope to define a PolicySpec
with the expected source and policy, assign it to the returned validation data,
and build the predicate.policy from that same specification so the passing test
exercises policy comparison.

🪄 Autofix

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: e34f50d2-f28d-4e42-9640-91153a2911eb

📥 Commits

Reviewing files that changed from the base of the PR and between 0250ec5 and 9880bdf.

📒 Files selected for processing (4)

• cmd/validate/image.go
• cmd/validate/image_test.go
• internal/image/validate.go
• internal/image/validate_test.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:10 AM UTC · Completed 9:27 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [help-text-inconsistency] cmd/validate/image.go:599 — The --vsa-public-key flag help text says "Required when --vsa-upload is set" but the actual enforcement gate also requires --vsa-expiration > 0. Since --vsa-expiration defaults to 168h, the help text is practically correct for default usage, but could confuse users who explicitly set --vsa-expiration=0. Consider updating to: "Required when --vsa-upload is set and --vsa-expiration is greater than zero."

• [style-consistency] internal/image/validate_test.go — Test helpers createPassingVSAEnvelope() and createFailedVSAEnvelope() build JSON via string concatenation rather than struct marshaling. Minor style deviation; the concatenated strings only contain RFC3339 timestamps which do not require JSON escaping, so the fragility risk is low.

Previous run

Review

Findings

High

• [breaking-cli] cmd/validate/image.go:299 — The new validation requiring --vsa-public-key when --vsa-upload is set is a breaking change for any external CI/CD pipeline or automation that currently invokes ec validate image --vsa-upload ... without --vsa-public-key. Those invocations will now fail with exit code 1. The PR body acknowledges this and states default Konflux pipelines are unaffected, but any non-default downstream consumer will break.
  Remediation: Consider whether a deprecation period or a minor version bump is warranted. If the security fix (EC-1842) necessitates an immediate break, document the migration path in release notes.

Medium

• [edge-case] internal/validate/vsa/validator.go:254 — VSA validator's ParseEffectiveTime only handles "now" and RFC3339 formats, while the policy package additionally accepts YYYY-MM-DD dates. When a user passes --effective-time 2025-01-15 (date-only), the VSA path returns a non-passing ValidationResult with message "invalid effective time", causing validation to fall back to full image validation. This silently disables VSA caching for date-only effective times. (Pre-existing issue in validator.go, newly exposed by this PR's wiring.)
  Remediation: Add YYYY-MM-DD fallback parsing to vsa.ParseEffectiveTime, or reuse policy.ParseEffectiveTime.

Low

• [scope-intent] cmd/validate/image.go:375 — The effectiveTime handling translates the "attestation" sentinel value to policy.Now specifically for the VSA path. The design choice is defensible since attestation-time semantics are meaningless for VSA policy comparison, but a brief code comment would improve clarity.

• [stale-doc] THREAT_MODEL.md:97 — Section 3.1 lists --vsa-signing-key and --vsa-upload as ec validate image-specific flags but does not include the new --vsa-public-key flag, which crosses a trust boundary (operator-controlled public key for VSA signature verification).

• [test-inadequate] internal/image/validate_test.go:459 — No test case covering the error path where ValidateVSAAndComparePolicy returns an error from signature verification failure. The warn-and-continue behavior is tested implicitly via the retriever-error path, but an explicit signature-verification-error test case would improve coverage of this security-critical code path.

• [stale-doc] docs/modules/ROOT/pages/types-of-attestations-and-manifests.adoc:63 — The VSA section lists --vsa-signing-key and --vsa-upload for ec validate image but does not mention --vsa-public-key, which is now required when --vsa-upload is set.

• [stale-reference] internal/validate/vsa/vsa.go:554 — CreateVSACheckerFromUploadFlags is still exported and defined but after this PR removes the only production caller, it becomes dead code.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (2)

Review

Findings

High

• [logic-error] cmd/validate/image.go:299 — The validation len(data.vsaUpload) > 0 && data.vsaPublicKey == "" unconditionally requires --vsa-public-key whenever --vsa-upload is set. However, --vsa-upload is also used for uploading newly generated VSAs (e.g., tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml:469), where no public key is needed. The public key is only consumed in the VSAValidationConfig struct passed to ValidateImageWithVSACheck, which is only reached when data.vsaExpiration > 0. The in-repo Konflux task uses --vsa-upload without --vsa-public-key or --vsa-expiration and will break with this change, contradicting the PR claim that "Default Konflux pipelines are unaffected."
  Remediation: Narrow the guard to if len(data.vsaUpload) > 0 && data.vsaExpiration > 0 && data.vsaPublicKey == "" { ... }. This ensures the public key is only required when the VSA skip/verification path is active. See also: [backward-compatibility] finding.

Medium

• [backward-compatibility] cmd/validate/image.go:299 — The new --vsa-public-key flag is enforced whenever --vsa-upload is set, making this a breaking CLI change broader than necessary. The public key is only consumed when data.vsaExpiration > 0 activates the VSA skip/verification path. Existing users of --vsa-upload for upload-only workflows (without --vsa-expiration) will encounter unexpected errors. See also: [logic-error] finding.

Low

• [test-inadequate] internal/image/validate_test.go — No test verifies fallback behavior when the VSA has a valid signature and "passed" status but the policy does not match. Since the core purpose of this PR is to add policy comparison to the image validation path, a test for the policy-mismatch-fallback path would strengthen coverage.
• [stale-doc] THREAT_MODEL.md:97 — Section 3.1 "CLI arguments and flags" lists --vsa-signing-key and --vsa-upload as security-relevant flags for ec validate image but does not include the new --vsa-public-key flag. Since --vsa-public-key is a trust-boundary input for signature verification, it should be listed.
• [policy-comparison-bypass] internal/validate/vsa/validator.go:124 — Pre-existing: ValidateVSAAndComparePolicy skips policy comparison when PolicySpec.Sources is empty and still returns Passed: true. Mitigated in practice because image validation always provides a policy with sources.
• [log-message-contract] features/__snapshots__/vsa.snap — Warning log message format changed from "Failed to check for existing VSA" to "Failed to validate existing VSA...failed to check existing VSA". If any monitoring matches on the old text, it may need updating.
• [error-handling-idiom] cmd/validate/image.go:299 — Error message uses "required when --vsa-upload is set" while the existing pattern at line 292 uses "required for --attestation-format=dsse". Minor format inconsistency.
• [exported-function-signature] internal/image/validate.go:175 — ValidateImageWithVSACheck signature changed from (*vsa.VSAChecker, time.Duration) to (*vsa.VSAValidationConfig). Located under internal/, so no cross-repo impact.
• [scope-creep] features/__snapshots__/validate_image.snap — 1152 lines of dead snapshot data removed (the "many components and sources" fixture whose scenario is commented out). While valid cleanup, this is unrelated to the security fix.
• [validation-grouping] cmd/validate/image.go:299 — The new validation is placed outside the if data.vsaEnabled block. This is actually correct: --vsa-upload with --vsa-expiration can activate the skip path without --vsa, so the check should not be inside the data.vsaEnabled guard.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (3)

Review

Findings

Medium

• [missing-new-flag] THREAT_MODEL.md:98 — The new --vsa-public-key flag (required when --vsa-upload is set) is not listed in THREAT_MODEL.md section 3.1, which inventories ec validate image CLI flags including --vsa-signing-key and --vsa-upload. Since this flag controls a security-relevant trust boundary (public key for VSA signature verification), it should be included in the threat model's entry-point inventory.
  Remediation: Add a row for --vsa-public-key in the section 3.1 table.

Low

• [incomplete-flag-listing] docs/modules/ROOT/pages/types-of-attestations-and-manifests.adoc:63 — The VSA section lists --vsa, --vsa-signing-key, --vsa-upload but does not mention the new --vsa-public-key requirement when --vsa-upload is set.
  Remediation: Add a note that --vsa-public-key is required when --vsa-upload is set.

• [test integrity] internal/image/validate_test.go — Unit tests for ValidateImageWithVSACheck all set IgnoreSignatureVerification: true. Signature verification is tested at the validator layer (validator_test.go) and in acceptance tests (features/vsa.feature), so coverage exists — but the integration through ValidateImageWithVSACheck specifically is not covered at the unit level.

• [import grouping] internal/image/validate_test.go:49 — The ecapi import is placed in its own blank-line-separated group. While validator.go uses the same pattern, most files in the codebase place ecapi in the third-party group. Minor inconsistency.

• [stale reference] internal/validate/vsa/vsa.go:554 — CreateVSACheckerFromUploadFlags is no longer called from any production code after this PR. It remains exported and tested but is now dead code.

• [edge case] cmd/validate/image.go — The --vsa-public-key validation requires the flag even when --vsa-expiration=0 (where the VSA skip path would never execute). Conservatively correct but could produce a confusing error for users who set expiration to 0 while keeping --vsa-upload for VSA generation.

Previous run (4)

Review

Findings

Medium

• [missing-doc] THREAT_MODEL.md:97 — Section 3.1 enumerates CLI flags for ec validate image including --vsa-signing-key and --vsa-upload, but does not list the new --vsa-public-key flag. While the flag is documented in section 3.8 for ec validate vsa (line 166), section 3.1 should be updated to reflect that --vsa-public-key now also applies to ec validate image.
  Remediation: Add a row to the section 3.1 table for --vsa-public-key with trust level ‘Operator-controlled’.

• [stale-doc] internal/validate/vsa/DESIGN.md:21 — States ‘The signing key is the same key used for the original image validation.’ This PR introduces --vsa-public-key as a separate key for VSA signature verification, distinct from --vsa-signing-key used for signing.
  Remediation: Update to reflect that VSA signing and verification use dedicated keys (--vsa-signing-key and --vsa-public-key).

• [breaking-cli] cmd/validate/image.go:299 — --vsa-upload now requires --vsa-public-key to be set, which is a breaking change to the CLI contract. Any CI/CD pipeline or script using ec validate image --vsa-upload ... without --vsa-public-key will fail. The PR body acknowledges this and states default Konflux pipelines are unaffected.
  Remediation: Ensure release notes communicate the breaking change and migration path.

Low

• [test-inadequate] internal/image/validate_test.go:443 — The PR adds test cases for ‘VSA passed - skip validation’ and ‘VSA predicate failed’ paths, improving coverage of the new result.Passed branching logic. The base-branch tests only exercised the error/fallback path via a mock retriever that always returned errors.

• [input-validation-gap] cmd/validate/image.go:597 — The --vsa-public-key flag accepts a file path but does not validate that the file exists in PreRunE. Errors surface later during signature verification. Consistent with existing --vsa-signing-key pattern.

• [stale-reference] internal/validate/vsa/vsa.go:554 — CreateVSACheckerFromUploadFlags still exists but is no longer called from the image validation path after this PR. Consider deprecating or removing it if no other callers remain.

• [code-organization] cmd/validate/image.go:682 — The vsaPublicKey struct field is placed after vsaExpiration instead of adjacent to vsaSigningKey. Similarly, --vsa-public-key is registered between --vsa-expiration and --attestation-output-dir rather than next to --vsa-signing-key. Grouping key-related fields and flags together would improve readability.

Previous run (5)

Review

Findings

Medium

• [edge-case] internal/validate/vsa/validator.go:251 — ParseEffectiveTime in the vsa package only handles "now" and RFC3339 values. When --effective-time=attestation is used, data.effectiveTime flows into VSAValidationConfig.EffectiveTime, and ParseEffectiveTime fails to parse it, causing ValidateVSAAndComparePolicy to return Passed: false. The caller falls through to full validation (safe), but the VSA caching optimization is silently defeated for --effective-time attestation users.
  Remediation: Add an "attestation" case in the vsa package’s ParseEffectiveTime, or handle it before constructing VSAValidationConfig.

• [breaking-cli] cmd/validate/image.go:299 — Adding a required --vsa-public-key flag when --vsa-upload is set is a breaking change to the CLI contract. Existing automation invoking ec validate image --vsa-upload ... without --vsa-public-key will now fail. The PR body acknowledges this as intentional.
  Remediation: Document in release notes. Consider a deprecation warning in one release before making it a hard error, if external consumers exist beyond default Konflux pipelines.

• [missing-doc] THREAT_MODEL.md:97 — THREAT_MODEL.md section 3.1 lists security-relevant CLI flags including --vsa-signing-key and --vsa-upload but does not include the new --vsa-public-key flag, which controls cryptographic verification of VSA signatures.
  Remediation: Add a row for --vsa-public-key in the section 3.1 table.

Low

• [edge-case] cmd/validate/image.go:299 — The validation requires --vsa-public-key whenever --vsa-upload is set, regardless of --vsa-expiration. A user who sets --vsa-expiration 0 to disable VSA checking is forced to provide a key that is never used.
  Remediation: Consider gating on vsaExpiration > 0 as well.

• [error-message-style] cmd/validate/image.go:298 — Error message phrasing differs from adjacent validation: "--vsa-public-key is required when --vsa-upload is set" vs existing "--vsa-signing-key required for --attestation-format=dsse".
  Remediation: Align style, e.g., "--vsa-public-key required when --vsa-upload is set".

• [stale-doc] .cursor/rules/vsa_functionality.mdc:336 — References ValidateVSAWithPolicyComparison and ValidationData which were renamed to ValidateVSAAndComparePolicy and VSAValidationConfig in a prior change. The ValidateImageWithVSACheck signature documentation is also outdated.
  Remediation: Update API references in .cursor/rules/vsa_functionality.mdc.

---

Labels: PR modifies CLI flag validation for VSA security hardening and addresses a red team finding about VSA bypass

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:29 AM UTC · Completed 9:49 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:19 AM UTC · Ended 10:25 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:26 AM UTC · Completed 10:47 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 10:50 AM UTC · Ended 11:11 AM UTC

Commit: 87c4a29 · View workflow run →

[coderabbitai]

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_test.go`:
- Around line 529-532: Strengthen fallback validation assertions in
internal/image/validate_test.go at lines 529-532 and 571-573: when expectSkip is
false, assert that the output from ValidateImageWithVSACheck is non-nil,
retaining the returned out value at lines 571-573 before asserting it. Keep the
existing error assertions and skip behavior unchanged.

🪄 Autofix

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: a88a7a2b-e0e6-4c73-b138-4cd4a468e64b

📥 Commits

Reviewing files that changed from the base of the PR and between 942d085 and 5a66a85.

📒 Files selected for processing (1)

• internal/image/validate_test.go

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/validate/image.go	93.33%	1 Missing ⚠️
internal/image/validate.go	88.88%	1 Missing ⚠️

Flag	Coverage Δ
acceptance	54.45% <75.00%> (+0.03%)	⬆️
generative	16.33% <0.00%> (-0.02%)	⬇️
integration	27.55% <16.66%> (-0.02%)	⬇️
unit	72.15% <54.16%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/validate/image.go	91.32% <93.33%> (+<0.01%)	⬆️
internal/image/validate.go	76.97% <88.88%> (+4.78%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:50 AM UTC · Completed 11:11 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:00 PM UTC · Completed 1:17 PM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:27 PM UTC · Completed 2:43 PM UTC

Commit: 87c4a29 · View workflow run →

Superseded by updated review