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) {