ssh-config: harden sameDirOrParent against symlinks + add edge tests
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) <noreply@anthropic.com>
This commit is contained in:
parent
b2756f5e7e
commit
cef9bf92a5
3 changed files with 155 additions and 5 deletions
|
|
@ -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 <bangerSSHConfigPath>` line
|
||||
// to ~/.ssh/config inside a banger-owned marker block. Idempotent:
|
||||
// running it twice leaves a single block. Also strips any legacy
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue