diff --git a/CHANGELOG.md b/CHANGELOG.md index 99ce7ed..2de4b42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,89 @@ ## Changelog +### v0.188.0 — D5: an app restore works from the drive alone (2026-07-30) — MinAgent 0.113.0 (unchanged) + +**Tier-1/Tier-2 no longer depend on the whole-guest tier.** Until now the recovery unit on the +customer's drive was secret-free, which made the two-lane split *look* independent while it was not: the +app's files sat safely on the drive and could not be brought back, because the secrets that make them +readable went down with the guest. After this, restoring an app needs **the drive and nothing else** — +not the server, not the operator, not the offsite copy. + +**Part 0 first: the brief's own recommendation did not survive the test it asked for.** It proposed that +only `data_key`-flagged secrets travel. Two findings overturned it, both evidenced before any code: + +1. **The `data_key` flag is not a trustworthy classification.** Only 5 fields across 4 apps carry it, + yet the catalog's own Hungarian labels contradict the flag elsewhere: `n8n/N8N_ENCRYPTION_KEY` + („Titkosítási kulcs"), `wanderer/POCKETBASE_ENCRYPTION_KEY` („Adatbázis titkosítási kulcs"), + `calcom/CALENDSO_ENCRYPTION_KEY`, `bookstack/APP_KEY` — same label as `adventurelog/SECRET_KEY`, + opposite flag. Travelling "only data keys" would omit real data keys, and the fail-closed gate would + not fire for them → a restore that succeeds onto unreadable data. Filed **R-127**. +2. **A DB password is not resettable in practice — proven, not argued.** `DumpAppVolumes` dumps every + compose named volume with no DB exclusion, so `immich_postgres_data.tar` is captured and restored. + Probe on `postgres:16-alpine` (seed → drop container, keep volume → redeploy with a regenerated + password): the **replay succeeded** (`docker exec psql`, no password — verbatim what `ImportDump` + does, and the image's local socket is `trust`), the **app path failed** over the compose network + (`FATAL: password authentication failed`), and the **old** password still worked — `POSTGRES_PASSWORD` + is ignored once PGDATA is non-empty. So the restore reports success, the dump replays, the rows are + there, and the application cannot reach them. 18 DB/root-password fields affected. MariaDB fails + louder: `getMariaDBPassword` reads the regenerated value from container env against a datadir holding + the old hash, so the replay itself gets Access denied (`nextcloud`, `romm`). + +**The rulings (operator, 2026-07-30).** `type: secret` travels; `type: password` never does; minus a +code register. Plaintext, as the data already is. + +- **TRAVELS (45 fields):** the 5 declared data keys, 18 DB/root passwords, 22 internal signing/encryption + secrets. Every one decrypts data on the SAME drive or authenticates to a container on an internal + compose network with no external listener — possessing it adds nothing to possessing the drive, which + is exactly D2's argument for plaintext DATA. +- **WITHHELD (8):** the 7 `type: password` admin/UI logins + `vaultwarden/ADMIN_TOKEN` via the + `nonPortableSecrets` register. These authenticate against published services, so their reach is NOT + bounded by the drive. **Excluding this class is what licenses the plaintext ruling; the two are coupled + and must not be relaxed independently.** The register is code, not a catalog flag — a boundary a + catalog push can silently move is not a boundary (cf. R-97a). + +**What a customer must possess to complete a Tier-1/2 restore after this change: the drive.** + +**Implementation** — one place per side, no parallel path. `stacks.PortableSecretEnvVars` is the whole +boundary; `GetStackRecoveryInfo` decrypts the portable class through the SAME `LoadAppConfigDecrypted` +the restore side uses; `buildUnitAppYaml` (was `buildStrippedAppYaml`) writes it at **0600** and names +the withheld class in the header so an operator can see WHY a credential is absent rather than suspect a +capture bug; `readUnitEnv` splits it back using the **manifest's** portable names, never guessed from key +names. `reconcileRestoreSecrets` stays a pure function — the new source arrives as an **argument**. +Manifest → **schema 2** + `portable_secret_env_vars` (NAMES only; the manifest is 0644). + +**Precedence: the UNIT WINS.** Not "newest wins". The unit's secrets are captured in the same run as the +dumps beside them (`runVolumeDumps` → `captureAllRecoveryUnits`), so the unit's value is the one that +matches the data about to be restored; the guest's is merely the most recent. A rotated data key does not +decrypt data encrypted with the old one, and a rotated DB password does not match the hash in the +restored data directory. Pinned in both directions — an undefined precedence between two sources of a +decryption key is a data-loss bug waiting for its first disagreement. + +**The fail-closed gate is unchanged and still fail-closed:** a data key in NEITHER source refuses +outright. D5 makes it normally present; "normally" is not a reason to soften a gate. + +**Three comments that asserted invariants D5 makes false were corrected, not left to read as settled** +(`CaptureRecoveryUnit` "NEVER writes a secret value", `RestoreFromRecoveryUnit` "no secret is read from +the unit", `appbackup/paths.go` + `appdata.go` "secret-free"), and the O4 WARN that claimed "stored data +is unaffected" for every non-data-key secret now says what is true — finding 2 disproves it for DB +passwords. + +**Backward compatible.** A schema-1 unit carries no secrets and still restores from the guest; the next +capture rewrites it (the app.yaml checksum changes). No existing backup changes, no data moves, and the +escrow / whole-guest / offsite tiers are untouched in code — the offsite copy simply carries the secrets +inside the unit it already pushed, encrypted at rest under the customer-owned restic password. + +**Tests** — `TestRestoreFromRecoveryUnitWithGuestAbsent` is D5's claim as a test rather than a +description; plus fail-closed-with-both-sources-absent, precedence both directions, the schema-1 +no-regression case, `readUnitEnv` splitting, and the wrong-outcome check that the withheld class appears +NOWHERE in the unit. Fixtures come from a unit written by the **real** `CaptureRecoveryUnit`, so the two +sides meet at real bytes. Seam: `Manager.stackProvider` only. **Four red-proofs, each verified to have +landed:** drop the portable merge → the consequence test fails; neuter the gate → 4 failures; flip +precedence → the unit-wins test fails; widen the class to `type: password` → the boundary test fails. + +**R-120's gate does not apply to this task** — it sits in `hub/internal/web/configs.go` +`handleSetArtifacts`, the golden-**vouch** form, and never runs on a controller image deploy. Re-baking +the golden is a follow-on so that FRESH installs get D5; it is not a prerequisite here. + ### v0.187.0 — R-108: network storage may not host an app's data namespace (2026-07-30) — MinAgent 0.113.0 (unchanged) **This is D5's precondition, and it is now met.** D5 moves app secrets into the local recovery unit so diff --git a/REUSE.md b/REUSE.md index 50d04e0..35b0bab 100644 --- a/REUSE.md +++ b/REUSE.md @@ -111,7 +111,9 @@ | `SaveAppConfig` / `LoadAppConfigDecrypted` | controller/internal/stacks/deploy.go | `(stackDir, cfg, encKey, sensitiveVars)` | app.yaml persistence | Encrypts only `SensitiveEnvVars(meta)`; never write app.yaml directly | | `generateValue` / `randomAlphanumeric` | controller/internal/stacks/deploy.go | `(spec "password:N\|hex:N\|base64key:N\|static:v")` | Auto-generated secrets | crypto/rand-backed; reuse the spec grammar | | `Manager.GenerateSecretForField` | controller/internal/stacks/deploy.go | `(stackName, envVar) (string, bool)` | Replacement value for a RESETTABLE secret from its catalog `generate` spec (O4 restore path via `backup.SetSecretGenerator`) | REFUSES `data_key` fields, spec-less and non-secret fields; never log the value | -| `reconcileRestoreSecrets` | controller/internal/backup/restore_unit.go | `(nonSecretEnv, recoveredSecrets, secretNames, dataKeyNames)` | Recovery-unit restore env merge | Units are secret-FREE by design; secrets come from live app.yaml | +| `reconcileRestoreSecrets` | controller/internal/backup/restore_unit.go | `(nonSecretEnv, unitSecrets, guestSecrets, secretNames, dataKeyNames)` | Recovery-unit restore env merge | **Precedence: UNIT WINS over guest** (the unit's secrets match the data being restored; the guest's are merely newest). Pure — new sources arrive as ARGUMENTS. Fail-closed data-key gate lives here | +| `stacks.PortableSecretEnvVars` | controller/internal/stacks/deploy.go | `(meta) []string` | **THE D5 secret boundary**: which secrets may travel on a customer drive | `type: secret` travels, `type: password` NEVER, minus the `nonPortableSecrets` code register. Withholding the password class is what licenses plaintext — do not relax one without the other | +| `buildUnitAppYaml` / `readUnitEnv` | controller/internal/backup/{recovery_unit,restore_unit}.go | `(info) []byte` / `(path, portableNames)` | The ONE place the unit's app.yaml is written / split back | Split is driven by the MANIFEST's portable names, never guessed from key names; write 0600; empty `portableNames` = schema-1 unit ⇒ everything is plain config | | `EncryptFile` / `DecryptFile` / `IsEncryptedFAB` | controller/internal/appexport/crypto.go | password-based file crypto | .fab export bundles | scrypt-derived AES+HMAC keys | | `maskRepoURL` | controller/internal/sync/sync.go | `(url) string` | Logging git URLs | Strips embedded credentials | | `metrics.RedactLine` | controller/internal/metrics/redact.go | `(s string) string` | ANY log line shipped off-box (issue context, log tails) | Masks password/passwd/secret/token/api-key/authorization/bearer values + 64-hex; apply BEFORE the line leaves the box — controller-side redaction is authoritative | diff --git a/controller/README.md b/controller/README.md index 54e5707..5f60476 100644 --- a/controller/README.md +++ b/controller/README.md @@ -808,36 +808,64 @@ Path computation is centralized in `backup/paths.go` via the `FelhomDataDir = "f > `AppSecondaryRsyncPath`, `SecondaryInfraPath`) describe the pre-strip layout — restic/cross-drive was > removed in slice 8C. This section is rewritten when Tier 2 (Phase 3) lands. -#### Per-app recovery unit (Phase 2, v0.53.x) — SECRET-FREE +#### Per-app recovery unit (Phase 2, v0.53.x; secret model rewritten by **D5**, v0.188.0) Each app's `backups/primary//` is a self-contained, recreatable **recovery unit**: ``` backups/primary// -├── compose/ docker-compose.yml + .felhom.yml + a SECRET-STRIPPED app.yaml +├── compose/ docker-compose.yml + .felhom.yml + app.yaml (0600 — CARRIES the portable secrets) ├── db-dumps/ app-consistent DB dump(s) ├── volume-dumps/ named-volume tars -└── manifest.json image pins, secret env-var NAMES, data_key names, checksums, secret_source +└── manifest.json image pins, secret NAMES, data_key names, portable NAMES, checksums, secret_source ``` -- **Secret-free by design.** The unit stores **no secret value, no data-encrypting key, and not the - Docker image** — only the pinned image tag(s) (re-pulled on restore) and the *names* of the secret / - `data_key` env vars. Rationale: app.yaml + the encryption key live on the guest rootfs → already in - the PBS whole-guest snapshot, and the hub is deliberately zero-knowledge. Restore recovers the - original secrets from the guest's own app.yaml (live, or via PBS); for a `data_key` app it - **fails closed** (refuse + warn) if the key can't be recovered — data-keys are NEVER generated. - **Resettable secrets (O4, v0.99.0):** an unrecoverable resettable secret (DB password etc.) gets a - **generated replacement** from its catalog `generate` spec (`stacks.GenerateSecretForField` via the - `backup.SetSecretGenerator` seam) instead of redeploying blank (which failed compose-up); the new - value persists encrypted through the normal `RecreateStackDefinitionFromUnit` → `SaveAppConfig` path. Fields - with no `generate` spec still proceed with a loud "may fail to start" WARN. Residual case: a restored - volume tar carrying the OLD internal credential hash may still need a manual in-DB reset. +- **The secret split (D5, schema 2, operator ruling 2026-07-30).** The unit was secret-free until + v0.188.0, and that made "restore from the drive alone" false: the fast, local, customer-doable + Tier-1/2 restore secretly depended on the slow, operator-driven whole-guest restore, because a + data-encrypting key or a DB password absent from the guest cannot be regenerated without leaving the + restored data unreachable. **Tier-1/2 now needs the drive and nothing else.** What travels is decided + in ONE place, `stacks.PortableSecretEnvVars`: + - **TRAVELS — every `type: secret` field** (45 of 53 across the catalog): the declared `data_key`s, + the 18 DB/root passwords, and the internal signing/encryption secrets. Each of these either + decrypts data sitting on the SAME drive or authenticates to a container on an internal compose + network with no external listener, so possessing it adds nothing to possessing the drive — which is + exactly D2's argument for keeping the DATA plaintext. Written into the unit's app.yaml at **0600**, + plaintext, like the data beside it. + - **WITHHELD — every `type: password` field** (7 admin/UI logins) **plus the `nonPortableSecrets` + register** (`vaultwarden/ADMIN_TOKEN`, whose `/admin` panel is on the app's public web port). + These authenticate against published services, so their blast radius is NOT bounded by the drive. + They stay in the guest and are regenerated on restore (O4). **Excluding this class is what licenses + the plaintext ruling — the two are coupled and must not be relaxed independently.** + - The register is **code, not a catalog flag**, deliberately: a security boundary a catalog push can + silently move is not a boundary (cf. R-97a). Adding an app whose `type: secret` field gates an + internet-reachable login means adding a row there. +- **Fail-closed is unchanged.** A `data_key` missing from **both** the unit and the guest still refuses + the restore outright (never generated). D5 makes the key normally present; "normally" is not a reason + to soften the gate. +- **Precedence: the UNIT WINS** over the guest when both hold a value. Not "newest wins" — the unit's + secrets are captured in the same run as the dumps beside them, so the unit's value is the one that + MATCHES THE DATA BEING RESTORED, while the guest's is merely the most recent. A rotated data key does + not decrypt data encrypted with the old one, and a rotated DB password does not match the hash inside + the restored data directory. Pinned in both directions. +- **Resettable secrets (O4, v0.99.0)** — now the rare path, since the portable class comes from the + unit. An unrecoverable withheld secret gets a **generated replacement** from its catalog `generate` + spec (`stacks.GenerateSecretForField` via the `backup.SetSecretGenerator` seam) rather than redeploying + blank; the value persists encrypted through `RecreateStackDefinitionFromUnit` → `SaveAppConfig`. + ⚠️ **R-127:** a regenerated **database** password is NOT harmless — `POSTGRES_PASSWORD` is ignored once + PGDATA is non-empty, so the restored data dir keeps the old role hash and the app cannot authenticate + against its own rows, while the dump replay (local trust socket) still reports success. The WARN says so. - Helpers: `RecoveryUnitPath` / `RecoveryUnitComposePath` / `RecoveryUnitManifestPath` (`internal/appbackup/paths.go`). Capture: `Manager.CaptureRecoveryUnit` (`internal/backup/recovery_unit.go`), - run from the daily DB dump and the periodic `RefreshCache` (idempotent checksum-skip). The non-secret - env comes from `StackDataProvider.GetStackRecoveryInfo` (excludes secret-named + encrypted values, so - the capture never touches a secret). `data_key` fields are marked in `.felhom.yml` - (`DeployField.DataKey`). + run from the daily DB dump and the periodic `RefreshCache` (idempotent checksum-skip); the split itself + is in `buildUnitAppYaml`. The env + portable values come from `StackDataProvider.GetStackRecoveryInfo`, + which keeps `NonSecretEnv` and the secret set disjoint by construction. `data_key` fields are marked in + `.felhom.yml` (`DeployField.DataKey`). +- **A schema-1 (pre-D5) unit carries no secrets** and still restores from the guest — the restore + degrades rather than failing, and the next capture rewrites the unit (the app.yaml checksum changes). +- **Consequence for the other tiers:** the unit is copied by Tier 2 (another customer drive, plaintext, + same reasoning) and pushed offsite by restic (`offbox.go` — encrypted at rest under the customer-owned + repo password). Neither tier's code changed; the secrets simply travel with the unit they already carried. - **Restore replays the DB dump (F17, v0.61.0; re-sequenced v0.153.0, R-47).** `RestoreFromRecoveryUnit` (and the `RestoreApp` fallback) stops the app → restores named-volume tars → recreates the compose definition and persists the recovered env (`RecreateStackDefinitionFromUnit` — **starts nothing**) diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index e70c675..b59d864 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -1440,12 +1440,14 @@ func (a *stackAdapter) GetStackHDDPath(name string) string { return "" } -// GetStackRecoveryInfo gathers the SECRET-FREE inputs for an app's recovery unit (Phase 2): the -// stack dir, pinned image tags, the non-secret env, and the NAMES of secret/data-key env vars. -// It deliberately does NOT decrypt or return any secret value — secret/password fields are stored -// encrypted in app.yaml, so excluding them (plus a defensive crypto.IsEncrypted guard) yields a -// plaintext, secret-free env. The actual secret values are recovered at restore time from the -// guest's own app.yaml (live, or via the PBS whole-guest snapshot), never from the unit. +// GetStackRecoveryInfo gathers the inputs for an app's recovery unit: the stack dir, pinned image +// tags, the non-secret env, the NAMES of every secret/data-key env var, and — since D5 — the decrypted +// VALUES of the PORTABLE secret class. +// +// The non-secret env still excludes every named secret (plus a defensive crypto.IsEncrypted guard), so +// the two sets are disjoint by construction and a secret can only reach the unit by being in the +// portable class. The EXCLUDED class (`type: password`, and the nonPortableSecrets register) is +// name-only here and is recovered from the guest — or regenerated (O4) — exactly as before. func (a *stackAdapter) GetStackRecoveryInfo(name string) (backup.RecoveryInfo, bool) { s, ok := a.mgr.GetStack(name) if !ok { @@ -1482,13 +1484,31 @@ func (a *stackAdapter) GetStackRecoveryInfo(name string) (backup.RecoveryInfo, b } } + // D5: decrypt the portable class so it can travel in the unit. Reuses the SAME decrypt path as the + // restore side (LoadAppConfigDecrypted), so there is one way to turn app.yaml into plaintext, not + // two. A name whose value is absent/empty is simply omitted — the restore's fail-closed data-key + // gate is what decides whether that is survivable. + portableNames := stacks.PortableSecretEnvVars(&meta) + portable := make(map[string]string, len(portableNames)) + if len(portableNames) > 0 { + if dec := stacks.LoadAppConfigDecrypted(stackDir, a.encKey); dec != nil { + for _, n := range portableNames { + if v, ok := dec.Env[n]; ok && v != "" { + portable[n] = v + } + } + } + } + return backup.RecoveryInfo{ - StackDir: stackDir, - DisplayName: s.Meta.DisplayName, - ImagePins: backup.ParseComposeImages(s.ComposePath), - NonSecretEnv: nonSecret, - SecretEnvVars: secretNames, - DataKeyEnvVars: dataKeys, + StackDir: stackDir, + DisplayName: s.Meta.DisplayName, + ImagePins: backup.ParseComposeImages(s.ComposePath), + NonSecretEnv: nonSecret, + SecretEnvVars: secretNames, + DataKeyEnvVars: dataKeys, + PortableSecretEnvVars: portableNames, + PortableSecrets: portable, }, true } diff --git a/controller/internal/appbackup/appdata.go b/controller/internal/appbackup/appdata.go index b8ff70d..d872188 100644 --- a/controller/internal/appbackup/appdata.go +++ b/controller/internal/appbackup/appdata.go @@ -28,10 +28,10 @@ type StackDataProvider interface { StopStack(name string) error StartStack(name string) error RefreshAndIsRunning(name string) bool - // GetStackRecoveryInfo returns the data needed to capture a SECRET-FREE recovery unit - // (Phase 2): the stack dir, pinned image tags, the non-secret env, and the NAMES of the - // secret/data-key env vars (values are NEVER returned — they are recovered at restore time - // from the guest's own app.yaml, live or via the PBS whole-guest snapshot). ok=false if the + // GetStackRecoveryInfo returns the data needed to capture a recovery unit: the stack dir, + // pinned image tags, the non-secret env, the NAMES of the secret/data-key env vars, and (D5) + // the decrypted VALUES of the portable class. A WITHHELD secret's value is never returned — + // it is recovered at restore time from the guest's app.yaml, or regenerated. ok=false if the // stack is unknown. GetStackRecoveryInfo(name string) (RecoveryInfo, bool) @@ -62,17 +62,27 @@ type StackDataProvider interface { GetStackClassifiedBinds(name string) ([]ClassifiedBind, bool) } -// RecoveryInfo carries everything needed to write a secret-free recovery unit for a stack. -// It deliberately holds NO secret values — only the names of secret/data-key env vars, so the -// manifest can record what must be recovered from elsewhere (guest app.yaml / PBS) without the -// unit ever storing a secret or a data-encrypting key. +// RecoveryInfo carries everything needed to write a recovery unit for a stack. +// +// D5: it now carries the VALUES of the PORTABLE secret class (stacks.PortableSecretEnvVars — every +// `type: secret` field bar the nonPortableSecrets register), because a Tier-1/2 restore that depends +// on the guest for a data-encrypting key or a DB password is not independent of the guest at all: the +// data sits safely on the drive and cannot be read back. The EXCLUDED class (`type: password` admin +// logins) is still name-only and never leaves the guest. type RecoveryInfo struct { StackDir string // dir holding docker-compose.yml + .felhom.yml + app.yaml DisplayName string // app display name ImagePins []string // pinned image tags from compose `image:` lines (re-pulled on restore) - NonSecretEnv map[string]string // env with all secret/password/data-key values removed (plaintext only) - SecretEnvVars []string // NAMES of stripped secret/password fields (recovered from guest/PBS) + NonSecretEnv map[string]string // env with ALL secret/password values removed (plaintext only) + SecretEnvVars []string // NAMES of every secret/password field DataKeyEnvVars []string // NAMES of data-encrypting-key fields (fail-closed gate on restore) + // PortableSecretEnvVars are the NAMES of the secrets that travel in the unit (D5), and + // PortableSecrets their DECRYPTED values. A name present here but absent from PortableSecrets was + // unset/empty in the guest's app.yaml — the restore's fail-closed gate decides what that means. + // Never logged, never in the manifest's value space: the values reach disk only inside the unit's + // 0600 app.yaml. + PortableSecretEnvVars []string + PortableSecrets map[string]string } // ParseComposeImages extracts the pinned image references (`image: repo:tag`) from a diff --git a/controller/internal/appbackup/paths.go b/controller/internal/appbackup/paths.go index ad5e961..5686b51 100644 --- a/controller/internal/appbackup/paths.go +++ b/controller/internal/appbackup/paths.go @@ -40,9 +40,10 @@ func PrimaryBackupPath(nsRoot string) string { // RecoveryUnitPath returns the per-app self-contained recovery-unit ROOT under a namespace root. // It is the existing per-app backup dir (`backups/primary//`) — the legacy name is kept so the // db-dumps/ and volume-dumps/ already written there need no migration; the unit gains compose/ and -// manifest.json as siblings, making the whole dir a complete, recreatable unit (Phase 2). The unit is -// secret-free: secrets/data-keys are recovered from the guest's own app.yaml (live or via PBS), never -// stored here. See backup.recoveryUnit / restore for the capture + restore flow. +// manifest.json as siblings, making the whole dir a complete, recreatable unit (Phase 2). Since D5 the +// unit's compose/app.yaml CARRIES the portable secret class (data keys, DB passwords, internal signing +// secrets) at mode 0600, so a Tier-1/2 restore needs the drive and nothing else; internet-reachable +// admin logins are still withheld. See backup.recoveryUnit / restore for the capture + restore flow. func RecoveryUnitPath(nsRoot, stackName string) string { return filepath.Join(nsRoot, "backups", "primary", stackName) } diff --git a/controller/internal/backup/recovery_unit.go b/controller/internal/backup/recovery_unit.go index 761714e..98875b3 100644 --- a/controller/internal/backup/recovery_unit.go +++ b/controller/internal/backup/recovery_unit.go @@ -15,20 +15,28 @@ import ( "gopkg.in/yaml.v3" ) -// RecoveryManifest describes an app's self-contained, SECRET-FREE recovery unit (Phase 2). +// RecoveryManifest describes an app's self-contained recovery unit. // // The unit on a drive is `/backups/primary//` and contains: // -// compose/ docker-compose.yml + .felhom.yml + a SECRET-STRIPPED app.yaml +// compose/ docker-compose.yml + .felhom.yml + app.yaml (0600; carries the PORTABLE secrets) // db-dumps/ app-consistent DB dump(s) (written by the dump flow) // volume-dumps/ named-volume tars (written by the dump flow) // manifest.json this file // -// The unit holds NO secret values, NO data-encrypting keys, and NOT the Docker image — only the -// pinned image tag(s) (re-pulled on restore) and the NAMES of the secret/data-key env vars. The -// secret values are recovered at restore time from the guest's own app.yaml (live on the rootfs, -// or via the PBS whole-guest snapshot) — see Restore. "Restore from the unit alone" is therefore -// honestly "unit + the guest's app.yaml"; SecretSource records that dependency explicitly. +// D5 (schema 2) changed what the unit holds. Before it held NO secret at all, which made +// "restore from the drive alone" false: the fast, local, customer-doable Tier-1/2 restore secretly +// depended on the slow, operator-driven whole-guest restore, because a data-encrypting key or a DB +// password absent from the guest cannot be regenerated without rendering the restored data +// unreachable. The unit now carries the PORTABLE secret class (stacks.PortableSecretEnvVars) in its +// 0600 app.yaml, and Tier-1/2 needs the DRIVE AND NOTHING ELSE. +// +// It still holds NO `type: password` admin login (those are internet-reachable, so their blast radius +// is not bounded by the drive — they stay in the guest and are regenerated on restore) and NOT the +// Docker image, only the pinned tag(s), re-pulled on restore. SecretSource records the split. +// +// A schema-1 unit carries no secrets: the restore degrades to the pre-D5 guest-only behaviour rather +// than failing, and the next capture rewrites it (the app.yaml checksum changes). type RecoveryManifest struct { SchemaVersion int `json:"schema_version"` AppName string `json:"app_name"` @@ -38,13 +46,17 @@ type RecoveryManifest struct { Drive string `json:"drive"` // HDD_PATH (in-guest mount) NamespaceRoot string `json:"namespace_root"` // resolved felhom-data namespace root ImagePins []string `json:"image_pins"` // image NOT stored — re-pulled on restore - SecretEnvVars []string `json:"secret_env_vars"` // NAMES only — recovered from guest/PBS + SecretEnvVars []string `json:"secret_env_vars"` // NAMES of every secret/password field DataKeyEnvVars []string `json:"data_key_env_vars"` // fail-closed gate on restore SecretSource string `json:"secret_source"` // human note: where secrets come from ConfigFiles []string `json:"config_files"` // captured into compose/ DBDumps []string `json:"db_dumps"` VolumeDumps []string `json:"volume_dumps"` Checksums map[string]string `json:"checksums"` // sha256 of captured compose/ files + // PortableSecretEnvVars (D5) are the NAMES of the secrets this unit's app.yaml CARRIES. Names only + // — the manifest is 0644 and never holds a value. The restore reads it to know which app.yaml env + // entries are secrets rather than plain config; absent (schema 1) ⇒ the unit carries none. + PortableSecretEnvVars []string `json:"portable_secret_env_vars,omitempty"` // R-43/R-44 (v0.148.0): the coherence stamp. An offsite run refreshes the dumps FIRST and then // captures the unit, so a manifest carrying an OffsiteRunID asserts "the db-dumps/ in this unit // were taken by that run" — i.e. the snapshot is an internally coherent {DB@T, files@T} pair. @@ -68,9 +80,11 @@ func (m *Manager) SetTier2Notifier(fn func(stackName, destLabel string, dur time m.tier2Notify = fn } -// CaptureRecoveryUnit writes/refreshes an app's secret-free recovery unit: it captures the -// compose + metadata + a secret-stripped app.yaml into compose/, enumerates the DB/volume dumps -// already present, and writes manifest.json. It NEVER writes a secret value or the Docker image. +// CaptureRecoveryUnit writes/refreshes an app's recovery unit: it captures the compose + metadata + +// an app.yaml carrying the PORTABLE secret class (D5) into compose/, enumerates the DB/volume dumps +// already present, and writes manifest.json. It never writes the Docker image (only the pinned tag), +// and never writes a WITHHELD secret — the split is decided in buildUnitAppYaml, pinned by +// TestCaptureRecoveryUnitCarriesPortableSecretsOnly. // // Idempotent: it builds the captured content in memory first and SKIPS all writes when the unit is // already current (same config checksums, same dump set, same controller version) — so it can run on @@ -107,7 +121,7 @@ func (m *Manager) CaptureRecoveryUnit(stackName string) error { checksums[fname] = sha256Hex(data) configFiles = append(configFiles, fname) } - appYaml := buildStrippedAppYaml(info) + appYaml := buildUnitAppYaml(info) files = append(files, capFile{"app.yaml", appYaml, 0600}) checksums["app.yaml"] = sha256Hex(appYaml) configFiles = append(configFiles, "app.yaml") @@ -151,30 +165,34 @@ func (m *Manager) CaptureRecoveryUnit(stackName string) error { } manifest := &RecoveryManifest{ - SchemaVersion: 1, - AppName: stackName, - DisplayName: info.DisplayName, - ControllerVer: version, - CreatedAt: time.Now().UTC().Format(time.RFC3339), - Drive: drivePath, - NamespaceRoot: nsRoot, - ImagePins: info.ImagePins, - SecretEnvVars: info.SecretEnvVars, - DataKeyEnvVars: info.DataKeyEnvVars, - SecretSource: "guest app.yaml (live rootfs) or PBS whole-guest snapshot — never stored in this unit", - ConfigFiles: configFiles, - DBDumps: dbDumps, - VolumeDumps: volDumps, - Checksums: checksums, - OffsiteRunID: runID, - DumpsAt: dumpsAt, + SchemaVersion: 2, // D5: compose/app.yaml carries the portable secret class + AppName: stackName, + DisplayName: info.DisplayName, + ControllerVer: version, + CreatedAt: time.Now().UTC().Format(time.RFC3339), + Drive: drivePath, + NamespaceRoot: nsRoot, + ImagePins: info.ImagePins, + SecretEnvVars: info.SecretEnvVars, + DataKeyEnvVars: info.DataKeyEnvVars, + PortableSecretEnvVars: info.PortableSecretEnvVars, + SecretSource: "portable secrets (data keys, DB passwords, internal signing secrets) are IN this unit's compose/app.yaml (0600); internet-reachable admin logins are NOT, and come from the guest's app.yaml or are regenerated on restore", + ConfigFiles: configFiles, + DBDumps: dbDumps, + VolumeDumps: volDumps, + Checksums: checksums, + OffsiteRunID: runID, + DumpsAt: dumpsAt, } if err := writeManifest(manifestPath, manifest); err != nil { return fmt.Errorf("writing manifest: %w", err) } - m.logger.Printf("[INFO] [backup] Recovery unit captured for %s → %s (images=%d, secrets-referenced=%d, data_keys=%d)", - stackName, RecoveryUnitPath(nsRoot, stackName), len(info.ImagePins), len(info.SecretEnvVars), len(info.DataKeyEnvVars)) + // Counts and NAMES only — never a value (D5 puts more secrets through this path than before). + m.logger.Printf("[INFO] [backup] Recovery unit captured for %s → %s (images=%d, secrets-referenced=%d, data_keys=%d, portable-carried=%d/%d, withheld=%d)", + stackName, RecoveryUnitPath(nsRoot, stackName), len(info.ImagePins), len(info.SecretEnvVars), + len(info.DataKeyEnvVars), len(info.PortableSecrets), len(info.PortableSecretEnvVars), + len(withheldSecretNames(info))) return nil } @@ -201,29 +219,63 @@ func (m *Manager) versionLocked() string { return m.version } -// strippedAppYaml is the on-disk shape of the secret-free app.yaml captured into the unit. +// strippedAppYaml is the on-disk shape of the app.yaml captured into the unit. The name is historical: +// since D5 the `env` map carries the PORTABLE secrets alongside the plain config (see buildUnitAppYaml). type strippedAppYaml struct { Deployed bool `yaml:"deployed"` Env map[string]string `yaml:"env"` } -// buildStrippedAppYaml renders a secret-free app.yaml (non-secret env only) as bytes. Deterministic: -// yaml.v3 sorts map keys and the secret-name list comes in stable metadata order, so identical input -// yields identical bytes (needed for the checksum-skip guard). -func buildStrippedAppYaml(info RecoveryInfo) []byte { - body, err := yaml.Marshal(strippedAppYaml{Deployed: true, Env: info.NonSecretEnv}) +// buildUnitAppYaml renders the unit's app.yaml as bytes: the non-secret env PLUS the portable secret +// values (D5). Deterministic: yaml.v3 sorts map keys and the name lists come in stable metadata order, +// so identical input yields identical bytes (needed for the checksum-skip guard). +// +// This is the ONE place the capture side decides what does and does not reach the drive — there is no +// second path that writes a unit app.yaml. The caller writes the result 0600. +func buildUnitAppYaml(info RecoveryInfo) []byte { + env := make(map[string]string, len(info.NonSecretEnv)+len(info.PortableSecrets)) + for k, v := range info.NonSecretEnv { + env[k] = v + } + // Portable secrets last: NonSecretEnv is disjoint from the secret set by construction + // (GetStackRecoveryInfo), so this cannot shadow a plain config value. + for k, v := range info.PortableSecrets { + env[k] = v + } + body, err := yaml.Marshal(strippedAppYaml{Deployed: true, Env: env}) if err != nil { body = []byte("deployed: true\nenv: {}\n") } - header := "# Captured by felhom-controller recovery unit — SECRET-FREE.\n" + - "# Secret/data-key values are intentionally omitted; recover them at restore from the\n" + - "# guest's own app.yaml (live rootfs, or the PBS whole-guest snapshot). Stripped names:\n" - if len(info.SecretEnvVars) > 0 { - header += "# " + strings.Join(info.SecretEnvVars, ", ") + "\n" + header := "# Captured by felhom-controller recovery unit.\n" + + "# This file CARRIES SECRETS (D5) so a Tier-1/2 restore needs the drive and nothing else:\n" + + "# data-encrypting keys, database passwords and internal signing secrets. Mode 0600.\n" + if len(info.PortableSecretEnvVars) > 0 { + header += "# Carried: " + strings.Join(info.PortableSecretEnvVars, ", ") + "\n" + } + // The withheld class is named, not valued — an operator reading the unit must be able to see WHY a + // credential is missing rather than suspecting a capture bug. + if withheld := withheldSecretNames(info); len(withheld) > 0 { + header += "# WITHHELD (internet-reachable logins — stay in the guest, regenerated on restore): " + + strings.Join(withheld, ", ") + "\n" } return []byte(header + string(body)) } +// withheldSecretNames returns the secret names deliberately NOT carried by the unit, in stable order. +func withheldSecretNames(info RecoveryInfo) []string { + portable := make(map[string]bool, len(info.PortableSecretEnvVars)) + for _, n := range info.PortableSecretEnvVars { + portable[n] = true + } + var out []string + for _, n := range info.SecretEnvVars { + if !portable[n] { + out = append(out, n) + } + } + return out +} + // writeManifest writes the manifest JSON atomically. func writeManifest(dst string, manifest *RecoveryManifest) error { data, err := json.MarshalIndent(manifest, "", " ") diff --git a/controller/internal/backup/recovery_unit_test.go b/controller/internal/backup/recovery_unit_test.go index d1a8084..25958f5 100644 --- a/controller/internal/backup/recovery_unit_test.go +++ b/controller/internal/backup/recovery_unit_test.go @@ -70,12 +70,21 @@ func (f *fakeRecoveryProvider) StartStackServices(_ string, services []string) e return f.startSvcErr } -// TestCaptureRecoveryUnitIsSecretFree proves the captured unit (a) contains compose+config+manifest, -// (b) enumerates the existing dumps, and (c) is SECRET-FREE: a secret value present in the SOURCE -// app.yaml does NOT appear anywhere in the unit, because the capture writes the stripped NonSecretEnv -// (not the raw app.yaml). The manifest records the secret NAMES + data_key flag for recovery-from-guest. -func TestCaptureRecoveryUnitIsSecretFree(t *testing.T) { - const secretVal = "SUPERSECRETVALUE-do-not-leak" +// TestCaptureRecoveryUnitCarriesPortableSecretsOnly proves the captured unit (a) contains +// compose+config+manifest, (b) enumerates the existing dumps, and (c) implements the D5 secret split: +// the PORTABLE class is written into the unit's app.yaml, and the WITHHELD class appears NOWHERE in +// the unit. +// +// This test replaces TestCaptureRecoveryUnitIsSecretFree, whose global "no secret value appears in the +// unit" invariant D5 deliberately overturns for the portable class. The wrong-outcome half — the +// withheld value must still leak nowhere — is kept verbatim, because that is the half that is still a +// security boundary. +func TestCaptureRecoveryUnitCarriesPortableSecretsOnly(t *testing.T) { + const ( + dataKeyVal = "DATAKEY-must-travel-or-the-data-is-unreadable" + dbPwVal = "DBPASSWORD-must-travel-or-the-app-cannot-authenticate" + withheldVal = "ADMINLOGIN-must-never-reach-the-drive" + ) tmp := t.TempDir() stackDir := filepath.Join(tmp, "stack") drive := filepath.Join(tmp, "drive") // in-guest namespace root (basename need not be felhom-data) @@ -83,25 +92,30 @@ func TestCaptureRecoveryUnitIsSecretFree(t *testing.T) { t.Fatal(err) } - // Source stack files — the raw app.yaml DELIBERATELY holds a secret to prove it's not copied. mustWrite(t, filepath.Join(stackDir, "docker-compose.yml"), "services:\n app:\n image: example/app:1.2.3\n") mustWrite(t, filepath.Join(stackDir, ".felhom.yml"), "display_name: Example\n") + // The SOURCE app.yaml holds the withheld admin login too, so the leak check below is not vacuous: + // it fails if anything ever copies the raw app.yaml into the unit instead of the generated one. mustWrite(t, filepath.Join(stackDir, "app.yaml"), - "deployed: true\nenv:\n DB_PASSWORD: "+secretVal+"\n SUBDOMAIN: example\n") + "deployed: true\nenv:\n DB_PASSWORD: "+dbPwVal+"\n ADMIN_PASSWORD: "+withheldVal+ + "\n SUBDOMAIN: example\n") // Pre-existing dumps (written by the dump flow before capture). mustWrite(t, filepath.Join(AppDBDumpPath(drive, "example"), "example-postgres.sql"), "dump") mustWrite(t, filepath.Join(AppVolumeDumpPath(drive, "example"), "example_data.tar"), "tar") - // RecoveryInfo as the adapter would build it: secret values already stripped from NonSecretEnv. + // RecoveryInfo as the adapter builds it: NonSecretEnv holds no secret, PortableSecrets holds the + // decrypted portable class, and ADMIN_PASSWORD is named in SecretEnvVars but NOT portable. info := RecoveryInfo{ - StackDir: stackDir, - DisplayName: "Example", - ImagePins: []string{"example/app:1.2.3"}, - NonSecretEnv: map[string]string{"SUBDOMAIN": "example", "HDD_PATH": drive}, - SecretEnvVars: []string{"DB_PASSWORD", "SECRET_KEY"}, - DataKeyEnvVars: []string{"SECRET_KEY"}, + StackDir: stackDir, + DisplayName: "Example", + ImagePins: []string{"example/app:1.2.3"}, + NonSecretEnv: map[string]string{"SUBDOMAIN": "example", "HDD_PATH": drive}, + SecretEnvVars: []string{"DB_PASSWORD", "SECRET_KEY", "ADMIN_PASSWORD"}, + DataKeyEnvVars: []string{"SECRET_KEY"}, + PortableSecretEnvVars: []string{"DB_PASSWORD", "SECRET_KEY"}, + PortableSecrets: map[string]string{"DB_PASSWORD": dbPwVal, "SECRET_KEY": dataKeyVal}, } m := &Manager{ logger: log.New(io.Discard, "", 0), @@ -136,8 +150,8 @@ func TestCaptureRecoveryUnitIsSecretFree(t *testing.T) { if len(man.ImagePins) != 1 || man.ImagePins[0] != "example/app:1.2.3" { t.Errorf("image pins: %v", man.ImagePins) } - if len(man.SecretEnvVars) != 2 { - t.Errorf("secret env-var names: %v (want 2)", man.SecretEnvVars) + if len(man.SecretEnvVars) != 3 { + t.Errorf("secret env-var names: %v (want 3)", man.SecretEnvVars) } if len(man.DataKeyEnvVars) != 1 || man.DataKeyEnvVars[0] != "SECRET_KEY" { t.Errorf("data-key env-vars: %v", man.DataKeyEnvVars) @@ -145,21 +159,48 @@ func TestCaptureRecoveryUnitIsSecretFree(t *testing.T) { if len(man.DBDumps) != 1 || len(man.VolumeDumps) != 1 { t.Errorf("dumps enumerated: db=%v vol=%v", man.DBDumps, man.VolumeDumps) } - - // app.yaml in the unit must carry the non-secret env but NOT the secret value. - appy := mustRead(t, filepath.Join(composeDir, "app.yaml")) - if !strings.Contains(appy, "SUBDOMAIN") { - t.Errorf("stripped app.yaml missing non-secret env: %s", appy) + // D5: schema 2 + the carried names, so the restore can tell secrets from plain config. + if man.SchemaVersion != 2 { + t.Errorf("schema version = %d, want 2 (D5 units carry secrets)", man.SchemaVersion) + } + if len(man.PortableSecretEnvVars) != 2 { + t.Errorf("portable secret names: %v (want DB_PASSWORD + SECRET_KEY)", man.PortableSecretEnvVars) + } + // The manifest is 0644 — it must record NAMES, never a value. + if s := string(mfData); strings.Contains(s, dbPwVal) || strings.Contains(s, dataKeyVal) { + t.Error("SECRET LEAK: a secret VALUE reached manifest.json (names only)") } - // SECRET-FREE invariant: the secret value must not appear ANYWHERE in the unit. + // app.yaml in the unit must carry the non-secret env AND the portable secrets. + appyPath := filepath.Join(composeDir, "app.yaml") + appy := mustRead(t, appyPath) + if !strings.Contains(appy, "SUBDOMAIN") { + t.Errorf("unit app.yaml missing non-secret env: %s", appy) + } + // THE CONSEQUENCE of D5 at capture time: without these two values on the drive, a guest-less + // restore cannot read the data sitting beside them. + if !strings.Contains(appy, dataKeyVal) { + t.Error("data-encrypting key did NOT travel — a guest-less restore would be impossible") + } + if !strings.Contains(appy, dbPwVal) { + t.Error("DB password did NOT travel — the restored app could not authenticate to its own data") + } + // Secret-bearing ⇒ owner-only. + if fi, err := os.Stat(appyPath); err != nil { + t.Fatal(err) + } else if perm := fi.Mode().Perm(); perm != 0600 { + t.Errorf("unit app.yaml mode = %04o, want 0600 (it carries secrets)", perm) + } + + // THE WRONG-OUTCOME CHECK: the withheld class must appear NOWHERE in the unit. This is the half of + // the old secret-free invariant that D5 does not relax. unitRoot := RecoveryUnitPath(drive, "example") _ = filepath.WalkDir(unitRoot, func(path string, d fs.DirEntry, err error) error { if err != nil || d.IsDir() { return nil } - if strings.Contains(mustRead(t, path), secretVal) { - t.Errorf("SECRET LEAK: %q found in %s", secretVal, path) + if strings.Contains(mustRead(t, path), withheldVal) { + t.Errorf("SECRET LEAK: withheld admin login %q found in %s", withheldVal, path) } return nil }) diff --git a/controller/internal/backup/restore_unit.go b/controller/internal/backup/restore_unit.go index e24d681..3c208ac 100644 --- a/controller/internal/backup/restore_unit.go +++ b/controller/internal/backup/restore_unit.go @@ -11,27 +11,53 @@ import ( ) // reconcileRestoreSecrets merges the recovery unit's non-secret env with the secrets recovered from -// the guest's own app.yaml, and applies the FAIL-CLOSED data-key gate. It is the safety-critical heart -// of Phase 2b and is deliberately a pure function (no I/O) so it can be exhaustively unit-tested. +// the unit itself (D5) and from the guest's own app.yaml, and applies the FAIL-CLOSED data-key gate. +// It is the safety-critical heart of Phase 2b and is deliberately a pure function (no I/O) so it can +// be exhaustively unit-tested — the D5 source arrives as an ARGUMENT, not as a read. // -// Policy (per the Phase 2 design — see REPORT/CHANGELOG): -// - Regenerate NOTHING. Every secret comes from the guest (live rootfs, or PBS whole-guest restore). +// Policy: +// - Regenerate NOTHING here. Secrets come from the unit (portable class) or the guest (the rest). // - A missing DATA-ENCRYPTING key (`dataKeyNames`) is FATAL: regenerating it would render the // restored data unreadable, so we refuse and tell the operator to do a PBS whole-guest restore. -// - A missing resettable secret (DB password, admin password) is NON-fatal: it's returned in -// `missing` so the caller can warn; the app may simply need a credential reset, no data is lost. -func reconcileRestoreSecrets(nonSecretEnv, recoveredSecrets map[string]string, secretNames, dataKeyNames []string) (fullEnv map[string]string, missing []string, err error) { +// D5 means the key is normally IN the unit — but "normally" is not a reason to soften the gate. +// - A missing resettable secret is NON-fatal: returned in `missing` so the caller can warn or +// regenerate it (O4). No data is lost. +// +// PRECEDENCE — the UNIT WINS over the guest when both hold a value for the same name. +// +// This is not arbitrary and it is not "newest wins". The unit's secrets are captured in the SAME run +// as the dumps beside them (runVolumeDumps → captureAllRecoveryUnits, backup.go), so the unit's value +// is the one that MATCHES THE DATA ABOUT TO BE RESTORED, whereas the guest's value is merely the most +// recent. Where they disagree the guest's has been rotated since the capture, and preferring it is +// precisely the data-loss bug: +// - a rotated data-encrypting key does not decrypt data encrypted with the old one; +// - a rotated DB password does not match the scram/mysql hash inside the restored data directory +// (POSTGRES_PASSWORD is ignored once PGDATA is non-empty), so the app cannot reach its own rows. +// +// The restore persists fullEnv back to the guest's app.yaml (RecreateStackDefinitionFromUnit), so +// unit-wins also leaves the guest consistent with the data now on disk. +func reconcileRestoreSecrets(nonSecretEnv, unitSecrets, guestSecrets map[string]string, secretNames, dataKeyNames []string) (fullEnv map[string]string, missing []string, err error) { fullEnv = make(map[string]string, len(nonSecretEnv)+len(secretNames)) for k, v := range nonSecretEnv { fullEnv[k] = v } + // resolve applies the precedence: unit first, guest only as a fallback. + resolve := func(n string) (string, bool) { + if v, ok := unitSecrets[n]; ok && v != "" { + return v, true + } + if v, ok := guestSecrets[n]; ok && v != "" { + return v, true + } + return "", false + } have := func(n string) bool { - v, ok := recoveredSecrets[n] - return ok && v != "" + _, ok := resolve(n) + return ok } for _, n := range secretNames { - if have(n) { - fullEnv[n] = recoveredSecrets[n] + if v, ok := resolve(n); ok { + fullEnv[n] = v } else { missing = append(missing, n) } @@ -45,24 +71,42 @@ func reconcileRestoreSecrets(nonSecretEnv, recoveredSecrets map[string]string, s } if len(missingDataKeys) > 0 { return nil, missing, fmt.Errorf( - "refusing to restore: data-encrypting key(s) %v could not be recovered from the guest's app.yaml — "+ + "refusing to restore: data-encrypting key(s) %v are in NEITHER the recovery unit nor the guest's app.yaml — "+ "a PBS whole-guest restore is required first (regenerating the key would render stored data unreadable)", missingDataKeys) } return fullEnv, missing, nil } -// readStrippedEnv parses the non-secret env from a recovery unit's secret-stripped app.yaml. -func readStrippedEnv(path string) map[string]string { +// readUnitEnv parses a recovery unit's app.yaml and SPLITS it into the plain config env and the +// secrets the unit carries (D5), using the manifest's portable-secret names as the discriminator. +// +// The split is driven by the MANIFEST, not by guessing from key names: the manifest and the app.yaml +// are captured together and checksummed together, so they cannot disagree about which entries are +// secrets. A schema-1 unit has no portable names, so everything lands in nonSecret — exactly the +// pre-D5 behaviour, which is what makes an old unit still restorable. +func readUnitEnv(path string, portableNames []string) (nonSecret, unitSecrets map[string]string) { + nonSecret, unitSecrets = map[string]string{}, map[string]string{} data, err := os.ReadFile(path) if err != nil { - return map[string]string{} + return nonSecret, unitSecrets } var s strippedAppYaml if yaml.Unmarshal(data, &s) != nil || s.Env == nil { - return map[string]string{} + return nonSecret, unitSecrets } - return s.Env + isPortable := make(map[string]bool, len(portableNames)) + for _, n := range portableNames { + isPortable[n] = true + } + for k, v := range s.Env { + if isPortable[k] { + unitSecrets[k] = v + continue + } + nonSecret[k] = v + } + return nonSecret, unitSecrets } // hasReplayableDump reports whether dumpDir holds a .sql dump that the replay could actually use. @@ -85,13 +129,17 @@ func hasReplayableDump(dumpDir string) bool { return false } -// RestoreFromRecoveryUnit recreates an app from its on-drive recovery unit + the guest's own secrets. +// RestoreFromRecoveryUnit recreates an app from its on-drive recovery unit. // -// It reads the unit manifest, recovers the secret values from the guest's live app.yaml, applies the -// fail-closed data-key gate, restores the named-volume data from the unit's tars, then restores the -// app's definition from the unit and redeploys it with the reconstructed env (re-pulling the pinned -// image). No secret is ever regenerated, and no secret is read from the unit. If no unit exists it -// falls back to the legacy volume-only RestoreApp. +// It reads the unit manifest, takes the portable secrets from the UNIT and the rest from the guest's +// live app.yaml (unit wins — see reconcileRestoreSecrets), applies the fail-closed data-key gate, +// restores the named-volume data from the unit's tars, then restores the app's definition from the unit +// and redeploys it with the reconstructed env (re-pulling the pinned image). If no unit exists it falls +// back to the legacy volume-only RestoreApp. +// +// D5: this no longer needs the guest. A restore with the guest's app.yaml absent succeeds, which is +// pinned by TestRestoreFromRecoveryUnitWithGuestAbsent — the withheld class is regenerated (O4) and +// only a data key missing from BOTH sources still refuses. func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { if m.stackProvider == nil { return fmt.Errorf("stack provider not configured") @@ -126,11 +174,15 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { } composeDir := RecoveryUnitComposePath(nsRoot, stackName) - nonSecretEnv := readStrippedEnv(filepath.Join(composeDir, "app.yaml")) + nonSecretEnv, unitSecrets := readUnitEnv(filepath.Join(composeDir, "app.yaml"), manifest.PortableSecretEnvVars) - // Recover secrets from the GUEST (never the unit), then apply the fail-closed gate. - recovered := m.stackProvider.RecoverStackSecrets(stackName, manifest.SecretEnvVars) - fullEnv, missing, err := reconcileRestoreSecrets(nonSecretEnv, recovered, manifest.SecretEnvVars, manifest.DataKeyEnvVars) + // D5: the unit carries the portable class, so this is the leg that no longer needs the guest. The + // guest is still consulted for the WITHHELD class (internet-reachable admin logins) and as the + // fallback for a schema-1 unit — it returns an empty map when the guest is gone, which is the whole + // point: a Tier-1/2 restore must survive that. Precedence is unit-over-guest (see + // reconcileRestoreSecrets), then the fail-closed gate. + guestSecrets := m.stackProvider.RecoverStackSecrets(stackName, manifest.SecretEnvVars) + fullEnv, missing, err := reconcileRestoreSecrets(nonSecretEnv, unitSecrets, guestSecrets, manifest.SecretEnvVars, manifest.DataKeyEnvVars) if err != nil { m.logger.Printf("[ERROR] [backup] Restore REFUSED for %s: %v", stackName, err) return err @@ -141,6 +193,16 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { // encrypted in the guest app.yaml and round-trips on the next backup/restore. Data-keys are // never generated: the fail-closed gate above already refused if one was missing, and the // generator itself refuses data-key fields (defense-in-depth). Values are never logged. + // + // D5 shrinks this path to the rare case: the portable class now comes from the unit, so a + // generator run means the secret was empty at capture AND absent from the guest. + // + // It does NOT claim the reset is harmless. R-127: for a DB password it is not — a restored data + // directory keeps the OLD role hash (POSTGRES_PASSWORD is ignored once PGDATA is non-empty), so a + // regenerated value leaves the app unable to authenticate against its own restored rows while the + // dump replay, which uses the container's local trust socket, still reports success. The old wording + // here asserted "stored data is unaffected" for every non-data-key secret; that is false for the 18 + // DB/root-password fields and is now scoped to what is actually true. if len(missing) > 0 { dataKeySet := make(map[string]bool, len(manifest.DataKeyEnvVars)) for _, dk := range manifest.DataKeyEnvVars { @@ -158,7 +220,7 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { unresolved = append(unresolved, name) } if len(generated) > 0 { - m.logger.Printf("[WARN] [backup] Restore %s: generated replacement for %v — the credential was reset (old value unrecoverable); stored data is unaffected (no data-key involved)", + m.logger.Printf("[WARN] [backup] Restore %s: generated replacement for %v — the credential was reset (old value unrecoverable); no data-encrypting key was involved, but a regenerated DATABASE password will not match the restored data directory's stored hash (R-127) — check the app can reach its data", stackName, generated) } if len(unresolved) > 0 { diff --git a/controller/internal/backup/restore_unit_test.go b/controller/internal/backup/restore_unit_test.go index ef1669c..a57d356 100644 --- a/controller/internal/backup/restore_unit_test.go +++ b/controller/internal/backup/restore_unit_test.go @@ -3,10 +3,123 @@ package backup import ( "io" "log" + "os" "path/filepath" + "strings" "testing" ) +// captureFixtureUnit writes a real stack tree, runs the REAL CaptureRecoveryUnit over it, and returns +// the drive (namespace root) holding the resulting unit. +// +// Fixtures come from a CAPTURED unit rather than hand-written YAML deliberately: the capture side and +// the restore side must meet at real bytes on a real filesystem, so a change to the on-disk shape +// (header text, key ordering, the non-secret/portable split) cannot pass by having a test agree with +// itself. Everything from buildUnitAppYaml through readUnitEnv is production code here. +func captureFixtureUnit(t *testing.T, portable map[string]string) (drive string) { + t.Helper() + tmp := t.TempDir() + stackDir := filepath.Join(tmp, "stack") + drive = filepath.Join(tmp, "drive") + if err := os.MkdirAll(stackDir, 0755); err != nil { + t.Fatal(err) + } + mustWrite(t, filepath.Join(stackDir, "docker-compose.yml"), + "services:\n app:\n image: example/app:1\n db:\n image: postgres:16\n") + mustWrite(t, filepath.Join(stackDir, ".felhom.yml"), "display_name: App\n") + mustWrite(t, filepath.Join(stackDir, "app.yaml"), "deployed: true\nenv:\n SUBDOMAIN: trips\n") + + var names []string + for _, n := range []string{"DB_PASSWORD", "SECRET_KEY"} { + if _, ok := portable[n]; ok { + names = append(names, n) + } + } + info := RecoveryInfo{ + StackDir: stackDir, + DisplayName: "App", + ImagePins: []string{"example/app:1"}, + NonSecretEnv: map[string]string{"SUBDOMAIN": "trips"}, + SecretEnvVars: []string{"DB_PASSWORD", "SECRET_KEY"}, + DataKeyEnvVars: []string{"SECRET_KEY"}, + PortableSecretEnvVars: names, + PortableSecrets: portable, + } + m := &Manager{ + logger: log.New(io.Discard, "", 0), + systemDataPath: filepath.Join(tmp, "system"), + stackProvider: &fakeRecoveryProvider{info: info, hdd: drive}, + version: "vtest", + } + if err := m.CaptureRecoveryUnit("app"); err != nil { + t.Fatalf("capture fixture: %v", err) + } + return drive +} + +// TestRestoreFromRecoveryUnitWithGuestAbsent is D5's entire claim, as a test rather than a +// description: a Tier-1/2 restore SUCCEEDS when the guest's app.yaml is unavailable. +// +// SEAM (R-125): injection is at Manager.stackProvider only — i.e. the docker/compose operations and the +// guest's app.yaml decrypt. RecoverStackSecrets returning nil IS the guest being gone: it is exactly +// what the real adapter returns when the stack or its app.yaml cannot be read (main.go +// GetStack/LoadAppConfigDecrypted nil paths). Everything under test is production code: the unit was +// written by the real CaptureRecoveryUnit, read back by the real readUnitEnv, and reconciled by the +// real reconcileRestoreSecrets. +func TestRestoreFromRecoveryUnitWithGuestAbsent(t *testing.T) { + const ( + dataKey = "deadbeefdeadbeef" + dbPw = "pw-from-the-drive" + ) + drive := captureFixtureUnit(t, map[string]string{"DB_PASSWORD": dbPw, "SECRET_KEY": dataKey}) + + // The guest is GONE: no secrets recoverable from it at all. + fake := &fakeRecoveryProvider{hdd: drive, running: true, secrets: nil} + m := &Manager{logger: log.New(io.Discard, "", 0), + systemDataPath: filepath.Join(drive, "..", "sys"), stackProvider: fake} + + if err := m.RestoreFromRecoveryUnit("app"); err != nil { + t.Fatalf("restore must succeed from the drive alone, got: %v", err) + } + if fake.gotEnv == nil { + t.Fatal("recreate was not called — the restore did not reach the redeploy") + } + // The consequence: the app is redeployed with the key that decrypts the data beside it. + if fake.gotEnv["SECRET_KEY"] != dataKey { + t.Errorf("data-encrypting key not recovered from the unit: %q", fake.gotEnv["SECRET_KEY"]) + } + if fake.gotEnv["DB_PASSWORD"] != dbPw { + t.Errorf("DB password not recovered from the unit: %q", fake.gotEnv["DB_PASSWORD"]) + } + if fake.gotEnv["SUBDOMAIN"] != "trips" { + t.Errorf("plain config lost: %v", fake.gotEnv) + } +} + +// TestRestoreFromRecoveryUnitGuestAbsentStillFailsClosed proves D5 did not soften the gate: with the +// data key in NEITHER the unit nor the guest, the restore still REFUSES and mutates nothing. +func TestRestoreFromRecoveryUnitGuestAbsentStillFailsClosed(t *testing.T) { + // Unit carries only the DB password — the data key is absent from both sources. + drive := captureFixtureUnit(t, map[string]string{"DB_PASSWORD": "pw"}) + fake := &fakeRecoveryProvider{hdd: drive, running: true, secrets: nil} + m := &Manager{logger: log.New(io.Discard, "", 0), + systemDataPath: filepath.Join(drive, "..", "sys"), stackProvider: fake} + + err := m.RestoreFromRecoveryUnit("app") + if err == nil { + t.Fatal("expected fail-closed refusal when the data key is in neither source") + } + if !strings.Contains(err.Error(), "SECRET_KEY") { + t.Errorf("refusal should name the missing data key, got: %v", err) + } + if fake.gotEnv != nil { + t.Errorf("recreate must NOT be called on refusal, got %v", fake.gotEnv) + } + if fake.stopped { + t.Error("the live app must not be stopped when the restore refuses") + } +} + // TestRestoreFromRecoveryUnitOrchestration exercises the full in-process flow: read manifest → // recover secrets → apply gate → recreate with the reconciled env. It proves (a) on success the // recreate is called with non-secret env + recovered secrets merged, and (b) on a missing data-key the @@ -15,7 +128,7 @@ func TestRestoreFromRecoveryUnitOrchestration(t *testing.T) { newUnit := func(t *testing.T) (drive string) { tmp := t.TempDir() drive = filepath.Join(tmp, "drive") - // stripped (secret-free) app.yaml in the unit + // A schema-1 unit: no portable secrets, so the guest is the only source (pre-D5 behaviour). mustWrite(t, filepath.Join(RecoveryUnitComposePath(drive, "app"), "app.yaml"), "deployed: true\nenv:\n SUBDOMAIN: trips\n") man := &RecoveryManifest{SchemaVersion: 1, AppName: "app", ControllerVer: "v", @@ -49,6 +162,21 @@ func TestRestoreFromRecoveryUnitOrchestration(t *testing.T) { } }) + t.Run("schema-1 unit still restores from the guest — no regression", func(t *testing.T) { + // An old unit carries nothing; the guest must still be able to supply everything. + drive := newUnit(t) + fake := &fakeRecoveryProvider{hdd: drive, running: true, + secrets: map[string]string{"DB_PASSWORD": "pw", "SECRET_KEY": "deadbeef"}} + m := &Manager{logger: log.New(io.Discard, "", 0), + systemDataPath: filepath.Join(drive, "..", "sys"), stackProvider: fake} + if err := m.RestoreFromRecoveryUnit("app"); err != nil { + t.Fatalf("a pre-D5 unit must still restore: %v", err) + } + if fake.gotEnv["SECRET_KEY"] != "deadbeef" { + t.Errorf("guest fallback lost the data key: %v", fake.gotEnv) + } + }) + t.Run("data-key unrecoverable — REFUSED, recreate not called", func(t *testing.T) { drive := newUnit(t) fake := &fakeRecoveryProvider{ @@ -67,13 +195,82 @@ func TestRestoreFromRecoveryUnitOrchestration(t *testing.T) { }) } +// TestReconcileRestoreSecretsPrecedence pins the D5 precedence rule in BOTH directions. An undefined +// precedence between two sources of a decryption key is a data-loss bug waiting for its first +// disagreement, so this is not a style question. +// +// The UNIT wins: its secrets were captured in the same run as the dumps beside them, so the unit's +// value is the one that matches the data about to be restored. The guest's value is merely the most +// recent — and a rotated key does not decrypt data encrypted with the old one. +func TestReconcileRestoreSecretsPrecedence(t *testing.T) { + nonSecret := map[string]string{"SUBDOMAIN": "trips"} + names := []string{"DB_PASSWORD", "SECRET_KEY"} + dataKeys := []string{"SECRET_KEY"} + + t.Run("both sources disagree — the UNIT wins", func(t *testing.T) { + unit := map[string]string{"DB_PASSWORD": "unit-pw", "SECRET_KEY": "unit-key"} + guest := map[string]string{"DB_PASSWORD": "guest-pw", "SECRET_KEY": "guest-key"} + full, missing, err := reconcileRestoreSecrets(nonSecret, unit, guest, names, dataKeys) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(missing) != 0 { + t.Errorf("missing: %v", missing) + } + if full["SECRET_KEY"] != "unit-key" { + t.Errorf("data key: got %q, want the UNIT's value (it matches the restored data)", full["SECRET_KEY"]) + } + if full["DB_PASSWORD"] != "unit-pw" { + t.Errorf("DB password: got %q, want the UNIT's value (it matches the restored data dir hash)", full["DB_PASSWORD"]) + } + }) + + t.Run("unit silent — the GUEST fills in", func(t *testing.T) { + // The withheld class (admin logins) is never in the unit, so this direction must work too. + guest := map[string]string{"DB_PASSWORD": "guest-pw", "SECRET_KEY": "guest-key"} + full, _, err := reconcileRestoreSecrets(nonSecret, nil, guest, names, dataKeys) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if full["SECRET_KEY"] != "guest-key" || full["DB_PASSWORD"] != "guest-pw" { + t.Errorf("guest fallback not applied: %v", full) + } + }) + + t.Run("unit present but EMPTY for a name — the guest fills in", func(t *testing.T) { + // An empty value is not a value; it must not shadow a good one from the guest. + unit := map[string]string{"SECRET_KEY": ""} + guest := map[string]string{"SECRET_KEY": "guest-key", "DB_PASSWORD": "guest-pw"} + full, _, err := reconcileRestoreSecrets(nonSecret, unit, guest, names, dataKeys) + if err != nil { + t.Fatalf("an empty unit value must fall through to the guest, got: %v", err) + } + if full["SECRET_KEY"] != "guest-key" { + t.Errorf("empty unit value shadowed the guest: %q", full["SECRET_KEY"]) + } + }) + + t.Run("a portable secret never shadows plain config", func(t *testing.T) { + // GetStackRecoveryInfo keeps the two sets disjoint; if that ever breaks, the merge order in + // buildUnitAppYaml decides silently. Pin the intended outcome. + full, _, err := reconcileRestoreSecrets(map[string]string{"DB_PASSWORD": "should-not-win"}, + map[string]string{"DB_PASSWORD": "unit-pw"}, nil, []string{"DB_PASSWORD"}, nil) + if err != nil { + t.Fatal(err) + } + if full["DB_PASSWORD"] != "unit-pw" { + t.Errorf("the secret source must win over a stray non-secret entry: %q", full["DB_PASSWORD"]) + } + }) +} + // TestReconcileRestoreSecrets covers the safety-critical fail-closed gate + secret reconciliation. func TestReconcileRestoreSecrets(t *testing.T) { nonSecret := map[string]string{"SUBDOMAIN": "trips", "DOMAIN": "demo-felhom.eu"} t.Run("all recovered, no data_key — full env, no error", func(t *testing.T) { - recovered := map[string]string{"DB_PASSWORD": "pw", "SECRET_KEY": "deadbeef"} - full, missing, err := reconcileRestoreSecrets(nonSecret, recovered, + guest := map[string]string{"DB_PASSWORD": "pw", "SECRET_KEY": "deadbeef"} + full, missing, err := reconcileRestoreSecrets(nonSecret, nil, guest, []string{"DB_PASSWORD", "SECRET_KEY"}, nil) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -87,9 +284,9 @@ func TestReconcileRestoreSecrets(t *testing.T) { } }) - t.Run("data_key missing — FAIL CLOSED (refuse)", func(t *testing.T) { - recovered := map[string]string{"DB_PASSWORD": "pw"} // SECRET_KEY (a data_key) is gone - full, _, err := reconcileRestoreSecrets(nonSecret, recovered, + t.Run("data_key missing from BOTH sources — FAIL CLOSED (refuse)", func(t *testing.T) { + guest := map[string]string{"DB_PASSWORD": "pw"} // SECRET_KEY (a data_key) is gone + full, _, err := reconcileRestoreSecrets(nonSecret, nil, guest, []string{"DB_PASSWORD", "SECRET_KEY"}, []string{"SECRET_KEY"}) if err == nil { t.Fatal("expected fail-closed error for missing data-encrypting key, got nil") @@ -99,17 +296,34 @@ func TestReconcileRestoreSecrets(t *testing.T) { } }) - t.Run("data_key empty value — FAIL CLOSED", func(t *testing.T) { - recovered := map[string]string{"SECRET_KEY": ""} // present but empty == unrecoverable - _, _, err := reconcileRestoreSecrets(nonSecret, recovered, []string{"SECRET_KEY"}, []string{"SECRET_KEY"}) + t.Run("data_key empty in both — FAIL CLOSED", func(t *testing.T) { + guest := map[string]string{"SECRET_KEY": ""} // present but empty == unrecoverable + _, _, err := reconcileRestoreSecrets(nonSecret, map[string]string{"SECRET_KEY": ""}, guest, + []string{"SECRET_KEY"}, []string{"SECRET_KEY"}) if err == nil { t.Fatal("empty data-key value must fail closed") } }) + t.Run("data_key recovered from the UNIT — no refusal", func(t *testing.T) { + // The D5 case: the guest is gone but the unit carries the key, so the gate must NOT fire. + unit := map[string]string{"SECRET_KEY": "deadbeef", "DB_PASSWORD": "pw"} + full, missing, err := reconcileRestoreSecrets(nonSecret, unit, nil, + []string{"DB_PASSWORD", "SECRET_KEY"}, []string{"SECRET_KEY"}) + if err != nil { + t.Fatalf("the unit's data key must satisfy the gate: %v", err) + } + if len(missing) != 0 { + t.Errorf("nothing should be missing: %v", missing) + } + if full["SECRET_KEY"] != "deadbeef" { + t.Errorf("data key wrong: %v", full) + } + }) + t.Run("resettable secret missing — proceed with warning", func(t *testing.T) { - recovered := map[string]string{"SECRET_KEY": "deadbeef"} // data_key ok; DB_PASSWORD missing - full, missing, err := reconcileRestoreSecrets(nonSecret, recovered, + guest := map[string]string{"SECRET_KEY": "deadbeef"} // data_key ok; DB_PASSWORD missing + full, missing, err := reconcileRestoreSecrets(nonSecret, nil, guest, []string{"DB_PASSWORD", "SECRET_KEY"}, []string{"SECRET_KEY"}) if err != nil { t.Fatalf("a missing resettable secret must NOT fail closed: %v", err) @@ -125,3 +339,38 @@ func TestReconcileRestoreSecrets(t *testing.T) { } }) } + +// TestReadUnitEnvSplitsByManifest proves the split is driven by the manifest's portable names, and that +// a schema-1 unit (no names) degrades to "everything is plain config" rather than losing entries. +func TestReadUnitEnvSplitsByManifest(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "app.yaml") + mustWrite(t, path, "deployed: true\nenv:\n SUBDOMAIN: trips\n DB_PASSWORD: pw\n SECRET_KEY: key\n") + + t.Run("named secrets land in unitSecrets, the rest in nonSecret", func(t *testing.T) { + nonSecret, unitSecrets := readUnitEnv(path, []string{"DB_PASSWORD", "SECRET_KEY"}) + if nonSecret["SUBDOMAIN"] != "trips" || len(nonSecret) != 1 { + t.Errorf("nonSecret = %v, want only SUBDOMAIN", nonSecret) + } + if unitSecrets["DB_PASSWORD"] != "pw" || unitSecrets["SECRET_KEY"] != "key" || len(unitSecrets) != 2 { + t.Errorf("unitSecrets = %v, want the two named secrets", unitSecrets) + } + }) + + t.Run("schema-1 (no portable names) — everything is plain config, nothing lost", func(t *testing.T) { + nonSecret, unitSecrets := readUnitEnv(path, nil) + if len(unitSecrets) != 0 { + t.Errorf("a schema-1 unit carries no secrets, got %v", unitSecrets) + } + if len(nonSecret) != 3 { + t.Errorf("nonSecret should keep every entry, got %v", nonSecret) + } + }) + + t.Run("absent file — empty maps, no panic", func(t *testing.T) { + nonSecret, unitSecrets := readUnitEnv(filepath.Join(dir, "nope.yaml"), []string{"DB_PASSWORD"}) + if len(nonSecret) != 0 || len(unitSecrets) != 0 { + t.Errorf("want empty maps, got %v / %v", nonSecret, unitSecrets) + } + }) +} diff --git a/controller/internal/stacks/deploy.go b/controller/internal/stacks/deploy.go index e38fd59..bb12f07 100644 --- a/controller/internal/stacks/deploy.go +++ b/controller/internal/stacks/deploy.go @@ -822,6 +822,48 @@ func SensitiveEnvVars(meta *Metadata) []string { return vars } +// nonPortableSecrets is the register of secrets that must NEVER travel in an on-drive recovery unit +// even though the catalog types them `secret` — i.e. credentials whose reach is NOT bounded by +// physical possession of the drive, because they authenticate against a service published to the +// internet. Keyed by catalog SLUG (never empty — LoadMetadata falls back to the directory name). +// +// It is a CODE register, not a catalog flag, deliberately: the D5 ruling is a security boundary, and +// a boundary a catalog push can silently move is not a boundary (the R-97a lesson — an invariant that +// only configuration enforced). Adding an app whose `type: secret` field gates an internet-reachable +// login means adding a row here. +// +// - vaultwarden/ADMIN_TOKEN gates the /admin panel, served on the app's own public web port. +var nonPortableSecrets = map[string]map[string]bool{ + "vaultwarden": {"ADMIN_TOKEN": true}, +} + +// PortableSecretEnvVars returns the env-var names of secrets that TRAVEL inside the on-drive recovery +// unit (D5), in deterministic metadata order. +// +// The ruling (operator, 2026-07-30): `type: secret` travels, `type: password` does not, minus +// nonPortableSecrets. The line is drawn on REACH, not on whether a secret is nominally resettable: +// +// - Every `type: secret` field either decrypts data sitting on the SAME drive (the 5 declared +// data_keys, plus encryption keys the catalog labels as such but never flagged — see R-127) or +// authenticates to a container on an internal compose network with no external listener (the 18 +// DB/root passwords, and the signing secrets). Possessing it adds nothing to possessing the +// drive, which is exactly D2's argument for keeping the DATA plaintext. +// - Every `type: password` field is an admin/UI login for a published service, so its blast radius +// is NOT bounded by the drive. Those stay in the guest and are regenerated on restore (O4). +// +// Excluding the `type: password` class is what licenses the plaintext ruling; the two are coupled and +// must not be relaxed independently. +func PortableSecretEnvVars(meta *Metadata) []string { + blocked := nonPortableSecrets[meta.Slug] + var vars []string + for _, f := range meta.DeployFields { + if f.Type == "secret" && !blocked[f.EnvVar] { + vars = append(vars, f.EnvVar) + } + } + return vars +} + // --- Secret generation --- const alphanumChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" diff --git a/controller/internal/stacks/portable_secrets_test.go b/controller/internal/stacks/portable_secrets_test.go new file mode 100644 index 0000000..1a40b08 --- /dev/null +++ b/controller/internal/stacks/portable_secrets_test.go @@ -0,0 +1,76 @@ +package stacks + +import "testing" + +// TestPortableSecretEnvVars pins the D5 ruling (operator, 2026-07-30) about WHICH secrets may travel +// on the customer's backup drive: `type: secret` travels, `type: password` does not, minus the +// nonPortableSecrets register. +// +// This is a security boundary, so the test asserts the CONSEQUENCE in both directions — what travels +// and, more importantly, what must not. A change that widens the class fails here. +func TestPortableSecretEnvVars(t *testing.T) { + t.Run("type=secret travels, type=password does not", func(t *testing.T) { + meta := &Metadata{Slug: "paperless-ngx", DeployFields: []DeployField{ + {EnvVar: "DB_PASSWORD", Type: "secret"}, + {EnvVar: "PAPERLESS_SECRET_KEY", Type: "secret"}, + {EnvVar: "PAPERLESS_ADMIN_PASSWORD", Type: "password"}, + {EnvVar: "SUBDOMAIN", Type: "subdomain"}, + }} + got := PortableSecretEnvVars(meta) + want := []string{"DB_PASSWORD", "PAPERLESS_SECRET_KEY"} + if len(got) != len(want) { + t.Fatalf("portable = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { // order must be stable metadata order (checksum-skip depends on it) + t.Errorf("portable[%d] = %q, want %q (got %v)", i, got[i], want[i], got) + } + } + }) + + t.Run("an internet-reachable admin login is WITHHELD even when typed secret", func(t *testing.T) { + meta := &Metadata{Slug: "vaultwarden", DeployFields: []DeployField{ + {EnvVar: "ADMIN_TOKEN", Type: "secret"}, + }} + if got := PortableSecretEnvVars(meta); len(got) != 0 { + t.Errorf("vaultwarden/ADMIN_TOKEN must NOT travel (it gates the public /admin panel), got %v", got) + } + }) + + t.Run("the register is scoped to its app, not the bare env-var name", func(t *testing.T) { + // Another app's ADMIN_TOKEN is not vaultwarden's and must not be caught by the register. + meta := &Metadata{Slug: "some-other-app", DeployFields: []DeployField{ + {EnvVar: "ADMIN_TOKEN", Type: "secret"}, + }} + if got := PortableSecretEnvVars(meta); len(got) != 1 || got[0] != "ADMIN_TOKEN" { + t.Errorf("register must be slug-scoped, got %v", got) + } + }) + + t.Run("no deploy fields — nothing travels", func(t *testing.T) { + if got := PortableSecretEnvVars(&Metadata{Slug: "x"}); len(got) != 0 { + t.Errorf("want empty, got %v", got) + } + }) + + t.Run("portable is a SUBSET of SensitiveEnvVars", func(t *testing.T) { + // The unit's app.yaml splits env into non-secret + portable using two different helpers; if + // PortableSecretEnvVars ever returned a name SensitiveEnvVars does not, that name would land in + // NonSecretEnv as well and the disjointness GetStackRecoveryInfo relies on would break. + meta := &Metadata{Slug: "nextcloud", DeployFields: []DeployField{ + {EnvVar: "DB_PASSWORD", Type: "secret"}, + {EnvVar: "MYSQL_ROOT_PASSWORD", Type: "secret"}, + {EnvVar: "NEXTCLOUD_ADMIN_PASSWORD", Type: "password"}, + {EnvVar: "DOMAIN", Type: "domain"}, + }} + sensitive := make(map[string]bool) + for _, n := range SensitiveEnvVars(meta) { + sensitive[n] = true + } + for _, n := range PortableSecretEnvVars(meta) { + if !sensitive[n] { + t.Errorf("%q is portable but not sensitive — it would also land in NonSecretEnv", n) + } + } + }) +}