From a52851e79e6e91862d833f5de5e8a8dbb72a6bcd Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sun, 5 Jul 2026 11:52:04 +0200 Subject: [PATCH] =?UTF-8?q?fix(backup):=20O4=20=E2=80=94=20generate=20a=20?= =?UTF-8?q?replacement=20for=20unrecoverable=20resettable=20secrets=20on?= =?UTF-8?q?=20restore?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The proceed-path for a missing RESETTABLE secret redeployed the app with the secret blank (compose "Defaulting to a blank string" → exit 1, live-hit in the 2026-07-04 drill Phase 5). Now the restore generates a fresh credential instead: - stacks.Manager.GenerateSecretForField: replacement value from the field's catalog generate spec via the deploy flow's generateValue (no logic copied); refuses data-keys (defense-in-depth), spec-less and non-secret fields. - backup.Manager.SetSecretGenerator seam (wired in main.go), consulted in RestoreFromRecoveryUnit AFTER the untouched fail-closed gate, for missing names NOT in DataKeyEnvVars. The generated value rides fullEnv into RecreateStackFromUnit → RedeployFromEnv → SaveAppConfig, so it persists encrypted in the guest app.yaml and round-trips on the next backup/restore (no second write path). reconcileRestoreSecrets stays pure and untouched. - WARNs now discriminate: "generated replacement for X (credential was reset)" vs "X unrecoverable and has no generator — app may fail to start". Values are never logged (asserted in test). - Residual case (documented, not pretended away): if a restored volume tar carries the OLD internal credential hash, the app may still fail auth until a manual in-DB reset — generation fully fixes only the fresh-init case. Companion red-proof: pre-fix behaviour (generation skipped) fails TestRestoreGeneratesMissingResettableSecret on the non-empty DB_PASSWORD assertion (verified, reverted). Data-key gate proven unreachable by generation in TestRestoreGenerationNeverReachesDataKeys. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6 --- controller/cmd/controller/main.go | 3 + controller/internal/backup/backup.go | 12 ++ .../backup/restore_secrets_gen_test.go | 134 ++++++++++++++++++ controller/internal/backup/restore_unit.go | 31 +++- controller/internal/stacks/deploy.go | 36 +++++ .../internal/stacks/deploy_secretgen_test.go | 89 ++++++++++++ 6 files changed, 303 insertions(+), 2 deletions(-) create mode 100644 controller/internal/backup/restore_secrets_gen_test.go create mode 100644 controller/internal/stacks/deploy_secretgen_test.go diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index dc33578..215cb69 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -226,6 +226,9 @@ func main() { backupMgr = backup.NewManager(cfg, sett, logger) backupMgr.SetStackProvider(stackProv) backupMgr.SetVersion(Version) + // O4: restore-from-unit generates a replacement for an unrecoverable RESETTABLE secret + // (data-keys stay fail-closed) so the app redeploys with a fresh credential, not a blank one. + backupMgr.SetSecretGenerator(stackMgr.GenerateSecretForField) } // --- Wire the data-migration engine (B1) + backup↔migration mutual exclusion (Change 3) --- diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index 638db26..59945e0 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -44,6 +44,12 @@ type Manager struct { // disconnected) can be unit-tested without Docker. Nil → the real DumpAppVolumesSafe. dumpVolumesSafe func(stackName string) error + // generateSecret (O4), if set, produces a replacement value for a RESETTABLE secret that could + // not be recovered during restore-from-unit (wired to stacks.Manager.GenerateSecretForField in + // main.go). Nil / ok=false → the secret stays absent and the restore proceeds with a loud WARN. + // NEVER consulted for data-keys — the fail-closed gate refuses those before generation runs. + generateSecret func(stackName, envVar string) (string, bool) + // migrationRunning, if set, reports whether a data migration is in progress. The scheduled // backup paths skip when it returns true (Change 3 — backup ↔ migration mutual exclusion), so a // nightly dump/Tier-2 can't race a migration copy/cleanup on the same drive. @@ -506,6 +512,12 @@ func (m *Manager) releaseRunning() { m.mu.Unlock() } +// SetSecretGenerator wires the O4 resettable-secret generator used by RestoreFromRecoveryUnit +// (init-only, same contract as SetStackProvider: call once during single-threaded startup). +func (m *Manager) SetSecretGenerator(fn func(stackName, envVar string) (string, bool)) { + m.generateSecret = fn +} + // SetStackProvider sets the stack data provider for app data discovery. // // M2: this MUST be called exactly once during single-threaded startup (main.go), diff --git a/controller/internal/backup/restore_secrets_gen_test.go b/controller/internal/backup/restore_secrets_gen_test.go new file mode 100644 index 0000000..d04c27b --- /dev/null +++ b/controller/internal/backup/restore_secrets_gen_test.go @@ -0,0 +1,134 @@ +package backup + +import ( + "bytes" + "log" + "path/filepath" + "strings" + "testing" +) + +// newSecretGenUnit lays out a recovery unit whose manifest names one resettable secret +// (DB_PASSWORD) and one data-key (SECRET_KEY) — the O4 test fixture. +func newSecretGenUnit(t *testing.T) (drive string) { + t.Helper() + drive = filepath.Join(t.TempDir(), "drive") + mustWrite(t, filepath.Join(RecoveryUnitComposePath(drive, "app"), "app.yaml"), + "deployed: true\nenv:\n SUBDOMAIN: trips\n") + man := &RecoveryManifest{SchemaVersion: 1, AppName: "app", ControllerVer: "v", + SecretEnvVars: []string{"DB_PASSWORD", "SECRET_KEY"}, DataKeyEnvVars: []string{"SECRET_KEY"}} + if err := writeManifest(RecoveryUnitManifestPath(drive, "app"), man); err != nil { + t.Fatal(err) + } + return drive +} + +// TestRestoreGeneratesMissingResettableSecret proves Scenario F: with the data-key recovered but +// DB_PASSWORD unrecoverable, the restore PROCEEDS and RecreateStackFromUnit receives a NON-EMPTY +// generated DB_PASSWORD (pre-O4 it was simply absent → compose deployed blank → exit 1). Also +// proves the generator is consulted for the resettable secret ONLY — never a data-key — and that +// the generated VALUE never reaches the logs. +// COMPANION red-proof: reverting to the pre-fix behaviour (no generation) fails the non-empty +// DB_PASSWORD assertion. +func TestRestoreGeneratesMissingResettableSecret(t *testing.T) { + const genValue = "generated-secret-value-do-not-log" + drive := newSecretGenUnit(t) + fake := &fakeRecoveryProvider{ + hdd: drive, + running: true, + secrets: map[string]string{"SECRET_KEY": "deadbeef"}, // DB_PASSWORD unrecoverable + } + var logBuf bytes.Buffer + m := &Manager{logger: log.New(&logBuf, "", 0), systemDataPath: filepath.Join(drive, "..", "sys"), + stackProvider: fake} + var genCalls []string + m.generateSecret = func(stackName, envVar string) (string, bool) { + genCalls = append(genCalls, envVar) + return genValue, true + } + + if err := m.RestoreFromRecoveryUnit("app"); err != nil { + t.Fatalf("restore must proceed for a missing RESETTABLE secret: %v", err) + } + if fake.gotEnv == nil { + t.Fatal("recreate was not called") + } + if fake.gotEnv["DB_PASSWORD"] != genValue { + t.Errorf("DB_PASSWORD = %q, want the generated replacement (pre-O4: absent → blank deploy)", fake.gotEnv["DB_PASSWORD"]) + } + if fake.gotEnv["SECRET_KEY"] != "deadbeef" { + t.Errorf("recovered data-key must pass through verbatim, got %q", fake.gotEnv["SECRET_KEY"]) + } + if len(genCalls) != 1 || genCalls[0] != "DB_PASSWORD" { + t.Errorf("generator consulted for %v, want exactly [DB_PASSWORD] (never data-keys)", genCalls) + } + logs := logBuf.String() + if !strings.Contains(logs, "generated replacement") || !strings.Contains(logs, "DB_PASSWORD") { + t.Errorf("WARN must name the reset credential; logs:\n%s", logs) + } + // Secrets safety: the generated VALUE must never be logged — names only. + if strings.Contains(logs, genValue) { + t.Errorf("SECRET LEAK: generated value found in logs:\n%s", logs) + } +} + +// TestRestoreProceedsWhenNoGenerator proves Scenario G: a missing resettable secret with NO +// generator (seam returns ok=false, or no seam wired) still proceeds — env var absent, loud WARN +// that the app may fail to start — and never invents a default. +func TestRestoreProceedsWhenNoGenerator(t *testing.T) { + run := func(t *testing.T, wire bool) { + drive := newSecretGenUnit(t) + fake := &fakeRecoveryProvider{ + hdd: drive, + running: true, + secrets: map[string]string{"SECRET_KEY": "deadbeef"}, + } + var logBuf bytes.Buffer + m := &Manager{logger: log.New(&logBuf, "", 0), systemDataPath: filepath.Join(drive, "..", "sys"), + stackProvider: fake} + if wire { + m.generateSecret = func(string, string) (string, bool) { return "", false } // no spec (Scenario G) + } + + if err := m.RestoreFromRecoveryUnit("app"); err != nil { + t.Fatalf("restore must still proceed: %v", err) + } + if _, present := fake.gotEnv["DB_PASSWORD"]; present { + t.Errorf("no generator → the secret must stay absent, not be invented: %v", fake.gotEnv) + } + logs := logBuf.String() + if !strings.Contains(logs, "no generator") || !strings.Contains(logs, "may fail to start") || !strings.Contains(logs, "DB_PASSWORD") { + t.Errorf("upgraded WARN must name the var and the may-fail consequence; logs:\n%s", logs) + } + } + t.Run("generator wired, field has no spec", func(t *testing.T) { run(t, true) }) + t.Run("no generator wired at all", func(t *testing.T) { run(t, false) }) +} + +// TestRestoreGenerationNeverReachesDataKeys proves the frozen gate is untouched by O4: a missing +// DATA-KEY still refuses fail-closed BEFORE any generation — the generator is never consulted and +// the app is never recreated, even with a generator eagerly offering values. +func TestRestoreGenerationNeverReachesDataKeys(t *testing.T) { + drive := newSecretGenUnit(t) + fake := &fakeRecoveryProvider{ + hdd: drive, + secrets: map[string]string{"DB_PASSWORD": "pw"}, // SECRET_KEY (data_key) missing + } + m := &Manager{logger: log.New(bytes.NewBuffer(nil), "", 0), systemDataPath: filepath.Join(drive, "..", "sys"), + stackProvider: fake} + var genCalls []string + m.generateSecret = func(_, envVar string) (string, bool) { + genCalls = append(genCalls, envVar) + return "eager-value", true + } + + if err := m.RestoreFromRecoveryUnit("app"); err == nil { + t.Fatal("missing data-key must still refuse fail-closed") + } + if len(genCalls) != 0 { + t.Errorf("generator consulted for %v — must be unreachable when the gate refuses", genCalls) + } + if fake.gotEnv != nil { + t.Errorf("recreate must not run on refusal: %v", fake.gotEnv) + } +} diff --git a/controller/internal/backup/restore_unit.go b/controller/internal/backup/restore_unit.go index 4381bef..4092a80 100644 --- a/controller/internal/backup/restore_unit.go +++ b/controller/internal/backup/restore_unit.go @@ -114,9 +114,36 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { m.logger.Printf("[ERROR] [backup] Restore REFUSED for %s: %v", stackName, err) return err } + // O4: a missing RESETTABLE secret used to redeploy blank (compose "Defaulting to a blank + // string" → exit 1). Generate a replacement via the deploy flow's generator instead — + // RecreateStackFromUnit persists fullEnv through SaveAppConfig, so the new value lands + // 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. if len(missing) > 0 { - m.logger.Printf("[WARN] [backup] Restore %s: %d resettable secret(s) unrecoverable %v — proceeding (may need a credential reset; no data-key affected)", - stackName, len(missing), missing) + dataKeySet := make(map[string]bool, len(manifest.DataKeyEnvVars)) + for _, dk := range manifest.DataKeyEnvVars { + dataKeySet[dk] = true + } + var generated, unresolved []string + for _, name := range missing { + if !dataKeySet[name] && m.generateSecret != nil { + if v, ok := m.generateSecret(stackName, name); ok && v != "" { + fullEnv[name] = v + generated = append(generated, name) + continue + } + } + 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)", + stackName, generated) + } + if len(unresolved) > 0 { + m.logger.Printf("[WARN] [backup] Restore %s: %d resettable secret(s) unrecoverable and have no generator %v — proceeding, but the app may fail to start until the credential is set manually", + stackName, len(unresolved), unresolved) + } } m.logger.Printf("[INFO] [backup] Restoring %s from recovery unit: images=%d, secrets recovered=%d/%d, data_keys=%d", stackName, len(manifest.ImagePins), len(manifest.SecretEnvVars)-len(missing), len(manifest.SecretEnvVars), len(manifest.DataKeyEnvVars)) diff --git a/controller/internal/stacks/deploy.go b/controller/internal/stacks/deploy.go index 88f15c4..af1a08d 100644 --- a/controller/internal/stacks/deploy.go +++ b/controller/internal/stacks/deploy.go @@ -827,6 +827,42 @@ func generateValue(spec string) (string, error) { } } +// GenerateSecretForField generates a replacement value for a stack's RESETTABLE secret deploy-field +// (O4: the restore-from-unit path uses this — via backup.SetSecretGenerator — when a resettable +// secret cannot be recovered from the guest's app.yaml, so the app redeploys with a fresh credential +// instead of a blank one that fails compose-up). +// +// Returns ok=false when the field is unknown, has no generator spec, or — deliberately — is a +// DATA-ENCRYPTING key: data-keys are NEVER generated (regenerating one would render stored data +// unreadable; the restore's fail-closed gate refuses before this point, this is defense-in-depth). +// The generated VALUE is never logged — names only. +func (m *Manager) GenerateSecretForField(stackName, envVar string) (string, bool) { + s, ok := m.GetStack(stackName) + if !ok { + return "", false + } + meta := LoadMetadata(filepath.Dir(s.ComposePath)) + for _, f := range meta.DeployFields { + if f.EnvVar != envVar { + continue + } + if f.DataKey { + m.logger.Printf("[WARN] [stacks] GenerateSecretForField(%s/%s): refusing — field is a data-encrypting key", stackName, envVar) + return "", false + } + if (f.Type != "secret" && f.Type != "password") || f.Generate == "" { + return "", false + } + value, err := generateValue(f.Generate) + if err != nil || value == "" { + m.logger.Printf("[ERROR] [stacks] GenerateSecretForField(%s/%s): generator %q failed: %v", stackName, envVar, f.Generate, err) + return "", false + } + return value, true + } + return "", false +} + // InjectMissingFields checks deployed stacks for new deploy_fields that are not // yet in app.yaml and auto-generates values for secret/domain fields. // Called after sync (for updated stacks) and on startup (for all deployed stacks). diff --git a/controller/internal/stacks/deploy_secretgen_test.go b/controller/internal/stacks/deploy_secretgen_test.go new file mode 100644 index 0000000..9850a6d --- /dev/null +++ b/controller/internal/stacks/deploy_secretgen_test.go @@ -0,0 +1,89 @@ +package stacks + +import ( + "io" + "log" + "os" + "path/filepath" + "regexp" + "testing" +) + +// newSecretGenManager builds a Manager with one stack whose .felhom.yml declares the O4 test +// fields: a generatable resettable secret, a data-key, and a spec-less secret. +func newSecretGenManager(t *testing.T) *Manager { + t.Helper() + stackDir := filepath.Join(t.TempDir(), "app") + if err := os.MkdirAll(stackDir, 0755); err != nil { + t.Fatal(err) + } + meta := `display_name: App +deploy_fields: + - env_var: DB_PASSWORD + type: secret + generate: "password:24" + - env_var: SECRET_KEY + type: secret + generate: "hex:32" + data_key: true + - env_var: ADMIN_TOKEN + type: secret + - env_var: SUBDOMAIN + type: subdomain + default: app +` + if err := os.WriteFile(filepath.Join(stackDir, ".felhom.yml"), []byte(meta), 0644); err != nil { + t.Fatal(err) + } + return &Manager{ + logger: log.New(io.Discard, "", 0), + stacks: map[string]*Stack{ + "app": {Name: "app", ComposePath: filepath.Join(stackDir, "docker-compose.yml")}, + }, + } +} + +// TestGenerateSecretForField covers the O4 generator seam's contract: spec-conformant values for +// resettable secrets, and REFUSAL for data-keys (frozen fail-closed territory), spec-less fields, +// non-secret fields, and unknown stacks/vars. +func TestGenerateSecretForField(t *testing.T) { + m := newSecretGenManager(t) + + t.Run("resettable secret with spec → spec-conformant value", func(t *testing.T) { + v, ok := m.GenerateSecretForField("app", "DB_PASSWORD") + if !ok { + t.Fatal("expected generation for DB_PASSWORD (generate: password:24)") + } + if len(v) != 24 || !regexp.MustCompile(`^[A-Za-z0-9]+$`).MatchString(v) { + t.Errorf("value does not conform to password:24 (len=%d)", len(v)) + } + // Distinct per call (crypto/rand-backed, not a constant). + if v2, _ := m.GenerateSecretForField("app", "DB_PASSWORD"); v2 == v { + t.Error("two generations returned the same value") + } + }) + + t.Run("data-key → REFUSED even with a generate spec", func(t *testing.T) { + if v, ok := m.GenerateSecretForField("app", "SECRET_KEY"); ok || v != "" { + t.Error("a data-encrypting key must NEVER be generated") + } + }) + + t.Run("no generate spec → refused", func(t *testing.T) { + if _, ok := m.GenerateSecretForField("app", "ADMIN_TOKEN"); ok { + t.Error("spec-less secret must not be generated (Scenario G: proceed-with-warn instead)") + } + }) + + t.Run("non-secret field / unknown var / unknown stack → refused", func(t *testing.T) { + if _, ok := m.GenerateSecretForField("app", "SUBDOMAIN"); ok { + t.Error("non-secret field must not be generated") + } + if _, ok := m.GenerateSecretForField("app", "NOPE"); ok { + t.Error("unknown env var must not be generated") + } + if _, ok := m.GenerateSecretForField("ghost", "DB_PASSWORD"); ok { + t.Error("unknown stack must not be generated") + } + }) +}