ssh-config: narrow the legacy-dir cleanup so it can't delete a user key

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) <noreply@anthropic.com>
This commit is contained in:
Thales Maciel 2026-04-22 16:31:07 -03:00
parent fba30f26d4
commit b1fbf695ca
No known key found for this signature in database
GPG key ID: 33112E6833C34679
4 changed files with 192 additions and 9 deletions

View file

@ -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 <bangerSSHConfigPath>` line
// to ~/.ssh/config inside a banger-owned marker block. Idempotent:
// running it twice leaves a single block. Also strips any legacy