feat(secrets): stop retaining Secret values in the control plane#208
Merged
Conversation
…cret watches Implements Track A, steps 1-3 of the secret-handling roadmap (docs/future/secret-handling-roadmap.md) - the immediate process- and crypto-level security wins, ahead of the separate RBAC-narrowing track. Step 1 - stop caching Secret values, drop control-plane Secret watches: - Remove the GitTarget control-plane Secret watch (and its now-orphaned encryptionSecretToGitTargets map func). A full-object Secret watch made the controller-runtime cache retain every Secret value in every namespace. - Disable the typed-Secret cache on the manager (Client.Cache.DisableFor corev1.Secret), so typed Secret reads go straight to the API server and stay fresh. GitProvider stays pull-based (no auth-Secret watch, by design). Step 2 - remove private age identities from the SOPS encrypt path: - The write path only encrypts, so it needs public age recipients only. - Drop AgeIdentities/Environment from ResolvedEncryptionConfig; delete the age identity temp-file path / SOPS_AGE_KEY_FILE; stop passing blanket Secret data into the sops process env. Recipients still reach sops via the target .sops.yaml, so encryption output is unchanged. A BYO age-key Secret is still read to derive its public recipient; the private identity is never returned, written to disk, or handed to sops. Step 3 - unify the control-plane reconcile fallback at 5 minutes: - Collapse the short (2m) / medium (5m) / long (10m) requeue intervals into a single RequeueSteadyInterval (5m) for GitProvider, GitTarget, WatchRule, and ClusterWatchRule. Genuine spec changes still trigger immediately via watches; this is only the periodic fallback that picks up out-of-band credential and age-key changes now that Secrets are not watched. Docs: security-model.md and design/reconcile-triggering.md updated to the direct-read model (behavior change, not yet an RBAC change - the default install still grants secrets get;list;watch until the RBAC-narrowing track lands); the gitpathaccepted-projection-race design doc gets a dated note for the removed Secret watch and unified interval. Adds the roadmap and two plan docs under docs/future/. RBAC narrowing (Track B) is intentionally out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR unifies controller requeue timing around a single 5-minute steady interval, removes Secret watch/caching paths from controller runtime behavior, refactors age encryption to use recipients only, and updates design documents to match the new runtime and future RBAC plans. ChangesRequeue interval unification and Secret retention removal
Estimated code review effort: 4 (Complex) | ~60 minutes Design and roadmap documentation
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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 `@docs/future/secret-value-retention-plan.md`:
- Around line 29-31: Update the “Current problem” wording to match the new
Secret cache exception introduced by Client.Cache.DisableFor for corev1.Secret.
In the secret-value-retention-plan document, revise the manager/cache baseline
text so it no longer says the manager is simply created without cache options;
make it clear that the shared cache still applies generally, but Secrets are now
explicitly excluded, and keep the distinction between the existing state and
what Track A changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 14b58067-3923-4abf-8f21-bbee92c19a97
📒 Files selected for processing (17)
cmd/main.godocs/design/manifest/gitpathaccepted-projection-race-and-external-drift.mddocs/design/reconcile-triggering.mddocs/future/scoped-rbac-least-privilege-plan.mddocs/future/secret-handling-roadmap.mddocs/future/secret-value-retention-plan.mddocs/security-model.mdinternal/controller/clusterwatchrule_controller.gointernal/controller/constants.gointernal/controller/dependency_watches_test.gointernal/controller/gitprovider_controller.gointernal/controller/gitprovider_controller_test.gointernal/controller/gittarget_controller.gointernal/controller/watchrule_controller.gointernal/git/encryption.gointernal/git/encryption_test.gointernal/watch/manager.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ommit Follow-up to the secret-value-retention change (plan step 5). With the Secret cache disabled, every typed Secret read is a real API GET, so redundant reads now cost etcd round-trips. commitPendingWrites resolved the credentials Secret unconditionally but only used it on the first commit of a push cycle (the branch that runs PrepareBranch to fetch the remote tip). Every subsequent commit in a burst read the credentials Secret - plus its known_hosts lookups - and discarded it. Resolve auth lazily, only inside the !hasPendingCommits branch that actually touches the remote, mirroring ensureRepositoryInitialized. No memo/cache is introduced (the plan cautions against that - it would reintroduce stale-credential behavior): pushPendingCommits and the first commit still resolve directly, once per operation, reflecting current credentials. This only removes the reads that were never used. Adds a unit test that counts credentials-Secret GETs across a two-commit cycle: 1 on the first commit, 0 on the second. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update the three secret-handling docs to the current state: Track A (retention + crypto exposure) shipped in this PR; Track B (RBAC narrowing) not started. Marks the retention-plan "Done" checklist complete and annotates the roadmap's recommended order, keeping the runtime-vs-RBAC distinction explicit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
changed the title
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 `@docs/future/secret-value-retention-plan.md`:
- Around line 3-20: The blockquote in secret-value-retention-plan.md contains an
empty quoted line that triggers markdownlint MD028. Update the quoted section
near the Status/Scope text so there is no blank line inside the blockquote, or
make the blank line an explicit quoted line using the same blockquote marker.
Keep the fix localized to the roadmap note content.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18d7661a-8b60-4b38-9c36-8de072a7986d
📒 Files selected for processing (5)
docs/future/scoped-rbac-least-privilege-plan.mddocs/future/secret-handling-roadmap.mddocs/future/secret-value-retention-plan.mdinternal/git/branch_worker.gointernal/git/branch_worker_credread_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/future/scoped-rbac-least-privilege-plan.md
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.
Summary
Implements Track A of the secret-handling roadmap
(docs/future/secret-handling-roadmap.md) — the
process- and crypto-level security wins for #205,
ahead of the separate RBAC-narrowing track. The controller no longer keeps cluster Secret
values in memory, no longer writes private age keys to disk, and reacts to credential/age-key
changes on one 5-minute reconcile instead of a Secret informer.
What changed
Client.Cache.DisableFor: corev1.Secret, so typed Secret reads go straight to the API andnothing caches Secret values. GitProvider stays pull-based.
SOPS_AGE_KEY_FILE, no identity temp file, no blanket secret-data env. Recipients stillreach
sopsvia the target.sops.yaml, so encrypted output is unchanged.RequeueSteadyIntervalfor the four control-plane reconcilers.on every commit but only used it on the first (the remote fetch); it now resolves lazily.
No cache/memo is introduced.
Out of scope
RBAC narrowing (Track B). This is a runtime change: the default install still grants
secrets get;list;watchplus the dynamic-watch wildcard until the RBAC-narrowing track lands.security-model.mdanddesign/reconcile-triggering.mdcall that distinction out.Validation
task lint✅ ·task test✅ (coverage ratchet holds) ·task test-e2e✅ (55 passed / 0failed), including the SOPS/age specs that exercise the recipients-from-
.sops.yamlpathend-to-end.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation