diff --git a/cmd/main.go b/cmd/main.go index 613e13ff..43985ad5 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -23,12 +23,14 @@ import ( "go.uber.org/zap/zapcore" _ "k8s.io/client-go/plugin/pkg/client/auth" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/certwatcher" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" @@ -843,6 +845,18 @@ func newManager( Metrics: metricsOptions, HealthProbeBindAddress: probeAddr, WebhookServer: webhookServer, + // Never cache Secret values. The control plane reads a small set of named + // Secrets (Git credentials, signing keys, age keys) directly by name; caching + // them would start a cluster-wide Secret informer that retains every Secret + // value in memory. Typed Secret Get/List go straight to the API server instead, + // which also keeps credential/age-key rotation reads fresh. Mirrored Secrets + // selected by a WatchRule use the separate dynamic-watch path, not this cache. + // See docs/future/secret-value-retention-plan.md. + Client: client.Options{ + Cache: &client.CacheOptions{ + DisableFor: []client.Object{&corev1.Secret{}}, + }, + }, }) if err != nil { setupLog.Error(err, "unable to start manager") diff --git a/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md b/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md index 9c375215..21683672 100644 --- a/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md +++ b/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md @@ -25,6 +25,16 @@ is and is not the cause, then the design direction. ## 1. Facts and observations (no interpretation yet) +> **Update (2026-07-07, secret-value-retention):** §1 is a point-in-time snapshot of the +> code at investigation time. Two facts below have since changed. (1) The GitTarget +> control-plane **Secret watch was removed**, so `SetupWithManager` now watches +> `GitProvider`, `WatchRule`, and `ClusterWatchRule` — not `Secret` (§1.5). (2) The +> `RequeueShortInterval`/`RequeueLongInterval` split (2 min / 10 min) collapsed into a single +> 5-minute `RequeueSteadyInterval`, so the happy-path GitTarget reconcile requeues after +> **5 minutes, not 10** (§1.5, §2). The projection-race analysis and its GitPath-channel fix +> are otherwise unaffected. See +> [`../../future/secret-value-retention-plan.md`](../../future/secret-value-retention-plan.md). + ### 1.1 The failure CI run [`28331751067`](https://github.com/ConfigButler/gitops-reverser/actions/runs/28331751067) diff --git a/docs/design/reconcile-triggering.md b/docs/design/reconcile-triggering.md index 9b6976d8..1441c39b 100644 --- a/docs/design/reconcile-triggering.md +++ b/docs/design/reconcile-triggering.md @@ -28,23 +28,33 @@ Facts first, then the fix, then the gaps, then the toolbox. | Constant | Value | |---|---| | `RequeueStreamSettleInterval` | 10 s | -| `RequeueShortInterval` | 2 min | -| `RequeueMediumInterval` | 5 min | -| `RequeueLongInterval` | 10 min | +| `RequeueSteadyInterval` | 5 min | + +> **Update (2026-07-07, secret-value-retention):** the former `RequeueShortInterval` +> (2 min), `RequeueMediumInterval` (5 min), and `RequeueLongInterval` (10 min) collapsed +> into a single 5-minute `RequeueSteadyInterval`. The control plane no longer watches +> Secrets, so out-of-band credential/age-key changes are picked up on this unified steady +> cadence instead of via a Secret informer. Where the narrative below says "up to 10 +> minutes," read 5 minutes. See +> [`future/secret-value-retention-plan.md`](../future/secret-value-retention-plan.md). ### 1.2 Per-controller triggers | Controller | Watches (besides `For`) | Predicate | Happy-path requeue | Data-plane edge | |---|---|---|---|---| -| **GitProvider** | *(none)* | `GenerationChanged` on self | 10 min | ❌ | -| **GitTarget** | Secret, GitProvider, WatchRule, ClusterWatchRule | `GenerationChanged` on deps | 10 min | ✅ GitPath channel (new) | +| **GitProvider** | *(none)* | `GenerationChanged` on self | 5 min | ❌ | +| **GitTarget** | GitProvider, WatchRule, ClusterWatchRule | `GenerationChanged` on deps | 5 min | ✅ GitPath channel (new) | | **WatchRule** | GitTarget, GitProvider | `GenerationChanged` | 5 min | ❌ | | **ClusterWatchRule** | GitTarget, GitProvider | `GenerationChanged` | 5 min | ❌ | | **CommitRequest** | *(none)* | — | polls every 2 s | polls worker, no push | -Sources: `gitprovider_controller.go:428`, `gittarget_controller.go:967`, -`watchrule_controller.go:458`, `clusterwatchrule_controller.go:455`, -`commitrequest_controller.go:377`. +The GitTarget no longer watches Secrets: the encryption-Secret watch was removed so the +process stops retaining every Secret value in the cluster; generated-age-Secret recovery +and out-of-band age-key updates ride the 5-minute reconcile instead. + +Sources: `gitprovider_controller.go`, `gittarget_controller.go`, +`watchrule_controller.go`, `clusterwatchrule_controller.go`, +`commitrequest_controller.go`. ### 1.3 Two structural consequences @@ -136,19 +146,24 @@ already exists; only the push edge is missing. A nudge would replace the poll. ### Gap B — control-plane status does not propagate to dependents **B1. WatchRule / ClusterWatchRule lag their GitTarget/GitProvider.** They watch -those deps with `GenerationChangedPredicate` (`watchrule_controller.go:465`, -`clusterwatchrule_controller.go:463`), so when a GitTarget flips to refused or a +those deps with `GenerationChangedPredicate`, so when a GitTarget flips to refused or a GitProvider goes `Ready=False` (status-only changes), the rules do **not** re-reconcile; their projected `GitTargetReady` dependency condition is stale for up -to `RequeueMediumInterval` (5 min). +to `RequeueSteadyInterval` (5 min). ### Gap C — missing dependency watches **C1. GitProvider does not watch its credentials Secret.** `SetupWithManager` is -`For(GitProvider)` only (`gitprovider_controller.go:428`). A rotated or corrected -Git credentials Secret is not picked up until the 10-minute requeue, so -`Ready=False (ConnectionFailed)` lingers after the fix. (GitTarget watches Secrets -for *encryption*, but no controller watches the *auth* Secret.) +`For(GitProvider)` only. A rotated or corrected Git credentials Secret is not picked up +until the 5-minute steady requeue, so `Ready=False (ConnectionFailed)` lingers that long. + +> **Superseded (2026-07-07, secret-value-retention):** not watching the auth Secret is now a +> deliberate least-privilege choice, not a gap to fix with a Secret watch. The control plane +> reads credential and age-key Secrets directly by name and relies on the 5-minute +> `RequeueSteadyInterval` to pick up rotations; the GitTarget's former encryption-Secret watch +> was also removed. Adding a Secret watch would reintroduce cluster-wide Secret retention. See +> [`future/secret-value-retention-plan.md`](../future/secret-value-retention-plan.md). The +> §5 recommendation to "watch the auth Secret" below is retained only as historical context. --- diff --git a/docs/future/scoped-rbac-least-privilege-plan.md b/docs/future/scoped-rbac-least-privilege-plan.md new file mode 100644 index 00000000..8adcfda0 --- /dev/null +++ b/docs/future/scoped-rbac-least-privilege-plan.md @@ -0,0 +1,253 @@ +# Scoped RBAC and least-privilege install + +> Status: design plan — **not started** (this is Track B). Its runtime prerequisites +> (stop retaining Secret values, drop control-plane Secret watches, 5-minute reconcile +> fallback) shipped in PR +> [#208](https://github.com/ConfigButler/gitops-reverser/pull/208), so the runtime no longer +> *needs* Secret `list/watch` for controller-owned inputs. What remains is the packaging/RBAC +> work below: the default install still *grants* the broad Secret permission and the +> `*/* get,list,watch` wildcard. +> Date: 2026-07-07 (updated 2026-07-08). +> Roadmap: [Secret handling roadmap](secret-handling-roadmap.md). +> Related issue: [#205](https://github.com/ConfigButler/gitops-reverser/issues/205). + +## Summary + +This plan is about what the operator's ServiceAccount is allowed to read. It is separate +from the Secret cache cleanup in +[secret-value-retention-plan.md](secret-value-retention-plan.md), which reduces what the +process keeps in memory. + +The key constraint is Kubernetes RBAC: permissions are additive. There is no deny rule and +no "all resources except Secrets" rule. The default install currently grants: + +- core `secrets get,list,watch,create,update,patch`; +- core `namespaces get,list,watch`; +- `*/* get,list,watch` for dynamic watching. + +While the wildcard exists, a namespaced Secret Role does not reduce the ServiceAccount's +cluster-wide Secret read permission. Real least privilege means running without the +wildcard and granting only the resources the install needs. + +## What the reporter wants + +The reported use case is narrower than the default product model: + +- use one Git credential Secret in one namespace; +- do not mirror Kubernetes Secrets into Git at all; +- avoid cluster-wide Secret reads; +- keep SOPS encryption one-way and avoid private material on the write path. + +The right answer is not only `--secret-namespaces`. That flag name also implies a scoped +value cache, which is not the target after the retention cleanup. The better plan is: + +1. ✅ stop retaining Secret values in the controller process (PR #208); +2. ✅ drop control-plane Secret watches and use a 5-minute reconcile fallback (PR #208); +3. ⬜ add a mode that refuses to mirror Secrets; +4. ⬜ support BYO-RBAC without the wildcard; +5. ⬜ generate the narrow RBAC needed for a specific install. + +## Current RBAC sources + +The generated ClusterRole is in +[config/rbac/role.yaml](../../config/rbac/role.yaml) and mirrored into the chart at +[charts/gitops-reverser/config/role.yaml](../../charts/gitops-reverser/config/role.yaml). + +Important grants: + +```yaml +- apiGroups: [""] + resources: ["namespaces"] + verbs: ["get", "list", "watch"] +- apiGroups: [""] + resources: ["secrets"] + verbs: ["create", "get", "list", "patch", "update", "watch"] +- apiGroups: ["*"] + resources: ["*"] + verbs: ["get", "list", "watch"] +``` + +Why they exist: + +- The wildcard comes from the dynamic watch manager: + [internal/watch/manager.go](../../internal/watch/manager.go#L33). It lets + `WatchRule` and `ClusterWatchRule` select many current and future resource types without + an RBAC redeploy. +- Secret reads support Git credentials, age encryption configuration, and signing keys. + Secret list/watch is only needed for the current Secret reactivity watch and should go + away in the retention plan. +- Secret writes support generated signing and age-key Secrets. +- namespace reads are needed for namespaced rule resolution. + +## Supported install modes + +### Default mode + +Keep the generated ClusterRole and wildcard. + +Good for convenience and broad discovery. Not least privilege. + +### Hardened static mode + +Operators set: + +```yaml +rbac: + create: false +serviceAccount: + create: false + name: existing-service-account +``` + +They bind that ServiceAccount to a hand-written or generated role set. This already works +at the packaging layer. What is missing is documentation and better runtime behavior when +a rule selects something not granted. + +### Future dynamic mode + +The operator reconciles its own Roles and RoleBindings from active rules. This is not a +near-term target. It requires RBAC write permission, privilege-escalation safeguards, and +an explicit opt-in. + +## Plan + +### 1. Document BYO-RBAC + +Write an operator-facing recipe for hardened static installs: + +- disable chart-created RBAC and ServiceAccount; +- create a ServiceAccount owned by the platform team; +- grant fixed controller permissions; +- grant variable watch permissions based on the install's rules; +- document the trade-off: no wildcard means no automatic access to new resource types. + +Fixed permissions include: + +- GitOps Reverser CRDs, `/status`, and `/finalizers`; +- leader-election leases only if leader election is enabled. It is not enabled today, and + the generated role does not currently grant `coordination.k8s.io/leases`; +- events if emitted; +- namespace reads for namespaced rule resolution; +- scoped Secret `get` for referenced Git credentials and age-key inputs; +- scoped Secret writes for generated signing and age-key material; +- Secret `list/watch` only if optional fast-rotation metadata watches are enabled. + +Variable permissions include: + +- one `get,list,watch` rule per selected GVR; +- Role when every selection is namespace-scoped; +- ClusterRole when the selected resource or selection scope is cluster-wide. + +### 2. Add "never mirror Secrets" + +Add an exclusion policy, for example: + +```text +--exclude-resources=secrets +``` + +or a structured counterpart to `SensitiveResourcePolicy`. + +Behavior: + +- Secrets are refused by the followability funnel; +- `WatchRule` or `ClusterWatchRule` status explains the refusal; +- the dynamic target watch path never opens a Secret watch; +- generated RBAC can omit mirrored-Secret read grants. + +This serves the reporter's strongest requirement: Git credentials remain controller input, +but cluster Secrets are not mirrored output. + +### 3. Make followability permission-aware + +The current funnel checks discovery-advertised verbs, not the ServiceAccount's actual +authorization: +[internal/typeset/funnel.go](../../internal/typeset/funnel.go#L16). + +With narrow RBAC, a type can look followable in discovery and then fail with `403 +Forbidden` when the dynamic watch opens. That should become a clean refusal. + +Implementation shape: + +- collect effective permissions for relevant namespaces and cluster scope; +- prefer `SelfSubjectRulesReview` for bulk rules; +- use `SelfSubjectAccessReview` as a fallback for ambiguous wildcard or aggregated cases; +- add permitted verbs to the type observation; +- fail the verbs requirement with a new reason such as `ReasonNotPermitted`; +- surface the reason on `WatchRule` and `ClusterWatchRule` status. + +This is not required to remove Secret values from the cache, but it is required to make +least-privilege installs pleasant and diagnosable. + +### 4. Add an RBAC generator + +Add a command or task that reads manifests or live cluster state and emits the narrow role +set for: + +- `GitProvider`; +- `GitTarget`; +- `WatchRule`; +- `ClusterWatchRule`. + +Output: + +- fixed controller grants; +- Secret input grants for referenced Git credentials and age-key Secrets; +- generated Secret write grants where `generateWhenMissing` or signing-key generation can + create/update Secrets; +- one watch grant per selected GVR; +- no `*/*` wildcard when `--no-wildcard` is selected. + +Start with manifest input. It is reviewable in CI and does not require cluster access. +Live-cluster input can come later. + +### 5. Use a 5-minute control-plane reconcile fallback + +The hardened default should not need Secret watches for controller-owned inputs. Instead, +control-plane controllers should use a common 5-minute periodic reconcile: + +- credential or age-key rotation is picked up within 5 minutes if no related work happens + sooner; +- branch workers still read credentials directly when work is happening; +- generated-key recovery and recipient annotation refresh can wait for the next GitTarget + reconcile; +- RBAC for controller-owned Secret inputs can drop `list/watch` and keep scoped `get`. + +This does not affect data-plane mirroring watches. Those still need `get,list,watch` for +the resources the user chooses to mirror. + +### 6. Optional: namespace-scope Secret metadata watches + +After [secret-value-retention-plan.md](secret-value-retention-plan.md) switches to direct +reads and a 5-minute reconcile fallback, metadata watches can be added back only for +installs that need faster Secret rotation. Avoid adding a public flag first if a generated +static namespace set is enough. + +```text +--secret-watch-namespaces=ns1,ns2 +``` + +If a flag is needed, prefer `--secret-watch-namespaces` over the issue's +`--secret-namespaces`: it says exactly what is scoped. This should not create a full +Secret value cache. It only scopes rotation reactivity. The matching RBAC still needs +normal Secret `get,list,watch` permissions in those namespaces. + +## What not to do + +- Do not claim metadata-only watch lowers RBAC. It does not. +- Do not reintroduce a namespace-scoped full Secret cache as the primary fix. +- Do not require a Secret metadata watch by default when a 5-minute reconcile is enough. +- Do not claim a namespaced Secret Role helps while `*/* get,list,watch` is still bound. +- Do not replace the default wildcard install for everyone. +- Do not use `resourceNames` as the main model for Secret watches; list/watch and rotation + behavior make that brittle. + +## Done + +This plan is complete when: + +- hardened static install docs exist; +- users can opt out of mirroring Secrets; +- missing watch permissions surface as rule status, not unexplained watch churn; +- an RBAC generator can produce a no-wildcard role set for a concrete install; +- public docs clearly distinguish RBAC blast radius from in-process Secret exposure. diff --git a/docs/future/secret-handling-roadmap.md b/docs/future/secret-handling-roadmap.md new file mode 100644 index 00000000..b27c3afb --- /dev/null +++ b/docs/future/secret-handling-roadmap.md @@ -0,0 +1,120 @@ +# Secret handling roadmap + +> Status: roadmap. **Track A (process/crypto exposure) is implemented; Track B (RBAC blast +> radius) is not started.** +> Date: 2026-07-07 (updated 2026-07-08). +> Related issue: [#205](https://github.com/ConfigButler/gitops-reverser/issues/205). +> Detailed plans: +> [Secret value retention](secret-value-retention-plan.md) and +> [Scoped RBAC and least-privilege install](scoped-rbac-least-privilege-plan.md). + +> **Progress.** PR [#208](https://github.com/ConfigButler/gitops-reverser/pull/208) +> delivered Track A: the controller no longer retains Secret values in memory, no longer +> watches Secrets on the control plane, reconciles on a unified 5-minute fallback, and keeps +> the SOPS write path public-recipient-only. This closes the **process-exposure** and +> **rotation-reaction** axes of the table below. The **RBAC blast radius** axis — the literal +> "limit secrets permissions" ask in #205 — is Track B and still open: the default install +> still grants `secrets get;list;watch` plus the dynamic-watch wildcard. + +## Why this exists + +The issue asks for `--secret-namespaces=ns1,ns2` so the operator stops watching every +Secret in the cluster. That request is directionally right, but it mixes three different +problems: + +| Axis | Question | Main fix | +|---|---|---| +| Process exposure | What Secret values does the controller hold in memory? | Direct Secret reads only | +| Rotation reaction | How fast do changed credentials or age keys take effect? | 5-minute reconcile fallback | +| RBAC blast radius | What is the ServiceAccount allowed to read? | Drop the wildcard and generate narrow RBAC | + +Those fixes should not be shipped as one large feature. The first two are small and give +the biggest immediate security win. RBAC narrowing is a separate install mode with a real +trade-off: no wildcard means no automatic access to newly selected resource types. + +The important update: control-plane Secret watches are not required. Mirroring still needs +data-plane watches for selected resources, and controllers should still watch our own CRDs. +But Git credentials and age-key inputs can be read directly and refreshed by a 5-minute +periodic reconcile instead of a Secret informer. + +## Current state + +The operator has three Secret paths today: + +- `GitTarget` registers a full-object Secret watch to react to generated age Secret + changes. With the default controller-runtime cache, that means every Secret value in + every namespace can be held in memory. +- `GitProvider`, branch workers, encryption setup, and signing-key helpers read specific + Secrets through the cached client. +- Mirrored Secrets selected by a `WatchRule` or `ClusterWatchRule` use the dynamic watch + path. That path is about user-selected resources, not the controller's own credential + inputs. + +The default chart also grants `*/* get,list,watch` for dynamic watching. Because +Kubernetes RBAC is additive, a namespaced Secret Role does not reduce the ServiceAccount's +Secret read permission while that wildcard remains. + +## Decision + +Treat this as two tracks. + +**Track A: stop retaining Secret values.** This is the first implementation track. It +keeps the default broad install behavior, but changes the process footprint: + +- delete control-plane Secret watches instead of converting them to metadata; +- bypass the cache for typed Secret reads; +- read referenced Secret values directly by name; +- standardize control-plane periodic reconciles at 5 minutes; +- remove private age identities from the SOPS encrypt path. + +This solves the immediate "the process caches every Secret value forever" problem and +removes the need for Secret list/watch on controller-owned inputs in the default mode. + +**Track B: support narrow installs.** This is the install/RBAC track: + +- document bring-your-own RBAC; +- add a "never mirror Secrets" policy; +- make followability permission-aware so missing grants become status refusals rather than + runtime watch churn; +- add a generator for minimal Roles and ClusterRoles. + +This is what reduces what the ServiceAccount is allowed to read. + +## Recommended order + +Steps 1–3 are **done** (PR +[#208](https://github.com/ConfigButler/gitops-reverser/pull/208)); steps 4–6 are Track B and +**not started**; step 7 is optional and not needed while the 5-minute fallback suffices. + +1. ✅ **Stop value caching and drop Secret dependency watches.** + Implemented the small controller/client changes in + [secret-value-retention-plan.md](secret-value-retention-plan.md). This makes Secret + `list/watch` unnecessary for controller-owned inputs; packaging cleanup follows in the + RBAC track. +2. ✅ **Remove private age identity handling from the encrypt path.** + SOPS encryption is now public-recipient only. Small deletion, strong security payoff. +3. ✅ **Set control-plane reconcile fallback to 5 minutes.** + `RequeueSteadyInterval` is now the common periodic reconcile cadence for control-plane + controllers after removing Secret watches. +4. ⬜ **Add `--exclude-resources=secrets` or equivalent.** + This lets an install say: Git credentials are controller input; Kubernetes Secrets are + not mirrored output. +5. ⬜ **Document BYO-RBAC and add permission-aware followability.** + This makes hand-narrowed RBAC predictable. +6. ⬜ **Add an RBAC generator.** + Generate the narrow role set from `GitProvider`, `GitTarget`, `WatchRule`, and + `ClusterWatchRule` manifests. +7. ⬜ **Optional: add Secret metadata watches for faster rotation.** + Do this only if the 5-minute fallback is not responsive enough. Prefer a namespace set + derived from manifests or generated Helm values before adding a runtime flag. + +## Non-goals + +- Do not replace the default wildcard install for everyone. The default remains convenient. +- Do not reintroduce a full Secret value informer just to support a namespace flag. +- Do not add a Secret metadata watch by default; direct reads plus a 5-minute reconcile are + the simpler baseline. +- Do not add a namespace flag before proving generated or inferred configuration is not + enough. +- Do not add a long-lived in-process Secret value TTL cache unless an install measures API + pressure and explicitly opts in. diff --git a/docs/future/secret-value-retention-plan.md b/docs/future/secret-value-retention-plan.md new file mode 100644 index 00000000..f5db818e --- /dev/null +++ b/docs/future/secret-value-retention-plan.md @@ -0,0 +1,265 @@ +# Stop retaining Secret values + +> Status: **implemented** (steps 1–6) in PR +> [#208](https://github.com/ConfigButler/gitops-reverser/pull/208). Step 7 (publish +> recipients as metadata) remains future work. +> Date: 2026-07-07 (implemented 2026-07-08). +> Roadmap: [Secret handling roadmap](secret-handling-roadmap.md). +> Scope: remove full Secret values from the controller-runtime cache, keep direct reads +> fresh, drop control-plane Secret watches, and remove private age identities from the SOPS +> encrypt path. + +> **What shipped.** The manager sets `Client.Cache.DisableFor: corev1.Secret`, so typed +> Secret reads bypass the cache and no informer retains Secret values. The GitTarget +> control-plane Secret watch was removed. The short/medium/long requeue intervals collapsed +> into one 5-minute `RequeueSteadyInterval`. The SOPS write path carries public age +> recipients only — no `SOPS_AGE_KEY_FILE`, no identity temp file, no blanket Secret-data +> env. And (step 5) the branch worker resolves Git credentials once per push cycle instead +> of per commit. RBAC narrowing is deliberately **not** part of this — it stays in +> [scoped-rbac-least-privilege-plan.md](scoped-rbac-least-privilege-plan.md). + +## Summary + +This plan is the first implementation slice of the Secret handling work. It does not try +to solve RBAC. It solves the process-level problem: the controller should not keep every +Secret value in the cluster in memory. + +The target state: + +- no controller-runtime informer stores full `corev1.Secret` objects; +- no controller-runtime Secret informer is started for control-plane dependencies; +- typed Secret reads go directly to the API server; +- external Secret changes are noticed by direct reads and a 5-minute reconcile fallback; +- SOPS encryption uses public age recipients only; +- no private age identity is written to disk or passed to the `sops` process. + +TTL is a fallback load optimization, not the design. + +## Current problem + +The manager is created without cache options: +[cmd/main.go](../../cmd/main.go#L841). Any informer started by the shared cache is +cluster-wide unless configured otherwise. + +`GitTarget.SetupWithManager` registers a full Secret watch: +[gittarget_controller.go](../../internal/controller/gittarget_controller.go#L965). +The map function only needs the Secret name and namespace, but the current watch stores +full Secret objects, including `data`. + +Several code paths also read specific Secrets through the cached client: + +- GitProvider credentials: + [gitprovider_controller.go](../../internal/controller/gitprovider_controller.go#L262) +- branch-worker credentials: + [branch_worker.go](../../internal/git/branch_worker.go#L504) +- age encryption Secret: + [encryption.go](../../internal/git/encryption.go#L173) +- signing keys: + [helpers.go](../../internal/git/helpers.go#L37) + +Result: one reactivity watch can make the process retain cleartext Secret values for the +whole cluster, even though the operator only needs a small number of named Secrets. + +## Target model + +Use one default access mode for controller-owned Secret inputs: + +- **Read values directly** only when code actually needs a specific Secret. +- **Reconcile every 5 minutes** so out-of-band Secret changes are picked up without a + Secret watch. + +```mermaid +flowchart TB + Timer["5-minute reconcile fallback"] + Reconcile["GitProvider / GitTarget / branch worker"] + DirectRead["direct Secret GET
specific name + namespace"] + API["kube-apiserver"] + + Timer --> Reconcile + Reconcile --> DirectRead + DirectRead --> API +``` + +This gives up instant Secret-rotation reaction in exchange for a cleaner default: no +Secret list/watch, no metadata informer, no namespace flag, and no Secret values retained +by the controller cache. Mirroring watches are separate and remain required for selected +resources. + +## Implementation steps + +### 1. Delete the GitTarget Secret watch + +Remove the control-plane Secret watch from `GitTarget.SetupWithManager`: + +```go +// internal/controller/gittarget_controller.go +- Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.encryptionSecretToGitTargets)). +``` + +Do not replace it with `WatchesMetadata` by default. Generated age Secret recovery and +out-of-band age-key updates can wait for the 5-minute GitTarget reconcile. That is a +better least-privilege baseline than keeping a cluster-wide Secret metadata watch. + +### 2. Disable cached reads for typed Secrets + +Add `Client.Cache.DisableFor` in `newManager`: + +```go +mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ + Scheme: scheme, + Metrics: metricsOptions, + HealthProbeBindAddress: probeAddr, + WebhookServer: webhookServer, + Client: client.Options{ + Cache: &client.CacheOptions{ + DisableFor: []client.Object{&corev1.Secret{}}, + }, + }, +}) +``` + +This keeps typed Secret reads live and prevents a typed Secret `Get` from starting a +full-object Secret cache. + +### 3. Keep GitProvider credential rotation pull-based + +Do not add a GitProvider Secret watch by default. `GitProvider` should revalidate on its +own spec changes and on the common 5-minute periodic reconcile. + +Branch workers also read credentials directly when they need them, so writes can pick up +new credentials on the next operation. GitProvider status may lag until the next 5-minute +validation pass, which is acceptable for the default least-privilege mode. + +### 4. Standardize control-plane reconcile fallback at 5 minutes + +After removing Secret watches, use 5 minutes as the common control-plane periodic +reconcile cadence. This replaces the old split where fast paths used 2 minutes, some +Secret/config failures used 5 minutes, and steady validation could wait up to 10 minutes. +In code terms, collapse the control-plane uses of the short, medium, and long requeue +intervals to a 5-minute steady fallback for GitProvider, GitTarget, WatchRule, and +ClusterWatchRule reconcilers. + +The intent is simple to explain: + +- no Secret watch by default; +- external Secret changes become visible within 5 minutes through reconcile; +- direct reads still use the freshest Secret value when work is already happening. + +### 5. Remove redundant credential reads where practical + +The branch worker reads the same credentials Secret several times during a push cycle. +Prefer a structural fix: + +- resolve auth once at the start of a flush or push operation; +- pass the resolved auth through clone, fetch, push, force-push, and retry code; +- keep direct reads as the default. + +Only add a short TTL memo if an install measures API pressure. If added, keep it opt-in +and off by default. A long TTL recreates stale-credential behavior. + +Direct reads are acceptable because the operator reads a small set of named Secrets, not +every object during every reconcile. A direct `GET` is real API/etcd work, so avoid tight +loops; the structural read-once change above is the preferred load fix. + +### 6. Stop passing private age identities to SOPS + +The write path only encrypts. It does not decrypt. + +Evidence in the current code: + +- `Encryptor` only exposes `Encrypt`: + [encryption.go](../../internal/git/encryption.go#L35) +- `SOPSEncryptor.Encrypt` runs `sops --encrypt`: + [sops_encryptor.go](../../internal/git/sops_encryptor.go#L43) +- encrypted-content reuse compares plaintext digest plus Kubernetes identity markers and + never decrypts Git content: + [content_writer.go](../../internal/git/content_writer.go#L87) + +age encryption needs public recipients. The private age identity is for decryption, and +this operator does not decrypt. So remove it from the hot path: + +- drop `AgeIdentities` from `ResolvedEncryptionConfig`; +- stop returning `secretIdentities` from `ResolveTargetEncryption`; +- remove `writeAgeIdentityFile`, `SOPS_AGE_KEY_FILE`, and the age identity temp-file path; +- remove blanket `toSOPSEnvironment(secret.Data)` for the age path. + +After this step, the operator may still read a BYO private age key to derive a recipient, +but it does not write the private key to disk or pass it to `sops`. + +### 7. Later: publish recipients as metadata + +This is useful but larger than the first slice. + +For generated age keys, the controller already writes +`configbutler.ai/age-recipient`. For BYO age-key Secrets, the controller can derive the +recipient once and publish a controller-owned annotation such as +`configbutler.ai/age-recipients`. + +Then the write path can read recipients from spec plus metadata and avoid reading private +age-key `data` except during bootstrap or rotation. + +Security rule: never blindly trust a user-written recipient annotation. If an attacker can +set the annotation to their public key, mirrored Secrets can be encrypted to a key they +control. The controller must derive and overwrite the annotation from the actual key +material, or the GitTarget spec must carry the public recipient directly. + +## RBAC impact + +The default plan no longer needs `secrets list/watch` for controller-owned credential and +age-key inputs. It still needs scoped `secrets get` for referenced input Secrets, plus +scoped write verbs where the operator generates signing or age-key Secrets. + +The runtime change can land before the generated default RBAC is narrowed. The important +contract is that `list/watch` becomes unnecessary for controller-owned Secret inputs, so +the least-privilege packaging can remove those verbs. + +If optional Secret metadata watches are added later, they use the normal Secret endpoint +and require `secrets get,list,watch` in their scope. Kubernetes RBAC does not provide a +metadata-only Secret permission. + +RBAC narrowing is covered by +[scoped-rbac-least-privilege-plan.md](scoped-rbac-least-privilege-plan.md). + +## Optional fast-rotation mode + +Add Secret metadata watches only if 5-minute rotation reaction is not good enough: + +- first preference: no Secret watch; +- second preference: have the RBAC generator or Helm values derive the static namespace set + from `GitProvider` and `GitTarget` manifests; +- last resort: add an explicit `--secret-watch-namespaces=ns1,ns2` flag for metadata + reactivity. + +If the flag exists, use the more precise name above rather than `--secret-namespaces`. +The original issue name sounds like it scopes a value cache. This plan scopes metadata +reactivity only and must not reintroduce a full Secret value cache. + +## Tests + +- Unit: Secret-to-GitProvider and Secret-to-GitTarget map functions enqueue only matching + objects only if optional metadata watch mode is implemented. +- Unit: SOPS environment no longer includes private age identity material. +- Envtest: typed Secret reads see fresh data after a direct Secret update, proving they do + not come from a stale cache. +- Unit or controller test: control-plane steady reconciles use a 5-minute interval. +- E2E: rotating a Git credential Secret is picked up by direct reads or by GitProvider + status within 5 minutes. +- E2E: rotating an age key Secret is picked up by direct reads or by GitTarget reconcile + within 5 minutes. + +## Done + +All of the following are met as of PR +[#208](https://github.com/ConfigButler/gitops-reverser/pull/208): + +- [x] no full-object Secret informer is created by the manager; +- [x] no Secret metadata informer is created by default; +- [x] typed Secret reads bypass the cache; +- [x] control-plane controllers use a 5-minute fallback reconcile for external changes; +- [x] SOPS encryption no longer receives private age identities; +- [x] docs clearly state the default mode needs Secret `get` for inputs, not Secret + `list/watch` (see [../security-model.md](../security-model.md)). + +Note this is a **runtime** milestone: the default install still *grants* `secrets +get;list;watch` — narrowing that grant is tracked separately in +[scoped-rbac-least-privilege-plan.md](scoped-rbac-least-privilege-plan.md). diff --git a/docs/security-model.md b/docs/security-model.md index dc287333..8e9c0b4d 100644 --- a/docs/security-model.md +++ b/docs/security-model.md @@ -10,7 +10,17 @@ GitOps Reverser writes the live state of watched resource types into Git. To do followed, but the controller needs read (get/list/watch) access to those types to materialize them. Broad WatchRules imply broad read access. - **Read referenced Secrets.** It reads the Git credentials Secret (and, when encryption is - configured, the SOPS/age key Secret) named by a GitProvider/GitTarget. + configured, the SOPS/age key Secret) named by a GitProvider/GitTarget. At **runtime** the + control-plane code paths read these controller-owned input Secrets **directly by name** (`get`): + they no longer `list` or `watch` Secrets and never cache Secret values in memory. Out-of-band + credential or age-key rotations are picked up on the direct read the next time work happens, and + at the latest by the 5-minute periodic reconcile. **This is a runtime-behavior change, not (yet) + an RBAC change:** the default install still *grants* `secrets get;list;watch` (and the + dynamic-watch wildcard), so the ServiceAccount's effective Secret permission is unchanged until + the separate RBAC-narrowing track lands. Mirrored Secrets selected by a WatchRule are a separate + concern and still use the watched-resource read path above. See + [`future/secret-value-retention-plan.md`](future/secret-value-retention-plan.md) and + [`future/scoped-rbac-least-privilege-plan.md`](future/scoped-rbac-least-privilege-plan.md). - **Receive audit events.** The kube-apiserver audit webhook posts events to the controller's audit ingress. Those events carry object metadata and, for some resources, request/response bodies. @@ -31,8 +41,10 @@ The controller does not need write access to watched resources. Its only write t Without encryption, a watched `Secret` is committed as-is (its data is plain in the repository). With SOPS + age (`GitTarget.spec.encryption`), Secret values are encrypted before commit using the age -recipients, and the private key never leaves the cluster. So: only watch `Secret` types you intend -to commit, and prefer encryption. Secret-shaped custom resource types can opt into the same +recipients, and the private key never leaves the cluster. Because the write path only encrypts, it +uses **public age recipients only**: the private age identity is never written to disk or passed to +the `sops` process, even when the recipient is derived from a cluster age-key Secret. So: only watch +`Secret` types you intend to commit, and prefer encryption. Secret-shaped custom resource types can opt into the same encryption path at controller startup. See [`sops-age-guide.md`](sops-age-guide.md). ## Git credentials Secret shape diff --git a/internal/controller/clusterwatchrule_controller.go b/internal/controller/clusterwatchrule_controller.go index 7ba14db8..26c6fa1a 100644 --- a/internal/controller/clusterwatchrule_controller.go +++ b/internal/controller/clusterwatchrule_controller.go @@ -238,7 +238,7 @@ func (r *ClusterWatchRuleReconciler) setReadyAndUpdateStatusWithTarget( return ctrl.Result{}, err } if conditionIsFalse(clusterRule.Status.Conditions, ConditionTypeResourcesResolved) { - return ctrl.Result{RequeueAfter: RequeueShortInterval}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } if !conditionIsTrue(clusterRule.Status.Conditions, ConditionTypeGitTargetReady) { return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil @@ -246,7 +246,7 @@ func (r *ClusterWatchRuleReconciler) setReadyAndUpdateStatusWithTarget( if !conditionIsTrue(clusterRule.Status.Conditions, ConditionTypeStreamsRunning) { return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil } - return ctrl.Result{RequeueAfter: RequeueMediumInterval}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } // setCondition sets or updates the Ready condition. @@ -374,7 +374,7 @@ func (r *ClusterWatchRuleReconciler) updateStatusAndRequeue( if err := r.updateStatusWithRetry(ctx, clusterRule); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: RequeueShortInterval}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } // updateStatusWithRetry updates the status with retry logic to handle race conditions. diff --git a/internal/controller/constants.go b/internal/controller/constants.go index b155426c..e5628492 100644 --- a/internal/controller/constants.go +++ b/internal/controller/constants.go @@ -56,12 +56,14 @@ const ( // cluster snapshot has been successfully committed to Git. MsgSnapshotCompleted = "Initial snapshot reconciliation completed" - // RequeueShortInterval is the requeue interval for transient errors. - RequeueShortInterval = 2 * time.Minute - // RequeueMediumInterval is the requeue interval for auth/secret errors. - RequeueMediumInterval = 5 * time.Minute - // RequeueLongInterval is the requeue interval for periodic revalidation. - RequeueLongInterval = 10 * time.Minute + // RequeueSteadyInterval is the unified control-plane periodic reconcile fallback. + // The control plane no longer watches Secrets (docs/future/secret-value-retention-plan.md), + // so out-of-band credential and age-key changes are picked up on this steady cadence + // instead of via a Secret informer. It replaces the former split of a 2-minute + // transient-retry, a 5-minute auth/secret, and a 10-minute revalidation interval with + // a single 5-minute fallback for the GitProvider, GitTarget, WatchRule, and + // ClusterWatchRule reconcilers. The fast stream-settle loop below is separate. + RequeueSteadyInterval = 5 * time.Minute // RequeueStreamSettleInterval is the requeue interval while a Ready GitTarget still // has streams pending replay completion. Stream status is computed during reconcile, so // this keeps status.streams fresh while watches converge. diff --git a/internal/controller/dependency_watches_test.go b/internal/controller/dependency_watches_test.go index b3fbcce1..86fb9fac 100644 --- a/internal/controller/dependency_watches_test.go +++ b/internal/controller/dependency_watches_test.go @@ -7,6 +7,7 @@ import ( "errors" "sort" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -385,3 +386,11 @@ func TestDependencyMapFunctionsTolerateListErrors(t *testing.T) { assert.Nil(t, (&ClusterWatchRuleReconciler{Client: client}).gitTargetToClusterWatchRules(ctx, target)) assert.Nil(t, (&ClusterWatchRuleReconciler{Client: client}).gitProviderToClusterWatchRules(ctx, provider)) } + +// TestRequeueSteadyIntervalIsFiveMinutes locks the unified control-plane periodic reconcile +// fallback at 5 minutes. After the secret-value-retention change the control plane no longer +// watches Secrets, so out-of-band credential and age-key rotations are picked up on this steady +// cadence rather than by a Secret informer. See docs/future/secret-value-retention-plan.md. +func TestRequeueSteadyIntervalIsFiveMinutes(t *testing.T) { + assert.Equal(t, 5*time.Minute, RequeueSteadyInterval) +} diff --git a/internal/controller/gitprovider_controller.go b/internal/controller/gitprovider_controller.go index 0be37ea8..3ac2146f 100644 --- a/internal/controller/gitprovider_controller.go +++ b/internal/controller/gitprovider_controller.go @@ -8,7 +8,6 @@ import ( "fmt" "strings" "sync" - "time" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -94,7 +93,7 @@ func (r *GitProviderReconciler) reconcileGitProvider( r.setProgressingConditions(gitProvider, ReasonChecking, "Validating repository connectivity...") if err := r.validateCommitConfiguration(gitProvider); err != nil { r.setStalledConditions(gitProvider, ReasonCommitConfigInvalid, err.Error()) - result, _ := r.updateStatusAndRequeue(ctx, gitProvider, RequeueLongInterval) + result, _ := r.updateStatusAndRequeue(ctx, gitProvider) return result, nil } @@ -109,14 +108,14 @@ func (r *GitProviderReconciler) reconcileGitProvider( } r.setStalledConditions(gitProvider, reason, err.Error()) - result, _ := r.updateStatusAndRequeue(ctx, gitProvider, RequeueMediumInterval) + result, _ := r.updateStatusAndRequeue(ctx, gitProvider) return result, nil } // Fetch and validate secret secret, shouldReturn := r.fetchAndValidateSecret(ctx, log, gitProvider) if shouldReturn { - result, _ := r.updateStatusAndRequeue(ctx, gitProvider, RequeueMediumInterval) + result, _ := r.updateStatusAndRequeue(ctx, gitProvider) return result, nil } @@ -190,7 +189,7 @@ func (r *GitProviderReconciler) getAuthFromSecret( secretName := gitProvider.Spec.SecretRef.Name r.setStalledConditions(gitProvider, ReasonSecretMalformed, fmt.Sprintf("Secret '%s' malformed: %v", secretName, err)) - result, _ := r.updateStatusAndRequeue(ctx, gitProvider, RequeueShortInterval) + result, _ := r.updateStatusAndRequeue(ctx, gitProvider) return nil, result, true } @@ -225,7 +224,7 @@ func (r *GitProviderReconciler) validateAndUpdateStatus( "url", gitProvider.Spec.URL) r.setStalledConditions(gitProvider, ReasonConnectionFailed, fmt.Sprintf("Failed to connect to repository: %v", err)) - return r.updateStatusAndRequeue(ctx, gitProvider, RequeueShortInterval) + return r.updateStatusAndRequeue(ctx, gitProvider) } log.V(1).Info("Repository connectivity validated successfully", "branchCount", branchCount) @@ -246,8 +245,8 @@ func (r *GitProviderReconciler) validateAndUpdateStatus( "namespace", gitProvider.Namespace, "branchCount", branchCount) }) - log.V(1).Info("Status update completed successfully, scheduling requeue", "requeueAfter", RequeueLongInterval) - return ctrl.Result{RequeueAfter: RequeueLongInterval}, nil + log.V(1).Info("Status update completed successfully, scheduling requeue", "requeueAfter", RequeueSteadyInterval) + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } // fetchSecret retrieves the secret containing Git credentials. @@ -385,16 +384,17 @@ func (r *GitProviderReconciler) setCondition( ) } -// updateStatusAndRequeue updates the status and returns requeue result. +// updateStatusAndRequeue updates the status and requeues on the unified control-plane steady +// interval. The control plane no longer watches Secrets, so every status outcome falls back to +// this single cadence; see docs/future/secret-value-retention-plan.md. func (r *GitProviderReconciler) updateStatusAndRequeue( ctx context.Context, gitProvider *configbutleraiv1alpha3.GitProvider, - requeueAfter time.Duration, ) (ctrl.Result, error) { if err := r.updateStatusWithRetry(ctx, gitProvider); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: requeueAfter}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } // updateStatusWithRetry updates the status with retry logic to handle race conditions. diff --git a/internal/controller/gitprovider_controller_test.go b/internal/controller/gitprovider_controller_test.go index 9aea77e3..ef6584e4 100644 --- a/internal/controller/gitprovider_controller_test.go +++ b/internal/controller/gitprovider_controller_test.go @@ -400,7 +400,9 @@ var _ = Describe("GitProvider Controller", func() { }) Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(time.Minute * 2)) + // Control-plane reconciles now share one steady fallback cadence; see + // docs/future/secret-value-retention-plan.md. + Expect(result.RequeueAfter).To(Equal(RequeueSteadyInterval)) // Verify the resource was updated with failure condition updatedProvider := &configbutleraiv1alpha3.GitProvider{} @@ -460,7 +462,7 @@ var _ = Describe("GitProvider Controller", func() { }) Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(time.Minute * 10)) + Expect(result.RequeueAfter).To(Equal(RequeueSteadyInterval)) updatedProvider := &configbutleraiv1alpha3.GitProvider{} err = k8sClient.Get( diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index a26703fe..50b5a531 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -145,7 +145,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if err := r.updateStatusWithRetry(ctx, &target); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: RequeueShortInterval}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } encryptionReady, encryptionMessage, encryptionRequeueAfter := r.evaluateEncryptionGate(ctx, &target, log) @@ -178,7 +178,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if err := r.updateStatusWithRetry(ctx, &target); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: RequeueShortInterval}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } // Ensure watch-first data-plane streams exist, then project source and target readiness @@ -220,7 +220,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if streamsSettling { return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil } - return ctrl.Result{RequeueAfter: RequeueLongInterval}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } func (r *GitTargetReconciler) evaluateValidatedGate( @@ -292,7 +292,7 @@ func (r *GitTargetReconciler) evaluateEncryptionGate( reason = GitTargetReasonMissingSecret } r.setCondition(target, GitTargetConditionEncryptionConfigured, metav1.ConditionFalse, reason, err.Error()) - return false, fmt.Sprintf("EncryptionConfigured gate failed: %s", reason), RequeueMediumInterval + return false, fmt.Sprintf("EncryptionConfigured gate failed: %s", reason), RequeueSteadyInterval } if _, err := git.ResolveTargetEncryption(ctx, r.Client, target); err != nil { reason := GitTargetReasonInvalidConfig @@ -300,7 +300,7 @@ func (r *GitTargetReconciler) evaluateEncryptionGate( reason = GitTargetReasonMissingSecret } r.setCondition(target, GitTargetConditionEncryptionConfigured, metav1.ConditionFalse, reason, err.Error()) - return false, fmt.Sprintf("EncryptionConfigured gate failed: %s", reason), RequeueMediumInterval + return false, fmt.Sprintf("EncryptionConfigured gate failed: %s", reason), RequeueSteadyInterval } r.setCondition( @@ -591,7 +591,7 @@ func (r *GitTargetReconciler) validateProviderAndBranch( if err := r.Get(ctx, gpKey, &gp); err != nil { if apierrors.IsNotFound(err) { msg := fmt.Sprintf("Referenced GitProvider '%s/%s' not found", providerNS, target.Spec.ProviderRef.Name) - result := ctrl.Result{RequeueAfter: RequeueShortInterval} + result := ctrl.Result{RequeueAfter: RequeueSteadyInterval} return false, msg, GitTargetReasonProviderNotFound, &result, nil } return false, "", "", nil, err @@ -617,7 +617,7 @@ func (r *GitTargetReconciler) validateProviderAndBranch( providerNS, target.Spec.ProviderRef.Name, ) - result := ctrl.Result{RequeueAfter: RequeueShortInterval} + result := ctrl.Result{RequeueAfter: RequeueSteadyInterval} return false, msg, GitTargetReasonBranchNotAllowed, &result, nil } @@ -684,7 +684,7 @@ func (r *GitTargetReconciler) checkForConflicts( target.Spec.Branch, ) } - return true, msg, GitTargetReasonTargetConflict, ctrl.Result{RequeueAfter: RequeueShortInterval}, nil + return true, msg, GitTargetReasonTargetConflict, ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } } @@ -962,7 +962,11 @@ func (r *GitTargetReconciler) updateStatusWithRetry( func (r *GitTargetReconciler) SetupWithManager(mgr ctrl.Manager) error { b := ctrl.NewControllerManagedBy(mgr). For(&configbutleraiv1alpha3.GitTarget{}). - Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.encryptionSecretToGitTargets)). + // No control-plane Secret watch. Reacting to age-key Secret changes with a + // full-object Secret watch made the process retain every Secret value in the + // cluster. Generated-age-Secret recovery and out-of-band age-key updates are + // picked up by the periodic reconcile (RequeueSteadyInterval) instead. + // See docs/future/secret-value-retention-plan.md. // GenerationChangedPredicate keeps this watch reacting to a freshly // applied or spec-changed GitProvider while ignoring the status-only // updates the controllers write themselves — without it every provider @@ -990,7 +994,7 @@ func (r *GitTargetReconciler) SetupWithManager(mgr ctrl.Manager) error { Named("gittarget") // React to a data-plane GitPath acceptance TRANSITION (refused/recovered) so GitPathAccepted - // is re-projected within one reconcile instead of lagging up to RequeueLongInterval (10m). + // is re-projected within one reconcile instead of lagging up to RequeueSteadyInterval (5m). // The watch manager records acceptance asynchronously and pushes a GenericEvent here. See // docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md. if r.EventRouter != nil && r.EventRouter.WatchManager != nil { @@ -1029,36 +1033,10 @@ func (r *GitTargetReconciler) clusterWatchRuleToGitTarget( }}} } -// encryptionSecretToGitTargets maps a Secret event to the GitTargets that reference it as their -// encryption secret, so the controller re-reconciles and recreates a deleted/emptied secret. -func (r *GitTargetReconciler) encryptionSecretToGitTargets( - ctx context.Context, - obj client.Object, -) []ctrlreconcile.Request { - var targets configbutleraiv1alpha3.GitTargetList - if err := r.List(ctx, &targets, client.InNamespace(obj.GetNamespace())); err != nil { - return nil - } - - var requests []ctrlreconcile.Request - for i := range targets.Items { - t := &targets.Items[i] - if !shouldGenerateAgeKey(t) { - continue - } - if t.Spec.Encryption.SecretRef.Name == obj.GetName() { - requests = append(requests, ctrlreconcile.Request{ - NamespacedName: k8stypes.NamespacedName{Name: t.Name, Namespace: t.Namespace}, - }) - } - } - return requests -} - // gitProviderToGitTargets maps a GitProvider event to every GitTarget in the // same namespace that references it, so a freshly-arrived provider re-enqueues // any dependents currently stuck on ProviderNotFound instead of waiting for the -// periodic RequeueShortInterval. +// periodic RequeueSteadyInterval. func (r *GitTargetReconciler) gitProviderToGitTargets( ctx context.Context, obj client.Object, diff --git a/internal/controller/watchrule_controller.go b/internal/controller/watchrule_controller.go index e7992706..b22730f8 100644 --- a/internal/controller/watchrule_controller.go +++ b/internal/controller/watchrule_controller.go @@ -5,7 +5,6 @@ package controller import ( "context" "fmt" - "time" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -141,7 +140,7 @@ func (r *WatchRuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( "Target.name must be specified", ) r.setRuleStalled(&watchRule, WatchRuleReasonGitDestinationInvalid, "Target.name must be specified") - return r.updateStatusAndRequeue(ctx, &watchRule, RequeueShortInterval) + return r.updateStatusAndRequeue(ctx, &watchRule) } return r.reconcileWatchRuleViaTarget(ctx, &watchRule) } @@ -182,7 +181,7 @@ func (r *WatchRuleReconciler) reconcileWatchRuleViaTarget( "Referenced GitTarget not found", ) r.setRuleStalled(watchRule, WatchRuleReasonGitTargetNotFound, "Referenced GitTarget not found") - return r.updateStatusAndRequeue(ctx, watchRule, RequeueShortInterval) + return r.updateStatusAndRequeue(ctx, watchRule) } r.setGitTargetReadyCondition(watchRule, target) @@ -215,7 +214,7 @@ func (r *WatchRuleReconciler) reconcileWatchRuleViaTarget( "Referenced GitProvider not found", ) r.setRuleStalled(watchRule, WatchRuleReasonGitProviderNotFound, "Referenced GitProvider not found") - return r.updateStatusAndRequeue(ctx, watchRule, RequeueShortInterval) + return r.updateStatusAndRequeue(ctx, watchRule) } // Ready check (GitProvider doesn't have status conditions yet in my implementation? I added them) @@ -263,7 +262,7 @@ func (r *WatchRuleReconciler) setReadyAndUpdateStatusWithTarget( return ctrl.Result{}, err } if conditionIsFalse(watchRule.Status.Conditions, ConditionTypeResourcesResolved) { - return ctrl.Result{RequeueAfter: RequeueShortInterval}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } if !conditionIsTrue(watchRule.Status.Conditions, ConditionTypeGitTargetReady) { return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil @@ -271,7 +270,7 @@ func (r *WatchRuleReconciler) setReadyAndUpdateStatusWithTarget( if !conditionIsTrue(watchRule.Status.Conditions, ConditionTypeStreamsRunning) { return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil } - return ctrl.Result{RequeueAfter: RequeueMediumInterval}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } // setCondition sets or updates the Ready condition. @@ -370,13 +369,15 @@ func conditionIsFalse(conditions []metav1.Condition, conditionType string) bool return false } -// updateStatusAndRequeue updates the status and returns requeue result. -func (r *WatchRuleReconciler) updateStatusAndRequeue( //nolint:lll // Function signature - ctx context.Context, watchRule *configbutleraiv1alpha3.WatchRule, requeueAfter time.Duration) (ctrl.Result, error) { +// updateStatusAndRequeue updates the status and requeues on the unified control-plane steady +// interval. The control plane no longer watches Secrets, so every status outcome falls back to +// this single cadence; see docs/future/secret-value-retention-plan.md. +func (r *WatchRuleReconciler) updateStatusAndRequeue( + ctx context.Context, watchRule *configbutleraiv1alpha3.WatchRule) (ctrl.Result, error) { if err := r.updateStatusWithRetry(ctx, watchRule); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: requeueAfter}, nil + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } // updateStatusWithRetry updates the status with retry logic to handle race conditions diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index acc69831..eec060d8 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -1052,13 +1052,17 @@ func (w *BranchWorker) commitPendingWrites(pendingWrites []PendingWrite, hasPend return fmt.Errorf("get GitProvider: %w", err) } - auth, err := getAuthFromSecret(w.ctx, w.Client, provider, w.sshHostKeys) - if err != nil { - return fmt.Errorf("resolve auth: %w", err) - } - repoPath := w.repoPathForRemote(provider.Spec.URL) if !hasPendingCommits { + // Resolve credentials only on the first commit of a push cycle — the one branch + // that touches the remote (PrepareBranch fetches the tip). Later commits in the + // same cycle build on the local repo and never use auth, so re-reading the + // credentials Secret here would be a wasted API GET per commit now that the + // Secret cache is disabled. See docs/future/secret-value-retention-plan.md §5. + auth, err := getAuthFromSecret(w.ctx, w.Client, provider, w.sshHostKeys) + if err != nil { + return fmt.Errorf("resolve auth: %w", err) + } pullReport, err := PrepareBranch(w.ctx, provider.Spec.URL, repoPath, w.Branch, auth) if err != nil { return fmt.Errorf("prepare repository: %w", err) diff --git a/internal/git/branch_worker_credread_test.go b/internal/git/branch_worker_credread_test.go new file mode 100644 index 00000000..39041332 --- /dev/null +++ b/internal/git/branch_worker_credread_test.go @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "path/filepath" + "sync/atomic" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// TestCommitPendingWrites_ResolvesCredentialsOncePerPushCycle proves the branch worker does not +// re-read the Git credentials Secret for every commit in a push cycle. Only the first commit +// (hasPendingCommits=false) touches the remote through PrepareBranch and needs auth; later commits +// build on the local repo and must not issue another Secret GET — with the Secret cache disabled +// that would be a wasted API round-trip per commit. See docs/future/secret-value-retention-plan.md §5. +func TestCommitPendingWrites_ResolvesCredentialsOncePerPushCycle(t *testing.T) { + tempDir := t.TempDir() + remotePath := filepath.Join(tempDir, "remote") + createBareRepo(t, remotePath) + + const credsSecretName = "git-creds" + var credentialReads atomic.Int64 + + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configv1alpha3.AddToScheme(scheme)) + + provider := &configv1alpha3.GitProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "test-repo", Namespace: "default"}, + Spec: configv1alpha3.GitProviderSpec{ + URL: "file://" + remotePath, + SecretRef: &configv1alpha3.LocalSecretReference{Name: credsSecretName}, + }, + } + // username/password resolves to HTTP basic auth, which the file:// transport ignores — so the + // commit path exercises the real credential read without needing a network remote. + creds := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: credsSecretName, Namespace: "default"}, + Data: map[string][]byte{"username": []byte("git"), "password": []byte("token")}, + } + + countingClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(provider, creds). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func( + ctx context.Context, + c client.WithWatch, + key client.ObjectKey, + obj client.Object, + opts ...client.GetOption, + ) error { + if _, ok := obj.(*corev1.Secret); ok && key.Name == credsSecretName { + credentialReads.Add(1) + } + return c.Get(ctx, key, obj, opts...) + }, + }). + Build() + + worker := NewBranchWorker(countingClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker.ctx = context.Background() + + // First commit of the cycle: fetches the remote tip, so it resolves credentials exactly once. + firstWrite, err := worker.buildGroupedPendingWrite(worker.ctx, []Event{createTestEvent(t, "pod-a")}) + require.NoError(t, err) + require.NoError(t, worker.commitPendingWrites([]PendingWrite{*firstWrite}, false)) + require.Equal(t, int64(1), credentialReads.Load(), + "first commit resolves credentials once for the remote fetch") + + // Second commit in the same cycle (hasPendingCommits=true): local-only, no credential read. + secondWrite, err := worker.buildGroupedPendingWrite(worker.ctx, []Event{createTestEvent(t, "pod-b")}) + require.NoError(t, err) + require.NoError(t, worker.commitPendingWrites([]PendingWrite{*secondWrite}, true)) + assert.Equal(t, int64(1), credentialReads.Load(), + "a later commit in the same push cycle must not re-read the credentials Secret") +} diff --git a/internal/git/encryption.go b/internal/git/encryption.go index 6c9df66e..e90d91e5 100644 --- a/internal/git/encryption.go +++ b/internal/git/encryption.go @@ -8,9 +8,6 @@ import ( "encoding/hex" "errors" "fmt" - "os" - "path/filepath" - "regexp" "sort" "strings" @@ -41,22 +38,19 @@ const ( EncryptionProviderSOPS = "sops" // defaultSOPSBinaryPath is resolved from PATH in the controller runtime image. defaultSOPSBinaryPath = "sops" - // sopsAgeKeyFileEnvVar points SOPS to a file containing age private identities. - sopsAgeKeyFileEnvVar = "SOPS" + "_AGE_KEY_FILE" - // ageSecretKeySuffix identifies Flux-compatible age private-key entries. + // ageSecretKeySuffix identifies Flux-compatible age private-key entries. The write path + // reads these only to derive the public recipient; the private identity is never kept. ageSecretKeySuffix = ".agekey" - // ageIdentityFileDir is the temp directory used for SOPS age identity files. - ageIdentityFileDir = "gitops-reverser-age-identities" ) -var envVarNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) - // ResolvedEncryptionConfig contains runtime encryption settings resolved from GitTarget spec. +// +// It carries public age recipients only. The write path encrypts, it never decrypts, so no +// private age identity is retained, written to disk, or passed to the sops process. See +// docs/future/secret-value-retention-plan.md. type ResolvedEncryptionConfig struct { Provider string - Environment map[string]string AgeRecipients []string - AgeIdentities []string } // ResolveTargetEncryption resolves and validates GitTarget encryption configuration. @@ -83,12 +77,7 @@ func ResolveTargetEncryption( if err != nil { return nil, err } - secretRecipients, secretIdentities, environment, err := resolveSecretRecipientsAndEnvironment( - ctx, - k8sClient, - target, - encryptionSpec, - ) + secretRecipients, err := resolveSecretRecipients(ctx, k8sClient, target, encryptionSpec) if err != nil { return nil, err } @@ -100,12 +89,9 @@ func ResolveTargetEncryption( ) } - environment = normalizeEnvironment(environment) return &ResolvedEncryptionConfig{ Provider: providerName, - Environment: environment, AgeRecipients: resolvedRecipients, - AgeIdentities: secretIdentities, }, nil } @@ -120,19 +106,23 @@ func resolveEncryptionProvider(encryptionSpec *v1alpha3.EncryptionSpec) (string, return providerName, nil } -func resolveSecretRecipientsAndEnvironment( +// resolveSecretRecipients reads the referenced age-key Secret and derives the public age +// recipients from its *.agekey entries. It intentionally derives recipients only: the private +// identities and the rest of the Secret data are never returned to the caller, so they cannot +// reach the sops process or disk. +func resolveSecretRecipients( ctx context.Context, k8sClient client.Client, target *v1alpha3.GitTarget, encryptionSpec *v1alpha3.EncryptionSpec, -) ([]string, []string, map[string]string, error) { +) ([]string, error) { if encryptionSpec.Age == nil || !encryptionSpec.Age.Recipients.ExtractFromSecret { - return nil, nil, nil, nil + return nil, nil } secretKind := strings.TrimSpace(encryptionSpec.SecretRef.Kind) if secretKind != "" && secretKind != "Secret" { - return nil, nil, nil, fmt.Errorf( + return nil, fmt.Errorf( "encryption.secretRef.kind must be Secret, got %q", encryptionSpec.SecretRef.Kind, ) @@ -140,23 +130,22 @@ func resolveSecretRecipientsAndEnvironment( secretName := strings.TrimSpace(encryptionSpec.SecretRef.Name) if secretName == "" { - return nil, nil, nil, errors.New( + return nil, errors.New( "encryption.secretRef.name must be set when age.recipients.extractFromSecret=true", ) } secret, secretKey, err := getEncryptionSecret(ctx, k8sClient, target.Namespace, secretName) if err != nil { - return nil, nil, nil, err + return nil, err } - secretRecipients, secretIdentities, err := resolveAgeRecipientsFromSecret(secret.Data) + secretRecipients, err := resolveAgeRecipientsFromSecret(secret.Data) if err != nil { - return nil, nil, nil, fmt.Errorf("failed to resolve recipients from encryption secret %s: %w", secretKey, err) + return nil, fmt.Errorf("failed to resolve recipients from encryption secret %s: %w", secretKey, err) } - environment := toSOPSEnvironment(secret.Data) - return secretRecipients, secretIdentities, environment, nil + return secretRecipients, nil } func getEncryptionSecret( @@ -177,13 +166,6 @@ func getEncryptionSecret( return &secret, secretKey, nil } -func normalizeEnvironment(environment map[string]string) map[string]string { - if len(environment) == 0 { - return nil - } - return environment -} - func configureSecretEncryptionWriter( writer *contentWriter, workDir string, @@ -196,12 +178,11 @@ func configureSecretEncryptionWriter( switch cfg.Provider { case EncryptionProviderSOPS: - environment, err := buildSOPSEnvironment(workDir, cfg) - if err != nil { - return err - } + // No environment is passed to sops: age recipients are published to the target's + // .sops.yaml at bootstrap, and the write path only encrypts, so it needs neither a + // private-key file (SOPS_AGE_KEY_FILE) nor blanket Secret data in the process env. scope := secretEncryptionCacheScope(workDir, cfg) - writer.setEncryptor(NewSOPSEncryptorWithEnv(defaultSOPSBinaryPath, "", workDir, environment), scope) + writer.setEncryptor(NewSOPSEncryptorWithEnv(defaultSOPSBinaryPath, "", workDir, nil), scope) return nil default: return fmt.Errorf("unsupported encryption provider %q", cfg.Provider) @@ -223,105 +204,30 @@ func secretEncryptionCacheScope(workDir string, cfg *ResolvedEncryptionConfig) s hasher.Write([]byte(strings.TrimSpace(recipient))) hasher.Write([]byte{0}) } - for _, identity := range cfg.AgeIdentities { - hasher.Write([]byte(strings.TrimSpace(identity))) - hasher.Write([]byte{0}) - } sum := hasher.Sum(nil) return hex.EncodeToString(sum[:16]) } -func buildSOPSEnvironment(workDir string, cfg *ResolvedEncryptionConfig) (map[string]string, error) { - environment := cloneEnvironment(cfg.Environment) - if len(cfg.AgeIdentities) == 0 { - return environment, nil - } - - ageKeyFilePath, err := writeAgeIdentityFile(workDir, cfg.AgeIdentities) - if err != nil { - return nil, fmt.Errorf("failed to write SOPS age identity file: %w", err) - } - if environment == nil { - environment = make(map[string]string, 1) - } - environment[sopsAgeKeyFileEnvVar] = ageKeyFilePath - return environment, nil -} - -func cloneEnvironment(environment map[string]string) map[string]string { - if len(environment) == 0 { - return nil - } - cloned := make(map[string]string, len(environment)) - for key, value := range environment { - cloned[key] = value - } - return cloned -} - -func writeAgeIdentityFile(workDir string, identities []string) (string, error) { - if len(identities) == 0 { - return "", errors.New("no age identities provided") - } - - dirPath := filepath.Join(os.TempDir(), ageIdentityFileDir) - if err := os.MkdirAll(dirPath, 0700); err != nil { - return "", fmt.Errorf("create age identity directory: %w", err) - } - - keyHash := sha256.Sum256([]byte(strings.TrimSpace(workDir))) - fileName := hex.EncodeToString(keyHash[:8]) + ageSecretKeySuffix - filePath := filepath.Join(dirPath, fileName) - fileContent := strings.Join(identities, "\n") + "\n" - if err := os.WriteFile(filePath, []byte(fileContent), 0600); err != nil { - return "", fmt.Errorf("write age identity file: %w", err) - } - - return filePath, nil -} - -func toSOPSEnvironment(secretData map[string][]byte) map[string]string { +func resolveAgeRecipientsFromSecret(secretData map[string][]byte) ([]string, error) { if len(secretData) == 0 { - return nil - } - - environment := make(map[string]string, len(secretData)) - for key, value := range secretData { - if !envVarNamePattern.MatchString(key) { - continue - } - environment[key] = string(value) - } - - if len(environment) == 0 { - return nil - } - - return environment -} - -func resolveAgeRecipientsFromSecret(secretData map[string][]byte) ([]string, []string, error) { - if len(secretData) == 0 { - return nil, nil, nil + return nil, nil } recipients := make([]string, 0, len(secretData)) - identities := make([]string, 0, len(secretData)) for key, value := range secretData { if !strings.HasSuffix(key, ageSecretKeySuffix) { continue } - recipient, identity, err := deriveAgeRecipientFromSecretEntry(key, string(value)) + recipient, err := deriveAgeRecipientFromSecretEntry(key, string(value)) if err != nil { - return nil, nil, err + return nil, err } recipients = append(recipients, recipient) - identities = append(identities, identity) } - return dedupeAndSortRecipients(recipients), dedupeAndSortIdentities(identities), nil + return dedupeAndSortRecipients(recipients), nil } func normalizePublicAgeRecipients(publicKeys []string) ([]string, error) { @@ -346,7 +252,10 @@ func normalizePublicAgeRecipients(publicKeys []string) ([]string, error) { return dedupeAndSortRecipients(recipients), nil } -func deriveAgeRecipientFromSecretEntry(secretKey, secretValue string) (string, string, error) { +// deriveAgeRecipientFromSecretEntry parses a single AGE-SECRET-KEY identity from a *.agekey +// Secret entry and returns only its public recipient. The parsed private identity is used +// solely to derive the recipient and is never returned, stored, or written to disk. +func deriveAgeRecipientFromSecretEntry(secretKey, secretValue string) (string, error) { lines := strings.Split(secretValue, "\n") identities := make([]string, 0, len(lines)) for _, line := range lines { @@ -358,21 +267,21 @@ func deriveAgeRecipientFromSecretEntry(secretKey, secretValue string) (string, s } if len(identities) == 0 { - return "", "", fmt.Errorf("%s must contain one AGE-SECRET-KEY identity", secretKey) + return "", fmt.Errorf("%s must contain one AGE-SECRET-KEY identity", secretKey) } if len(identities) > 1 { - return "", "", fmt.Errorf("%s must contain exactly one AGE-SECRET-KEY identity", secretKey) + return "", fmt.Errorf("%s must contain exactly one AGE-SECRET-KEY identity", secretKey) } if !strings.HasPrefix(identities[0], "AGE-SECRET-KEY-") { - return "", "", fmt.Errorf("%s must contain AGE-SECRET-KEY identity", secretKey) + return "", fmt.Errorf("%s must contain AGE-SECRET-KEY identity", secretKey) } identity, err := age.ParseX25519Identity(identities[0]) if err != nil { - return "", "", fmt.Errorf("invalid %s identity: %w", secretKey, err) + return "", fmt.Errorf("invalid %s identity: %w", secretKey, err) } - return identity.Recipient().String(), identity.String(), nil + return identity.Recipient().String(), nil } func dedupeAndSortRecipients(recipients []string) []string { @@ -400,29 +309,3 @@ func dedupeAndSortRecipients(recipients []string) []string { sort.Strings(result) return result } - -func dedupeAndSortIdentities(identities []string) []string { - if len(identities) == 0 { - return nil - } - - uniq := make(map[string]struct{}, len(identities)) - for _, identity := range identities { - trimmed := strings.TrimSpace(identity) - if trimmed == "" { - continue - } - uniq[trimmed] = struct{}{} - } - - if len(uniq) == 0 { - return nil - } - - result := make([]string, 0, len(uniq)) - for identity := range uniq { - result = append(result, identity) - } - sort.Strings(result) - return result -} diff --git a/internal/git/encryption_test.go b/internal/git/encryption_test.go index ef159f74..720b6692 100644 --- a/internal/git/encryption_test.go +++ b/internal/git/encryption_test.go @@ -4,8 +4,6 @@ package git import ( "context" - "os" - "path/filepath" "sort" "testing" @@ -136,7 +134,6 @@ func TestResolveTargetEncryption(t *testing.T) { require.NoError(t, err) require.NotNil(t, resolved) assert.Equal(t, []string{identity.Recipient().String()}, resolved.AgeRecipients) - assert.Nil(t, resolved.Environment) }) t.Run("fails when extractFromSecret is enabled and secret name is empty", func(t *testing.T) { @@ -229,7 +226,9 @@ func TestResolveTargetEncryption(t *testing.T) { Data: map[string][]byte{ "identity.agekey": []byte(firstIdentity.String()), "backup.agekey": []byte(secondIdentity.String()), - "SOPS_KMS_ARN": []byte("kms-arn"), + // A non-agekey entry that also happens to be a valid env var name must be + // ignored entirely: it is neither a recipient source nor passed to sops. + "SOPS_KMS_ARN": []byte("kms-arn"), }, }).Build() target := &v1alpha3.GitTarget{ @@ -265,58 +264,32 @@ func TestResolveTargetEncryption(t *testing.T) { } sort.Strings(expectedRecipients) assert.Equal(t, expectedRecipients, resolved.AgeRecipients) - assert.Equal(t, "kms-arn", resolved.Environment["SOPS_KMS_ARN"]) - - expectedIdentities := []string{ - firstIdentity.String(), - secondIdentity.String(), - } - sort.Strings(expectedIdentities) - assert.Equal(t, expectedIdentities, resolved.AgeIdentities) }) } -func TestBuildSOPSEnvironment(t *testing.T) { - t.Run("returns base environment when no age identities exist", func(t *testing.T) { - cfg := &ResolvedEncryptionConfig{ - Environment: map[string]string{ - "SOPS_KMS_ARN": "kms-arn", - }, - } - - env, err := buildSOPSEnvironment(t.TempDir(), cfg) - require.NoError(t, err) - assert.Equal(t, "kms-arn", env["SOPS_KMS_ARN"]) - _, hasKeyFile := env[sopsAgeKeyFileEnvVar] - assert.False(t, hasKeyFile) - }) - - t.Run("writes age identities to temp file and sets SOPS age key file env", func(t *testing.T) { - firstIdentity, err := age.GenerateX25519Identity() - require.NoError(t, err) - secondIdentity, err := age.GenerateX25519Identity() - require.NoError(t, err) - - workDir := t.TempDir() - cfg := &ResolvedEncryptionConfig{ - Environment: map[string]string{ - "SOPS_KMS_ARN": "kms-arn", - }, - AgeIdentities: []string{firstIdentity.String(), secondIdentity.String()}, - } - - env, err := buildSOPSEnvironment(workDir, cfg) - require.NoError(t, err) - keyFilePath := env[sopsAgeKeyFileEnvVar] - require.NotEmpty(t, keyFilePath) - assert.Equal(t, ageIdentityFileDir, filepath.Base(filepath.Dir(keyFilePath))) - - content, err := os.ReadFile(keyFilePath) - require.NoError(t, err) - assert.Contains(t, string(content), firstIdentity.String()) - assert.Contains(t, string(content), secondIdentity.String()) - assert.Equal(t, "kms-arn", env["SOPS_KMS_ARN"]) - }) +// TestConfigureSecretEncryptionWriter_NoPrivateAgeMaterial proves the SOPS write path never +// receives private age identity material: no SOPS_AGE_KEY_FILE, and no environment carrying +// AGE-SECRET-KEY content. Recipients reach sops through the target's .sops.yaml, not the env. +func TestConfigureSecretEncryptionWriter_NoPrivateAgeMaterial(t *testing.T) { + identity, err := age.GenerateX25519Identity() + require.NoError(t, err) + + writer := &contentWriter{} + cfg := &ResolvedEncryptionConfig{ + Provider: EncryptionProviderSOPS, + AgeRecipients: []string{identity.Recipient().String()}, + } + + require.NoError(t, configureSecretEncryptionWriter(writer, t.TempDir(), cfg)) + + encryptor, ok := writer.encryptor.(*SOPSEncryptor) + require.True(t, ok, "expected a *SOPSEncryptor to be configured") + assert.Empty(t, encryptor.env, "SOPS env must be empty; no private key file or secret data") + _, hasKeyFile := encryptor.env["SOPS_AGE_KEY_FILE"] + assert.False(t, hasKeyFile, "the age private-key file env var must never be set") + for key, value := range encryptor.env { + assert.NotContains(t, value, "AGE-SECRET-KEY", "env var %q must not carry a private age key", key) + } } func TestDeriveAgeRecipientFromSecretEntry(t *testing.T) { @@ -324,20 +297,19 @@ func TestDeriveAgeRecipientFromSecretEntry(t *testing.T) { identity, err := age.GenerateX25519Identity() require.NoError(t, err) - recipient, parsedIdentity, err := deriveAgeRecipientFromSecretEntry("identity.agekey", identity.String()) + recipient, err := deriveAgeRecipientFromSecretEntry("identity.agekey", identity.String()) require.NoError(t, err) assert.Equal(t, identity.Recipient().String(), recipient) - assert.Equal(t, identity.String(), parsedIdentity) }) t.Run("fails when identity is missing", func(t *testing.T) { - _, _, err := deriveAgeRecipientFromSecretEntry("identity.agekey", "") + _, err := deriveAgeRecipientFromSecretEntry("identity.agekey", "") require.Error(t, err) assert.Contains(t, err.Error(), "identity.agekey must contain one AGE-SECRET-KEY identity") }) t.Run("fails when identity is invalid", func(t *testing.T) { - _, _, err := deriveAgeRecipientFromSecretEntry("identity.agekey", "AGE-SECRET-KEY-1invalid") + _, err := deriveAgeRecipientFromSecretEntry("identity.agekey", "AGE-SECRET-KEY-1invalid") require.Error(t, err) assert.Contains(t, err.Error(), "invalid identity.agekey identity") }) @@ -349,7 +321,7 @@ func TestDeriveAgeRecipientFromSecretEntry(t *testing.T) { require.NoError(t, err) combined := first.String() + "\n" + second.String() - _, _, err = deriveAgeRecipientFromSecretEntry("identity.agekey", combined) + _, err = deriveAgeRecipientFromSecretEntry("identity.agekey", combined) require.Error(t, err) assert.Contains(t, err.Error(), "must contain exactly one AGE-SECRET-KEY identity") }) diff --git a/internal/watch/manager.go b/internal/watch/manager.go index 937ccb27..5f528bde 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -127,7 +127,7 @@ type Manager struct { // gitPathEventsCh carries a GenericEvent for a GitTarget whenever its GitPath acceptance // state TRANSITIONS, so the GitTarget controller re-projects GitPathAccepted promptly - // instead of waiting up to RequeueLongInterval (10m) for its next periodic reconcile. The + // instead of waiting up to RequeueSteadyInterval (5m) for its next periodic reconcile. The // data plane records acceptance asynchronously; without this edge the status lags. See // docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md. Lazily // created by GitPathEvents() and guarded by gitPathEventsMu.