fix(security): move Flux cosign verify to the path KSail reads - #2919
Conversation
The cosign keyless verify block sat at spec.cluster.verify. KSail reads it at spec.workload.flux.verify (FluxVerifySpec is reachable only via WorkloadSpec.Flux.Verify; ClusterSpec has no verify field), so the block was an unknown key and was silently dropped. The generated flux-system OCIRepository therefore carries no spec.verify, and production pulls the manifests artifact from a mutable `latest` tag with signature verification disabled. Moves the block verbatim — provider, all three matchOIDCIdentity entries, and their rationale — to spec.workload.flux.verify. Content is unchanged; only the location moves. Part of #2627
Evidence1. The key is unreachable.
⇒ the only path is 2. The consequence is live ( 3. The move is content-preserving. Comparing the old block against the new one as parsed data ( 4. Validation. What is NOT provenThat the published artifact will actually pass these three matchers at pull time. |
Ablation — why this went unnoticed for three weeksI ran the repo's own validation against both configs, changing only the location of the verify block:
Identical, and arm B emits no unknown-key or ignored-field warning — the only warnings in either run are unrelated So That makes a regression guard genuinely load-bearing rather than nice-to-have — a green validation run is not evidence that verification is configured. Filed separately; it wants a check that asserts the rendered/effective state, not one that greps the config, since a text check would be defeated by exactly the kind of restructuring that caused this. |
✅
|
Requesting a CodeRabbit review at the current head @coderabbitai review |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe production configuration removes the misplaced top-level Flux OCIRepository Cosign verification block. It adds the verification policy under Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Re-triggering CodeRabbit at head @coderabbitai review |
|
✅ Action performedReview finished.
|
![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
🤖 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 `@ksail.prod.yaml`:
- Around line 200-230: Add rendered-policy tests for the production Flux
workload configuration before enabling Cosign enforcement: verify KSail renders
OCIRepository.spec.verify with all three allowed matchOIDCIdentity subjects for
merge-queue ci.yaml, main-branch cd.yaml, and main-branch dr-rebuild.yaml, and
verify an invalid identity is rejected. Use the existing
effective-state/rendered validation test mechanisms and keep the configured
production identities unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13564c65-155f-4ed4-baa9-a96a75100320
📒 Files selected for processing (1)
ksail.prod.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
ksail*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Treat
ksail.yamlandksail.prod.yamlas ordinary configuration, validate both environments, and do not modify cluster state during maintenance.
Files:
ksail.prod.yaml
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Use Conventional Commit messages and titles, keep changes small and focused, and create pull requests as drafts.
Land new behavior disabled, validate it, then enable it using the appropriate layer: OpenFeature for per-request flags, Flagger for traffic shifting, Helm values/Kustomize for component toggles, and Kubernetes feature gates for platform behavior.
Files:
ksail.prod.yaml
The relocation half is now proven; the artifact half is blocked on a token scopeTwo separate claims sit behind this PR, and only one of them was ever about the review. Splitting them: 1. Does the block land where KSail reads it? — YES, verified structurally at the pinned version
Production renders with
So the key this PR moves to is the only one that exists, and the key it moves from was never in the schema — which is precisely why it was accepted and discarded in silence. 2. Is verification actually off in production right now? — YES, confirmed live3. Will the live artifact pass once enforcement is on? — STILL UNVERIFIED, and this is the promotion gateThe recorded blocker was "
The host PAT is missing Staying a draft. Merging starts enforcing signatures on the source that delivers every infrastructure manifest, so "the policy is now in the right place" is not sufficient — an artifact that fails the check stops manifest delivery. The relocation is right and the bug is real; the last step needs the scope. |
Readiness — the promotion gate this PR was parked on is now closedThe draft said it was waiting on one thing: proof that the live artifact will actually pass the 1. The defect is real, confirmed live. On the prod cluster the infra source is running
2. The key this PR moves to is the one KSail actually reads. Traced to the consumer, not 3. The live artifact is signed, by an identity this policy accepts. The artifact Flux is serving
which is the merge-queue prod deploy path — the first of the three pinned subjects. 4. The policy is discriminating, not vacuous. The accepting match was checked against five 5. Signing is unconditional and ran green. The Stated limit, so this is not read as more than it is. The agent PAT has no Operational effect on merge is unchanged from the body: Flux begins enforcing signatures on the |
Post-merge verification — the config landed, the enforcement did notChecked the live cluster after this merged and its deploy went green, rather than assuming the
So the key move is correct and necessary — it is genuinely the path KSail reads — but on its own it Two contributing causes, both checked: the verify patch sits on KSail's Flux install / Tracked as #2922 (sub-issue of #2627). #2850 asked to move the key and activate; the move landed |
1 participant
Why
Production has been pulling its infrastructure manifests with signature verification switched off, and nothing reported it. The cosign policy was written and looks correct — it just sits under a key KSail does not read, so it was silently discarded. Confirmed on the live cluster today: the
flux-systemsource tracking the mutablelatesttag has no verification attached.What
Moves the cosign block to the key KSail actually reads. The policy itself is untouched — same provider, same three trusted signers, same wording; only its location changes.
Operational note — this changes production behaviour. Once merged and reconciled, Flux starts enforcing signatures on the infrastructure source. That is the intended effect, but it is a real switch: if the published artifact does not match one of the three trusted signers, Flux stops applying new manifests (it keeps running the last good state, and reverting this restores the old behaviour).
Staying a draft until one thing is confirmed. I could not independently re-verify the live artifact's signature in this run —
cosignis not installed on the agent host — so I have not proven the artifact will pass. That check is the promotion gate, not the review.This delivers #2850. The rest of parent #2627 — publishing
latestonly after signing and attesting, and the DR rebuild's permissions — is tracked in its other children and untouched here.Fixes #2850
Part of #2627