diff --git a/controller/internal/stacks/deploy.go b/controller/internal/stacks/deploy.go index 74dc911..6bc64c0 100644 --- a/controller/internal/stacks/deploy.go +++ b/controller/internal/stacks/deploy.go @@ -291,15 +291,24 @@ func (m *Manager) DeployStack(req DeployRequest) (string, error) { } } - // Save app.yaml + // Save app.yaml. + // CTRL-T2-1: persist the env now, but mark the ON-DISK state Deployed:false + // until `docker compose up -d` actually succeeds (done in runComposeDeploy). + // A crash/power-loss during the image-pull window must NOT leave a + // ghost-deployed stack on disk (Deployed:true with no containers), which + // DeployStack would then refuse to redeploy. The IN-MEMORY Deployed flag is + // still set true below to preserve the "no stale Telepítés button during + // pull" UX; only the durable record waits for success. appCfg := &AppConfig{ - Deployed: true, + Deployed: true, // in-memory truth (see below); the disk write overrides to false DeployedAt: time.Now().UTC().Format(time.RFC3339), Env: env, LockedFields: lockedFields, } - if err := SaveAppConfig(stackDir, appCfg, m.encKey, SensitiveEnvVars(&meta)); err != nil { + diskCfg := *appCfg + diskCfg.Deployed = false // transitional: env saved, not yet marked deployed + if err := SaveAppConfig(stackDir, &diskCfg, m.encKey, SensitiveEnvVars(&meta)); err != nil { clearDeploying() return "", fmt.Errorf("saving app config: %w", err) } @@ -359,6 +368,25 @@ func (m *Manager) runComposeDeploy(name, stackDir string, env map[string]string, m.logger.Printf("[INFO] [stacks] Stack %s deployed successfully (took %.1fs)", name, time.Since(start).Seconds()) + // CTRL-T2-1: compose up -d succeeded — only NOW mark deployed on disk. + // (DeployStack wrote the env with Deployed:false; flip it true here so the + // durable record matches reality and survives a restart.) + meta := LoadMetadata(stackDir) + if err := SaveAppConfig(stackDir, appCfg, m.encKey, SensitiveEnvVars(&meta)); err != nil { + // Running but not durably recorded as deployed. Revert so the customer + // can cleanly redeploy rather than be stuck with a half-recorded stack. + m.logger.Printf("[ERROR] [stacks] Stack %s: compose succeeded but persisting deployed state failed: %v — reverting", name, err) + m.mu.Lock() + if s, ok := m.stacks[name]; ok { + s.Deployed = false + s.Deploying = false + s.DeployError = "deploy succeeded but state could not be saved: " + err.Error() + s.AppConfig = nil + } + m.mu.Unlock() + return + } + // Clear deploying flag m.mu.Lock() if s, ok := m.stacks[name]; ok { @@ -653,14 +681,17 @@ func SaveAppConfig(stackDir string, cfg *AppConfig, encKey []byte, sensitiveVars } for k, v := range cfg.Env { if encKey != nil && sensitiveSet[k] && !crypto.IsEncrypted(v) && v != "" { - if enc, err := crypto.Encrypt(encKey, v); err == nil { - saveCfg.Env[k] = enc - encryptedCount++ - continue - } else { - // H10 fix: log encryption failure — value will be saved in plaintext. - log.Printf("[WARN] [stacks] Failed to encrypt env var %q: %v — saving as plaintext", k, err) + enc, err := crypto.Encrypt(encKey, v) + if err != nil { + // H10 (fail-closed): NEVER persist a sensitive value in plaintext. + // Earlier code logged a WARN and fell through to a plaintext write; + // that leaked the secret to disk. Abort the save instead — callers + // already propagate this error and the deploy fails cleanly. + return fmt.Errorf("encrypting sensitive env var %q (refusing to persist plaintext): %w", k, err) } + saveCfg.Env[k] = enc + encryptedCount++ + continue } saveCfg.Env[k] = v } diff --git a/controller/internal/stacks/deploy_crashsafety_regression_test.go b/controller/internal/stacks/deploy_crashsafety_regression_test.go new file mode 100644 index 0000000..384f30c --- /dev/null +++ b/controller/internal/stacks/deploy_crashsafety_regression_test.go @@ -0,0 +1,101 @@ +package stacks + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Regression tests for the deploy-lifecycle slice (audit/2026-06-13): +// - H10: SaveAppConfig must FAIL CLOSED on an encryption error (never persist +// a sensitive value in plaintext). +// - CTRL-T2-1: the durable record must read as NOT deployed until a deploy +// actually completes, so a crash during the image-pull window leaves a +// redeployable stack rather than a ghost-deployed one. + +// TestSaveAppConfigFailsClosedOnEncryptError — H10. Originated as a failing +// reconcile test; now a permanent guard. A bad-length encKey makes crypto.Encrypt +// fail; SaveAppConfig must return an error and write NO plaintext secret. +func TestSaveAppConfigFailsClosedOnEncryptError(t *testing.T) { + dir := t.TempDir() + const secret = "supersecret-pw-do-not-leak" + cfg := &AppConfig{Deployed: true, Env: map[string]string{"DB_PASSWORD": secret}} + + badKey := []byte("short") // non-nil, invalid AES key length → crypto.Encrypt errors + + err := SaveAppConfig(dir, cfg, badKey, []string{"DB_PASSWORD"}) + if err == nil { + t.Fatalf("H10: SaveAppConfig returned nil on an encrypt failure — expected a fail-closed error") + } + + // No app.yaml should have been written; even if one was, it must not contain + // the plaintext secret. + if data, readErr := os.ReadFile(filepath.Join(dir, "app.yaml")); readErr == nil { + if strings.Contains(string(data), secret) { + t.Fatalf("H10: app.yaml contains the secret in plaintext after an encrypt failure:\n%s", data) + } + } +} + +// TestSaveAppConfigEncryptsWithGoodKey — companion: a valid key encrypts the +// sensitive var (it must not appear in plaintext) and the save succeeds. +func TestSaveAppConfigEncryptsWithGoodKey(t *testing.T) { + dir := t.TempDir() + const secret = "supersecret-pw" + key := make([]byte, 32) // valid AES-256 key + cfg := &AppConfig{Deployed: true, Env: map[string]string{"DB_PASSWORD": secret}} + + if err := SaveAppConfig(dir, cfg, key, []string{"DB_PASSWORD"}); err != nil { + t.Fatalf("SaveAppConfig with a valid key failed: %v", err) + } + data, err := os.ReadFile(filepath.Join(dir, "app.yaml")) + if err != nil { + t.Fatalf("reading app.yaml: %v", err) + } + if strings.Contains(string(data), secret) { + t.Fatalf("sensitive value was written in plaintext despite a valid key:\n%s", data) + } +} + +// TestTransitionalDeployStateReadsNotDeployed — CTRL-T2-1. DeployStack now +// writes the env with Deployed:false before launching compose, and flips it to +// Deployed:true only after `up -d` succeeds. This locks the durable-state +// semantics ScanStacks relies on: a stack persisted in the transitional state +// (the on-disk state left by a crash mid-pull) MUST read as not-deployed, so the +// customer can redeploy. The full crash-window is integration-level (needs a +// real compose); see the audit's manual repro. +func TestTransitionalDeployStateReadsNotDeployed(t *testing.T) { + key := make([]byte, 32) + sensitive := []string{"DB_PASSWORD"} + + // Transitional state DeployStack writes BEFORE compose succeeds. + transitional := t.TempDir() + if err := SaveAppConfig(transitional, &AppConfig{ + Deployed: false, DeployedAt: "2026-06-13T00:00:00Z", + Env: map[string]string{"DB_PASSWORD": "x"}, + }, key, sensitive); err != nil { + t.Fatalf("saving transitional config: %v", err) + } + got := LoadAppConfig(transitional) + if got == nil { + t.Fatal("LoadAppConfig returned nil for the transitional state") + } + // This is exactly the expression ScanStacks uses: deployed := cfg != nil && cfg.Deployed + if got.Deployed { + t.Fatalf("CTRL-T2-1: a deploy that did not complete reads as Deployed=true — ghost-deployed; "+ + "DeployStack must persist Deployed:false until compose succeeds") + } + + // Completed state (what runComposeDeploy writes on success) reads deployed. + completed := t.TempDir() + if err := SaveAppConfig(completed, &AppConfig{ + Deployed: true, DeployedAt: "2026-06-13T00:00:00Z", + Env: map[string]string{"DB_PASSWORD": "x"}, + }, key, sensitive); err != nil { + t.Fatalf("saving completed config: %v", err) + } + if c := LoadAppConfig(completed); c == nil || !c.Deployed { + t.Fatalf("CTRL-T2-1: a completed deploy must read as Deployed=true, got %+v", c) + } +}