From cef9bf92a5027b9c1d3a27fa8eabae1206656029 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Wed, 22 Apr 2026 17:48:06 -0300 Subject: [PATCH] ssh-config: harden sameDirOrParent against symlinks + add edge tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The symlink test in this commit catches a real bug: sameDirOrParent used filepath.Abs for both sides of the "is the key inside the legacy dir?" check, but filepath.Abs doesn't resolve symlinks. A user whose ssh_key_path pointed into ConfigDir/ssh via a symlinked spelling (e.g. ConfigDir itself is a symlink, or the user maintains an alias tree) would have their key silently deleted by the legacy-dir scrub — the gate thought the key lived elsewhere because the two spellings didn't match lexically. Fix: resolvePathForComparison tries filepath.EvalSymlinks first, falls back to filepath.Abs when the path doesn't exist yet (new install, pre-first-Open). Both sides of the sameDirOrParent comparison now use this helper, so a symlinked key + canonical dir (or the reverse) lands in the same physical path before the Rel check. Tests added in this commit: internal/daemon/ssh_client_config_test.go TestSameDirOrParentHandlesSymlinks — symlinked-key + canonical-dir and the reverse are both reported "inside"; unrelated paths stay out. Skips if the filesystem doesn't support symlinks. internal/config/config_test.go TestLoadNormalizesAbsoluteSSHKeyPath — trailing slash, duplicate slashes, dot segments all collapse via filepath.Clean, so two spellings of the same path compare equal downstream. TestEnsureDefaultSSHKeyRejectsCorruptExistingFile — regression guard against a future "regenerate if invalid" patch that would silently nuke a real user key. TestResolveSSHKeyPathRejectsEmptySSHDirAndStateDir — pins the absolute-path guard that stops a bad layout from scribbling into cwd (this was the test that caught the stray internal/config/ssh/ a few commits back). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/config/config_test.go | 80 +++++++++++++++++++++++ internal/daemon/ssh_client_config.go | 27 ++++++-- internal/daemon/ssh_client_config_test.go | 53 +++++++++++++++ 3 files changed, 155 insertions(+), 5 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 66a0379..96ff7bf 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -70,6 +70,86 @@ func TestLoadSSHKeyPathExpandsHomeAnchored(t *testing.T) { } } +// TestLoadNormalizesAbsoluteSSHKeyPath pins filepath.Clean behaviour +// for configured paths: trailing slashes and duplicate slashes are +// flattened so downstream comparisons (e.g. sameDirOrParent) don't +// see two spellings for the same path. +func TestLoadNormalizesAbsoluteSSHKeyPath(t *testing.T) { + cases := []struct { + name string + raw string + want string + }{ + {"trailing slash collapsed", "/tmp/keys/id_ed25519/", "/tmp/keys/id_ed25519"}, + {"duplicate slashes collapsed", "/tmp//keys///id_ed25519", "/tmp/keys/id_ed25519"}, + {"dot segments resolved", "/tmp/keys/./id_ed25519", "/tmp/keys/id_ed25519"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + configDir := t.TempDir() + data := []byte("ssh_key_path = \"" + tc.raw + "\"\n") + if err := os.WriteFile(filepath.Join(configDir, "config.toml"), data, 0o644); err != nil { + t.Fatalf("write config.toml: %v", err) + } + cfg, err := Load(paths.Layout{ConfigDir: configDir, SSHDir: t.TempDir()}) + if err != nil { + t.Fatalf("Load %q: %v", tc.raw, err) + } + if cfg.SSHKeyPath != tc.want { + t.Fatalf("SSHKeyPath = %q, want %q", cfg.SSHKeyPath, tc.want) + } + }) + } +} + +// TestEnsureDefaultSSHKeyRejectsCorruptExistingFile pins the +// "don't silently overwrite" contract: if someone wrote garbage to +// the default key path (or the key was truncated mid-write by a +// previous crash), config.Load must surface the parse error instead +// of pretending the file is usable. The regression we care about is +// a future refactor that adds "regenerate if invalid" silently — +// that would nuke a real user key on every daemon Open. +func TestEnsureDefaultSSHKeyRejectsCorruptExistingFile(t *testing.T) { + sshDir := t.TempDir() + corruptKey := filepath.Join(sshDir, "id_ed25519") + if err := os.WriteFile(corruptKey, []byte("not a pem private key"), 0o600); err != nil { + t.Fatalf("write corrupt key: %v", err) + } + + _, err := Load(paths.Layout{ConfigDir: t.TempDir(), SSHDir: sshDir}) + if err == nil { + t.Fatal("Load: want error when existing key file is not a valid private key") + } + // The error should mention the parse failure, not "regenerated". + if strings.Contains(err.Error(), "regenerat") { + t.Fatalf("Load silently regenerated: %v", err) + } + // Original garbage must still be there — the invariant is "don't + // touch files you can't parse". + data, readErr := os.ReadFile(corruptKey) + if readErr != nil { + t.Fatalf("ReadFile: %v", readErr) + } + if string(data) != "not a pem private key" { + t.Fatalf("key content = %q, want the original garbage", string(data)) + } +} + +// TestResolveSSHKeyPathRejectsEmptySSHDirAndStateDir pins the +// guard in resolveSSHKeyPath: if a caller builds a layout without +// SSHDir and StateDir, they shouldn't get a key generated in cwd. +// The guard existed before (added after a test scribbled into +// internal/config/ssh/); this test prevents it from going away. +func TestResolveSSHKeyPathRejectsEmptySSHDirAndStateDir(t *testing.T) { + _, err := Load(paths.Layout{ConfigDir: t.TempDir()}) + if err == nil { + t.Fatal("Load: want error when neither SSHDir nor StateDir is set") + } + if !strings.Contains(err.Error(), "must be absolute") { + t.Fatalf("Load error = %v, want 'must be absolute' diagnostic", err) + } +} + func TestLoadRejectsInvalidSSHKeyPath(t *testing.T) { cases := []struct { name string diff --git a/internal/daemon/ssh_client_config.go b/internal/daemon/ssh_client_config.go index fc2a604..063e7e7 100644 --- a/internal/daemon/ssh_client_config.go +++ b/internal/daemon/ssh_client_config.go @@ -122,18 +122,22 @@ func cleanupLegacySSHConfigDir(layout paths.Layout, keyPath string) { _ = 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. +// sameDirOrParent reports whether dir contains path (or equals it) +// after resolving symlinks. Used to gate destructive cleanup against +// a configured key that lives inside the cleanup target — either +// directly or via a symlinked spelling of the same physical +// location. Lexical comparison alone would miss the symlink case +// and let the scrub delete a user key aliased through an symlinked +// directory. func sameDirOrParent(dir, path string) bool { if strings.TrimSpace(dir) == "" || strings.TrimSpace(path) == "" { return false } - absDir, err := filepath.Abs(dir) + absDir, err := resolvePathForComparison(dir) if err != nil { return false } - absPath, err := filepath.Abs(path) + absPath, err := resolvePathForComparison(path) if err != nil { return false } @@ -147,6 +151,19 @@ func sameDirOrParent(dir, path string) bool { return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) } +// resolvePathForComparison returns an absolute, symlink-resolved +// version of p. Falls back to filepath.Abs when EvalSymlinks errors +// — typically because p refers to a file or directory that doesn't +// exist yet, which is fine for comparison purposes: two non-existent +// paths compared lexically is the best we can do and matches the +// pre-symlink-aware behaviour. +func resolvePathForComparison(p string) (string, error) { + if resolved, err := filepath.EvalSymlinks(p); err == nil { + return resolved, nil + } + return filepath.Abs(p) +} + // 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 9a2c4cb..8c16569 100644 --- a/internal/daemon/ssh_client_config_test.go +++ b/internal/daemon/ssh_client_config_test.go @@ -9,6 +9,59 @@ import ( "banger/internal/paths" ) +// TestSameDirOrParentHandlesSymlinks guards against a drift where +// sameDirOrParent (the gate that protects a user key under the +// legacy dir from the cleanup scrub) compares lexical paths and +// misses symlink aliasing. +// +// Scenario: user configured ssh_key_path at a path that lands inside +// ConfigDir/ssh via a symlink (e.g. ConfigDir is itself symlinked, +// or the user maintains a symlink alias for their key tree). The +// gate must resolve both sides to the same physical location and +// refuse to scrub. +func TestSameDirOrParentHandlesSymlinks(t *testing.T) { + physical := t.TempDir() + realDir := filepath.Join(physical, "real-ssh") + if err := os.Mkdir(realDir, 0o700); err != nil { + t.Fatalf("Mkdir: %v", err) + } + realKey := filepath.Join(realDir, "id_ed25519") + if err := os.WriteFile(realKey, []byte("PRIVATE"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + // A symlink that aliases the whole real-ssh directory. The user + // configured ssh_key_path via this alias, but sameDirOrParent is + // called with the canonical (realDir) legacyDir path. + aliasDir := filepath.Join(physical, "alias-ssh") + if err := os.Symlink(realDir, aliasDir); err != nil { + t.Skipf("symlink unsupported on this filesystem: %v", err) + } + aliasKey := filepath.Join(aliasDir, "id_ed25519") + + if !sameDirOrParent(realDir, aliasKey) { + t.Fatalf("sameDirOrParent(%q, %q) = false; symlinked key was not recognised as inside the dir — cleanup would delete it", realDir, aliasKey) + } + + // Reverse direction: dir provided as a symlink, key as canonical. + if !sameDirOrParent(aliasDir, realKey) { + t.Fatalf("sameDirOrParent(%q, %q) = false; reverse symlink direction also missed", aliasDir, realKey) + } + + // Negative: a key in a completely unrelated directory must not + // be reported inside either spelling of the legacy dir. + outside := filepath.Join(t.TempDir(), "other", "id_ed25519") + if err := os.MkdirAll(filepath.Dir(outside), 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(outside, []byte("UNRELATED"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if sameDirOrParent(realDir, outside) { + t.Fatalf("sameDirOrParent(%q, %q) = true; unrelated dir incorrectly flagged as inside", realDir, outside) + } +} + // 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