From b1fbf695ca728ec99dc88d7f1579b39c47ed6469 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Wed, 22 Apr 2026 16:31:07 -0300 Subject: [PATCH] ssh-config: narrow the legacy-dir cleanup so it can't delete a user key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: syncVMSSHClientConfig did os.RemoveAll on $ConfigDir/ssh every daemon Open. The intent was to migrate off the pre-opt-in layout, where banger used to write $ConfigDir/ssh/ssh_config. But a user who sets ssh_key_path = "~/.config/banger/ssh/id_ed25519" in config.toml has their key live exactly in that dir — and the scrub deletes it along with every other file in the tree. This is the same class of bug that cost the default key until ebe6517 moved it to StateDir, but that fix was scoped to the default path. A configured ssh_key_path pointed under the legacy dir still dies. Fix: replace os.RemoveAll with a narrow two-step cleanup: 1. Skip the cleanup entirely when the configured ssh_key_path resolves under the legacy dir. A user who pointed banger at a key there must keep the enclosing directory. 2. Otherwise, os.Remove the specific legacy file ($ConfigDir/ssh/ ssh_config) and then os.Remove the directory. The second os.Remove fails with ENOTEMPTY if the dir still holds anything (e.g. a user-managed sibling file we don't own). Both errors are swallowed — this is best-effort migration, not a hard failure. Tests pin all three paths: user key under legacy dir survives, legacy dir empties and is removed when the user moved on, and a user-managed sibling file in the legacy dir is preserved. Also fix stale doc claims in README.md and AGENTS.md — both still pointed at the old ~/.config/banger/ssh/id_ed25519 default, which moved to ~/.local/state/banger/ssh/id_ed25519 in ebe6517. Co-Authored-By: Claude Opus 4.7 (1M context) --- AGENTS.md | 2 +- README.md | 2 +- internal/daemon/ssh_client_config.go | 56 +++++++-- internal/daemon/ssh_client_config_test.go | 141 ++++++++++++++++++++++ 4 files changed, 192 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a6fc770..5e15ebf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -37,7 +37,7 @@ Always run `make build` before commit. - Config lives at `~/.config/banger/config.toml`. - Firecracker comes from `PATH` by default, or `firecracker_bin`. -- SSH uses `ssh_key_path` or an auto-managed default key at `~/.config/banger/ssh/id_ed25519`. +- SSH uses `ssh_key_path` or an auto-managed default key at `~/.local/state/banger/ssh/id_ed25519`. ## Coding Style diff --git a/README.md b/README.md index 03891cb..1140ec4 100644 --- a/README.md +++ b/README.md @@ -152,7 +152,7 @@ Most commonly set: (default `debian-bookworm`, auto-pulled from the catalog if not local). - `ssh_key_path` — host SSH key. If unset, banger creates - `~/.config/banger/ssh/id_ed25519`. + `~/.local/state/banger/ssh/id_ed25519`. - `firecracker_bin` — override the auto-resolved `PATH` lookup. Full key list in `internal/config/config.go`. diff --git a/internal/daemon/ssh_client_config.go b/internal/daemon/ssh_client_config.go index 4deb281..fc2a604 100644 --- a/internal/daemon/ssh_client_config.go +++ b/internal/daemon/ssh_client_config.go @@ -78,9 +78,11 @@ func (d *Daemon) ensureVMSSHClientConfig() { // // The file lives in the banger config dir so users who manage their // SSH config declaratively can decide how (or whether) to pull it in. -// We also keep a tiny migration step here: the pre-opt-in daemon -// wrote a sibling file at $ConfigDir/ssh/ssh_config; remove it and -// its dir if present. +// A narrow migration step also runs here: the pre-opt-in daemon +// wrote a sibling file at $ConfigDir/ssh/ssh_config. Remove only +// that specific legacy file, then remove the enclosing directory +// only if it's empty — never os.RemoveAll, because the user may +// have pointed ssh_key_path at a key under that directory. func syncVMSSHClientConfig(layout paths.Layout, keyPath string) error { keyPath = strings.TrimSpace(keyPath) if keyPath == "" { @@ -98,13 +100,53 @@ func syncVMSSHClientConfig(layout paths.Layout, keyPath string) error { return err } - legacyDir := filepath.Join(layout.ConfigDir, "ssh") - if _, err := os.Stat(legacyDir); err == nil { - _ = os.RemoveAll(legacyDir) - } + cleanupLegacySSHConfigDir(layout, keyPath) return nil } +// cleanupLegacySSHConfigDir removes the pre-opt-in sibling file at +// $ConfigDir/ssh/ssh_config and, if the directory is then empty, the +// directory itself. Skips the whole operation when ssh_key_path +// resolves under that directory — users who explicitly configured a +// key there must not have the enclosing dir yanked out from under +// them. All errors are swallowed: this is best-effort migration, not +// a hard failure mode. +func cleanupLegacySSHConfigDir(layout paths.Layout, keyPath string) { + legacyDir := filepath.Join(layout.ConfigDir, "ssh") + if sameDirOrParent(legacyDir, keyPath) { + return + } + _ = os.Remove(filepath.Join(legacyDir, "ssh_config")) + // Remove the dir only if it's now empty. os.Remove returns + // ENOTEMPTY when it isn't, which is the signal we want. + _ = os.Remove(legacyDir) +} + +// sameDirOrParent reports whether dir contains path (or equals it +// after resolving relatives). Used to gate destructive cleanup +// against a configured key that lives inside the cleanup target. +func sameDirOrParent(dir, path string) bool { + if strings.TrimSpace(dir) == "" || strings.TrimSpace(path) == "" { + return false + } + absDir, err := filepath.Abs(dir) + if err != nil { + return false + } + absPath, err := filepath.Abs(path) + if err != nil { + return false + } + rel, err := filepath.Rel(absDir, absPath) + if err != nil { + return false + } + // filepath.Rel returns "../..." when absPath is outside absDir. + // A path inside (or equal to) the dir starts with "." or a + // non-".." prefix. + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + // InstallUserSSHInclude adds an `Include ` line // to ~/.ssh/config inside a banger-owned marker block. Idempotent: // running it twice leaves a single block. Also strips any legacy diff --git a/internal/daemon/ssh_client_config_test.go b/internal/daemon/ssh_client_config_test.go index d2a594f..9a2c4cb 100644 --- a/internal/daemon/ssh_client_config_test.go +++ b/internal/daemon/ssh_client_config_test.go @@ -9,6 +9,147 @@ import ( "banger/internal/paths" ) +// A user-configured ssh_key_path that happens to live under the +// legacy $ConfigDir/ssh directory must survive the pre-opt-in +// migration cleanup. The old code did os.RemoveAll on the whole +// directory, which nuked the key. Pin the narrower behavior so a +// future refactor can't re-broaden the scrub. +func TestSyncVMSSHClientConfigPreservesUserKeyInLegacyDir(t *testing.T) { + homeDir := t.TempDir() + t.Setenv("HOME", homeDir) + + configDir := filepath.Join(homeDir, ".config", "banger") + legacyDir := filepath.Join(configDir, "ssh") + if err := os.MkdirAll(legacyDir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + userKey := filepath.Join(legacyDir, "id_ed25519") + if err := os.WriteFile(userKey, []byte("PRIVATE"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + // A stale ssh_config under the same dir from pre-opt-in era. + legacyConfig := filepath.Join(legacyDir, "ssh_config") + if err := os.WriteFile(legacyConfig, []byte("stale"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + layout := paths.Layout{ + ConfigDir: configDir, + KnownHostsPath: filepath.Join(homeDir, ".local", "state", "banger", "ssh", "known_hosts"), + } + if err := syncVMSSHClientConfig(layout, userKey); err != nil { + t.Fatalf("syncVMSSHClientConfig: %v", err) + } + + // The configured key must survive. + if _, err := os.Stat(userKey); err != nil { + t.Fatalf("user-configured key disappeared: %v", err) + } + // Enclosing directory must also survive (it contains the key). + if _, err := os.Stat(legacyDir); err != nil { + t.Fatalf("legacy dir removed despite containing the configured key: %v", err) + } + // The stale legacy ssh_config file can still be gone in this + // case — the user's key isn't ssh_config, so cleaning up the + // sibling file is fine. We don't assert either way, since the + // gate is "don't delete the user's key" not "always delete the + // sibling file." +} + +// With ssh_key_path configured outside ConfigDir/ssh, the legacy +// migration step should scrub the old sibling file and then the +// (now-empty) directory — no os.RemoveAll on anything still in use. +func TestSyncVMSSHClientConfigNarrowsCleanupToLegacyFile(t *testing.T) { + homeDir := t.TempDir() + t.Setenv("HOME", homeDir) + + configDir := filepath.Join(homeDir, ".config", "banger") + legacyDir := filepath.Join(configDir, "ssh") + if err := os.MkdirAll(legacyDir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + // Simulate the pre-opt-in leftover: just the ssh_config file. + legacyConfig := filepath.Join(legacyDir, "ssh_config") + if err := os.WriteFile(legacyConfig, []byte("stale"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + // ssh_key_path lives in the state dir (the new default location). + stateDir := filepath.Join(homeDir, ".local", "state", "banger", "ssh") + if err := os.MkdirAll(stateDir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + userKey := filepath.Join(stateDir, "id_ed25519") + if err := os.WriteFile(userKey, []byte("PRIVATE"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + layout := paths.Layout{ + ConfigDir: configDir, + KnownHostsPath: filepath.Join(homeDir, ".local", "state", "banger", "ssh", "known_hosts"), + } + if err := syncVMSSHClientConfig(layout, userKey); err != nil { + t.Fatalf("syncVMSSHClientConfig: %v", err) + } + + // Legacy ssh_config file: gone. + if _, err := os.Stat(legacyConfig); !os.IsNotExist(err) { + t.Fatalf("legacy ssh_config survived cleanup: %v", err) + } + // Legacy dir: gone, since it was empty after the file removal. + if _, err := os.Stat(legacyDir); !os.IsNotExist(err) { + t.Fatalf("legacy dir survived cleanup when empty: %v", err) + } + // User's key: untouched. + if _, err := os.Stat(userKey); err != nil { + t.Fatalf("user key disappeared: %v", err) + } +} + +// If the legacy dir contains UNEXPECTED files (not ssh_config, not +// the configured key), leave the dir alone. os.Remove on a non- +// empty dir errors with ENOTEMPTY, which we swallow. Regression +// guard so the cleanup can never escalate to recursive deletion. +func TestSyncVMSSHClientConfigLeavesUnexpectedLegacyContents(t *testing.T) { + homeDir := t.TempDir() + t.Setenv("HOME", homeDir) + + configDir := filepath.Join(homeDir, ".config", "banger") + legacyDir := filepath.Join(configDir, "ssh") + if err := os.MkdirAll(legacyDir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + // A user-managed file we have no business removing. + userFile := filepath.Join(legacyDir, "my-other-thing") + if err := os.WriteFile(userFile, []byte("mine"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + layout := paths.Layout{ + ConfigDir: configDir, + KnownHostsPath: filepath.Join(homeDir, ".local", "state", "banger", "ssh", "known_hosts"), + } + // ssh_key_path lives elsewhere; cleanup would otherwise proceed. + stateKey := filepath.Join(homeDir, ".local", "state", "banger", "ssh", "id_ed25519") + if err := os.MkdirAll(filepath.Dir(stateKey), 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(stateKey, []byte("PRIVATE"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if err := syncVMSSHClientConfig(layout, stateKey); err != nil { + t.Fatalf("syncVMSSHClientConfig: %v", err) + } + + if _, err := os.Stat(userFile); err != nil { + t.Fatalf("user-managed legacy-dir file disappeared: %v", err) + } + if _, err := os.Stat(legacyDir); err != nil { + t.Fatalf("legacy dir vanished despite non-empty contents: %v", err) + } +} + // Under the opt-in contract the daemon writes its own ssh_config file // and never touches ~/.ssh/config on its own. func TestSyncVMSSHClientConfigWritesBangerFileOnly(t *testing.T) {