mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-29 16:17:20 +09:00
Andrey Kolkov
f6a615f693
## What this PR does A CNPG `Backup` taken while the Postgres app still accepted `users[].password` copied that password into `status.underlyingResources`, where anyone with `get backups` in the namespace can read it. #4078 stopped new snapshots from carrying the field but left every existing `Backup` as it was (#4179). This adds `LegacyPasswordScrubber`, a leader-elected runnable in the backup strategy controller that, on start, lists `Backup` objects and removes `users.*.password` from every snapshot whose `kind`/`apiVersion` is the CNPG snapshot's. - **Why on every leader start and not a platform migration.** The migration hook runs pre-upgrade, while the old controller is still leader, so a `Backup` the old replica writes during the rollout would keep its password and the migration never re-runs. With `--leader-elect` on, the old replicas stop writing before the new leader's scrub runs, and a `Backup` brought back from an object-level export is cleaned on the next start too. On a clean cluster the whole pass is one List from the manager cache, which `BackupReconciler` already populates. - **Write shape.** The patch is a merge patch computed from the diff, so the request body only nulls the `password` keys and cannot overwrite status another writer changed since the List. The snapshot is handled as generic JSON (numbers kept as `json.Number`), so fields the current snapshot type does not know survive. - **Failure handling.** A `Backup` that cannot be decoded or patched is logged and skipped, and a failed List is logged too; `Start` always returns nil, because a returned error would stop the manager and take every backup down over one object. - **Restore compatibility.** Restore already ignored the key; a test decodes a scrubbed snapshot through `unmarshalCNPGBackupSnapshot` to pin that. - RBAC: `list` and `patch` on `backups.cozystack.io/backups` are already granted to the controller, so the chart is unchanged. - `docs/operations/backup-classes.md` no longer tells operators to delete pre-upgrade `Backup` objects, and says what the scrub does not reach (audit logs, etcd backups, external exports of the objects, Helm history of the init-script Secret), so a password once set through `users[].password` is still treated as exposed until rotated. The runnable carries a `TODO(#4179)` to remove it once upgrading from a release that accepted `users[].password` is no longer supported. Verification: `go test ./internal/backupcontroller/...` and `go build ./cmd/backupstrategy-controller/` are green. The new tests cover scrubbing with everything else kept (including an unknown field with a large integer), leaving non-CNPG, other-apiVersion, clean and snapshot-less Backups unpatched, and continuing past a Backup that fails to decode or patch. Disabling the delete, or returning on the first patch error, makes the tests fail. Not exercised on a live cluster yet. Fixes #4179 ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map against the diff: it touches only `internal/backupcontroller`, `cmd/backupstrategy-controller` and the in-repo `docs/operations/backup-classes.md`, with no package, values, schema, CRD, variant or asset change. The website carries no "delete pre-upgrade Backups" workaround this would make obsolete; its stale `users[name].password` rows are the #4078 follow-up already noted there. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(backups): the backup strategy controller now removes plaintext `users[].password` values left in `status.underlyingResources` of CNPG `Backup` objects taken before managed Postgres stopped accepting user passwords. It runs on every controller start and needs no action. It does not reach copies outside the cluster (audit logs, etcd backups, exported `Backup` objects), so a password once set through `users[].password` should still be treated as exposed until it is rotated. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Security** * At controller startup, plaintext user passwords are removed from matching CNPG backup snapshots. Other snapshot data is preserved. * This cleanup does not affect external copies or remove older init-script manifests from Helm history. Treat passwords previously set through `users[].password` as exposed until rotated. <!-- end of auto-generated comment: release notes by coderabbit.ai -->