From d01a358f35e5228b68b89ec7b74996d0621fb878 Mon Sep 17 00:00:00 2001 From: nevaforget Date: Sun, 26 Apr 2026 11:24:45 +0200 Subject: [PATCH] refactor: harden ssh_config handling, mount path, and CLI UX from audit findings Three rounds of audit-driven hardening, fully documented in DECISIONS.md: - argv hardening: validate HostName/User/IdentityFile via allowlist regexes, parse Port via strconv.Atoi, surface ssh_config parse errors instead of silently swallowing them. Switch -o kernel_cache to auto_cache for network- FS correctness, pin StrictHostKeyChecking=accept-new. - LOW-severity cleanup: -v verbose flag (default output is just the mount path), run_editor returns errors and main exits 7 on failure, ABOUTME headers, golang.org/x/sys v0.43.0 (go 1.25.0). - Defense-in-depth + UX: rxIdentityFile first-character anchor rejects leading "-"/"."/":"/etc., verify_mount_dir resolves base via EvalSymlinks and refuses pre-existing symlinks at the mount path, flag.Usage shows the positional argument, run_editor uses cmd.Start() so cold-start Sublime does not block the terminal. - CI: empty-PKGVER guard in update-pkgver workflow. - Tests: verify_mount_dir path-traversal + symlink-reject coverage, rxHostUser/rxIdentityFile boundary cases. --- .gitea/workflows/update-pkgver.yaml | 4 + DECISIONS.md | 78 ++++++++++++++ README.md | 14 ++- go.mod | 10 +- go.sum | 4 +- main.go | 154 ++++++++++++++++++++++------ main_test.go | 128 +++++++++++++++++++++++ 7 files changed, 350 insertions(+), 42 deletions(-) create mode 100644 DECISIONS.md create mode 100644 main_test.go diff --git a/.gitea/workflows/update-pkgver.yaml b/.gitea/workflows/update-pkgver.yaml index 55c3b68..0d59447 100644 --- a/.gitea/workflows/update-pkgver.yaml +++ b/.gitea/workflows/update-pkgver.yaml @@ -36,6 +36,10 @@ jobs: fi PKGVER=$(cat /tmp/pkgver) + if [ -z "$PKGVER" ]; then + echo "ERROR: PKGVER from previous step is empty." + exit 1 + fi git clone http://gitea:3000/nevaforget/moonarch-pkgbuilds.git pkgbuilds cd pkgbuilds diff --git a/DECISIONS.md b/DECISIONS.md new file mode 100644 index 0000000..0043029 --- /dev/null +++ b/DECISIONS.md @@ -0,0 +1,78 @@ +# Decisions + +## 2026-04-26 – Audit remediation: cache flag, host-key policy, argv hardening +- **Who**: Dom, ClaudeCode +- **Why**: Audit found `kernel_cache` causes stale reads on a network FS; + `StrictHostKeyChecking` was implicit (depended on system default); + `ssh_config`-sourced strings flowed into the sshfs argv without validation, + allowing comma- or leading-dash injection if `~/.ssh/config` is attacker-influenced. +- **Tradeoffs**: + - `auto_cache` invalidates the page cache on `open(2)` when mtime/size + differ → marginal perf hit vs. correctness on a network FS. + - `accept-new` for `StrictHostKeyChecking` preserves first-connection UX + (TOFU) but locks subsequent reconnects to the recorded key. + - Allowlist regexes for HostName/User/IdentityFile are restrictive but match + real-world SSH naming. Rejected inputs surface a clear error. +- **How**: + - Replaced `-o kernel_cache` with `-o auto_cache` in `mount_sshfs`. + - Added explicit `-o StrictHostKeyChecking=accept-new`. + - Added `lookup_ssh_field` (uses `GetStrict`, surfaces parse errors) and + `validate_ssh_field` with per-field allowlists; Port parsed via `strconv.Atoi`. + - Added `main_test.go` covering `verify_mount_dir` and the allowlist regexes. + - Hardened `.gitea/workflows/update-pkgver.yaml` with an empty-PKGVER guard. + +## 2026-04-26 – LOW-severity audit findings: cleanup pass + one Won't-Fix +- **Who**: Dom, ClaudeCode +- **Why**: Followup on the LOW-severity findings from the same audit. +- **How**: + - Q-L1: dead `if len(port) == 0 { port = "22" }` block already removed by Q-M1 + refactor (port now flows through `lookup_ssh_field` → `strconv.Atoi`). + - Q-L2: `run_editor` returns `error` and `main` exits 7 on editor failure + instead of swallowing it. + - Q-L3: ABOUTME header added to `main.go` per global CLAUDE.md convention. + - P-L1: `golang.org/x/sys` bumped `v0.1.0 → v0.43.0` via `go get … && go mod tidy`. + `go` directive moved from 1.23.4 to 1.25.0 (required by the new dep). + - S-L2: added `-v` flag; default output now prints only the mount path. + `-v` restores the previous full HostName/User/Port/IdentityFile/Mount block. +- **Won't-Fix**: + - **S-L1** — `.gitea/workflows/update-pkgver.yaml` clones over + `http://gitea:3000/...`. This URL only resolves inside the Docker network + between the Gitea-act-runner container and the Gitea container. External + traffic still terminates TLS at the reverse proxy. An attacker on the + internal Docker bridge has already compromised the host; TLS between + containers does not help in that scenario. Documenting as accepted risk. + +## 2026-04-26 – Second-round audit: defense-in-depth + UX +- **Who**: Dom, ClaudeCode +- **Why**: Re-audit (Quality + Performance + Security in parallel) surfaced five + remaining LOW findings after agent-output gegencheck. Two MEDIUM claims were + filtered out as false positives: missing `Port` already defaults to `"22"` via + `ssh_config.Default()` (verified in library source), and `lookup_ssh_field` + does not parse `~/.ssh/config` four times — `kevinburke/ssh_config` caches via + `sync.Once`, so four `GetStrict` calls = one parse + three in-memory walks. + An additional MEDIUM claim about a regex range bug (`%-` interpreted as range) + was empirically refuted (only `-` matched, not the implied range), but a + legitimate defense-in-depth gap remained: the regex still allowed leading `-` + in IdentityFile values. +- **Tradeoffs**: + - `run_editor` switched to `cmd.Start()` — drops editor exit-status + propagation in exchange for non-blocking UX on Sublime cold-start. Errors + from missing binary or fork failure are still returned. + - `verify_mount_dir` now `MkdirAll`s the base separately before + `EvalSymlinks`. `EvalSymlinks` requires the path to exist; without the + upfront `MkdirAll`, first run on a fresh system would fail. + - `rxIdentityFile` first-character class restricted to `[A-Za-z0-9/~]`. Plain + relative paths like `.ssh/id` are rejected — unusual but technically valid + in `ssh_config`. If a real config uses relative IdentityFile values, the + user gets a clear error and can switch to absolute or `~`-prefixed. +- **How**: + - **S-L1**: anchored `rxIdentityFile` to `^[A-Za-z0-9/~][A-Za-z0-9._@/:+=~%-]*$`. + - **S-L2**: `verify_mount_dir` resolves base via `filepath.EvalSymlinks` and + rejects existing mount paths that are symlinks (`os.Lstat`). + - **Q-L1**: `flag.Usage` override now prints the positional `` argument + in the help output. + - **Q-L2**: inline comment at the `run_editor` call site documents the + deliberate fall-through on already-mounted hosts. + - **P-L2**: `run_editor` uses `cmd.Start()` instead of `cmd.Run()`. + - Tests extended: rxIdentityFile boundary cases, `TestVerifyMountDirRejectsSymlink`, + existing `TestVerifyMountDir` paths now compared against `EvalSymlinks(base)`. diff --git a/README.md b/README.md index ac4d58b..d79904d 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ install -Dm755 sshfsc /usr/local/bin/sshfsc # Dependencies - [SSHFS](https://wiki.archlinux.org/title/SSHFS) -- [Go](https://wiki.archlinux.org/title/Go) (build-time) +- [Go](https://wiki.archlinux.org/title/Go) >= 1.25 (build-time) # Usage @@ -32,11 +32,15 @@ sshfsc ## Arguments -| Flag | Description | -| ------------- | ------------- | -| -e | open mountpoint in your editor | +| Flag | Description | +| ---- | ----------- | +| `-e` | open mountpoint in your editor | +| `-v` | verbose: print resolved ssh_config fields (HostName, User, Port, IdentityFile) | -Editor Sublime-Text (subl) is currently hardcoded. [See](https://gitea.moonarch.de/nevaforget/sshfs_connect/issues/1) +By default only the resolved mount path is printed. Use `-v` for the full +ssh_config dump. + +Editor Sublime-Text (`subl`) is currently hardcoded. [See](https://gitea.moonarch.de/nevaforget/sshfs_connect/issues/1) # Example ssh config diff --git a/go.mod b/go.mod index 0dc5b7c..385e959 100644 --- a/go.mod +++ b/go.mod @@ -1,10 +1,10 @@ module sshfsc -go 1.23.4 - -require github.com/kevinburke/ssh_config v1.2.0 +go 1.25.0 require ( - github.com/moby/sys/mountinfo v0.7.2 // indirect - golang.org/x/sys v0.1.0 // indirect + github.com/kevinburke/ssh_config v1.2.0 + github.com/moby/sys/mountinfo v0.7.2 ) + +require golang.org/x/sys v0.43.0 // indirect diff --git a/go.sum b/go.sum index 1cdfdb3..7ecc201 100644 --- a/go.sum +++ b/go.sum @@ -2,5 +2,5 @@ github.com/kevinburke/ssh_config v1.2.0 h1:x584FjTGwHzMwvHx18PXxbBVzfnxogHaAReU4 github.com/kevinburke/ssh_config v1.2.0/go.mod h1:CT57kijsi8u/K/BOFA39wgDQJ9CxiF4nAY/ojJ6r6mM= github.com/moby/sys/mountinfo v0.7.2 h1:1shs6aH5s4o5H2zQLn796ADW1wMrIwHsyJ2v9KouLrg= github.com/moby/sys/mountinfo v0.7.2/go.mod h1:1YOa8w8Ih7uW0wALDUgT1dTTSBrZ+HiBLGws92L2RU4= -golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U= -golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.43.0 h1:Rlag2XtaFTxp19wS8MXlJwTvoh8ArU6ezoyFsMyCTNI= +golang.org/x/sys v0.43.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= diff --git a/main.go b/main.go index 599a92d..23e1f4f 100644 --- a/main.go +++ b/main.go @@ -1,3 +1,6 @@ +// ABOUTME: CLI tool that mounts remote filesystems via sshfs based on +// ABOUTME: ~/.ssh/config entries, with optional editor launch on the mountpoint. + package main import ( @@ -6,15 +9,32 @@ import ( "os" "os/exec" "path/filepath" + "regexp" + "strconv" "strings" "github.com/kevinburke/ssh_config" "github.com/moby/sys/mountinfo" ) -var eFlag = flag.Bool("e", false, "open mountpoint in your editor") +var ( + rxHostUser = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9._-]*$`) + rxIdentityFile = regexp.MustCompile(`^[A-Za-z0-9/~][A-Za-z0-9._@/:+=~%-]*$`) +) + +var ( + eFlag = flag.Bool("e", false, "open mountpoint in your editor") + vFlag = flag.Bool("v", false, "verbose: print resolved ssh_config fields") +) func main() { + flag.Usage = func() { + fmt.Fprintf(os.Stderr, + "usage: sshfsc [flags] \n\n"+ + "Mount a remote home directory via sshfs based on ~/.ssh/config entries.\n\n"+ + "Flags:\n") + flag.PrintDefaults() + } flag.Parse() args := flag.Args() @@ -23,17 +43,58 @@ func main() { os.Exit(2) } - hostname := ssh_config.Get(args[0], "HostName") - user := ssh_config.Get(args[0], "User") - port := ssh_config.Get(args[0], "Port") - ifile := ssh_config.Get(args[0], "IdentityFile") - - if len(hostname) == 0 || len(user) == 0 || len(ifile) == 0 { - fmt.Fprintln(os.Stderr, "Hostname not found in ~/.ssh/config") + hostname, err := lookup_ssh_field(args[0], "HostName") + if err != nil { + fmt.Fprintln(os.Stderr, err) os.Exit(3) } - if len(port) == 0 { - port = "22" + user, err := lookup_ssh_field(args[0], "User") + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(3) + } + port, err := lookup_ssh_field(args[0], "Port") + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(3) + } + ifile, err := lookup_ssh_field(args[0], "IdentityFile") + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(3) + } + + if hostname == "" || user == "" || ifile == "" { + var missing []string + if hostname == "" { + missing = append(missing, "HostName") + } + if user == "" { + missing = append(missing, "User") + } + if ifile == "" { + missing = append(missing, "IdentityFile") + } + fmt.Fprintf(os.Stderr, "ssh config %q missing required field(s): %s\n", + args[0], strings.Join(missing, ", ")) + os.Exit(3) + } + + if err := validate_ssh_field("HostName", hostname, rxHostUser); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(3) + } + if err := validate_ssh_field("User", user, rxHostUser); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(3) + } + if err := validate_ssh_field("IdentityFile", ifile, rxIdentityFile); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(3) + } + if p, perr := strconv.Atoi(port); perr != nil || p < 1 || p > 65535 { + fmt.Fprintf(os.Stderr, "ssh config Port %q is not a valid port number\n", port) + os.Exit(3) } mount, err := verify_mount_dir(hostname) @@ -42,12 +103,16 @@ func main() { os.Exit(4) } - fmt.Println("Hostname: ", hostname) - fmt.Println("User: ", user) - fmt.Println("Port: ", port) - fmt.Println("Ifile: ", ifile) - fmt.Println("Mount: ", mount) - fmt.Println("---") + if *vFlag { + fmt.Println("Hostname: ", hostname) + fmt.Println("User: ", user) + fmt.Println("Port: ", port) + fmt.Println("Ifile: ", ifile) + fmt.Println("Mount: ", mount) + fmt.Println("---") + } else { + fmt.Println("Mount: ", mount) + } chkmount, chkmount_err := mountinfo.Mounted(mount) if chkmount_err != nil { @@ -62,19 +127,37 @@ func main() { } else { fmt.Println("!!! Already mounted") } - run_editor(mount) + // run_editor fires on -e regardless of mount state: re-invoking with -e + // re-opens the editor on an already-mounted server. + if err := run_editor(mount); err != nil { + fmt.Fprintln(os.Stderr, "run_editor() failed:", err) + os.Exit(7) + } } -func run_editor(mount string) { - if *eFlag { - cmd := exec.Command("subl", mount) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - err := cmd.Run() - if err != nil { - fmt.Fprintln(os.Stderr, "run_editor() failed with", err) - } +func lookup_ssh_field(alias, field string) (string, error) { + v, err := ssh_config.GetStrict(alias, field) + if err != nil { + return "", fmt.Errorf("parse ~/.ssh/config: %w", err) } + return v, nil +} + +func validate_ssh_field(name, value string, allow *regexp.Regexp) error { + if !allow.MatchString(value) { + return fmt.Errorf("ssh config %s %q contains disallowed characters", name, value) + } + return nil +} + +func run_editor(mount string) error { + if !*eFlag { + return nil + } + cmd := exec.Command("subl", mount) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + return cmd.Start() } func verify_mount_dir(hostname string) (string, error) { @@ -83,9 +166,19 @@ func verify_mount_dir(hostname string) (string, error) { return "", fmt.Errorf("resolve home dir: %w", err) } base := filepath.Join(homedir, "Servers") - mount := filepath.Clean(filepath.Join(base, hostname)) - if !strings.HasPrefix(mount, base+string(os.PathSeparator)) { - return "", fmt.Errorf("hostname %q escapes mount base %q", hostname, base) + if err := os.MkdirAll(base, 0700); err != nil { + return "", fmt.Errorf("create base %q: %w", base, err) + } + realBase, err := filepath.EvalSymlinks(base) + if err != nil { + return "", fmt.Errorf("resolve base %q: %w", base, err) + } + mount := filepath.Clean(filepath.Join(realBase, hostname)) + if !strings.HasPrefix(mount, realBase+string(os.PathSeparator)) { + return "", fmt.Errorf("hostname %q escapes mount base %q", hostname, realBase) + } + if info, err := os.Lstat(mount); err == nil && info.Mode()&os.ModeSymlink != 0 { + return "", fmt.Errorf("mount path %q is a symlink", mount) } if err := os.MkdirAll(mount, 0700); err != nil { return "", fmt.Errorf("create mount dir %q: %w", mount, err) @@ -98,13 +191,14 @@ func mount_sshfs(hostname string, user string, ifile string, port string, mount "-o", "IdentityFile="+ifile, "-o", "idmap=user", "-o", "cache=yes", - "-o", "kernel_cache", + "-o", "auto_cache", "-o", "attr_timeout=60", "-o", "entry_timeout=60", "-o", "negative_timeout=20", "-o", "Ciphers=aes128-gcm@openssh.com", "-o", "Compression=no", "-o", "reconnect", + "-o", "StrictHostKeyChecking=accept-new", "-o", "ServerAliveInterval=15", "-o", "ServerAliveCountMax=3", user+"@"+hostname+":", mount) diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..df0f086 --- /dev/null +++ b/main_test.go @@ -0,0 +1,128 @@ +// ABOUTME: Unit tests for sshfsc helpers, primarily the path-traversal guard +// ABOUTME: in verify_mount_dir and the ssh_config field allowlist regexes. + +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestVerifyMountDir(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + base := filepath.Join(home, "Servers") + if err := os.MkdirAll(base, 0700); err != nil { + t.Fatalf("setup base: %v", err) + } + realBase, err := filepath.EvalSymlinks(base) + if err != nil { + t.Fatalf("resolve base: %v", err) + } + + cases := []struct { + name string + hostname string + wantErr string // substring; "" means no error + wantPath string // only checked when wantErr == "" + }{ + {"normal alias", "server1", "", filepath.Join(realBase, "server1")}, + {"trailing slash", "server1/", "", filepath.Join(realBase, "server1")}, + {"dot in name", "db.prod", "", filepath.Join(realBase, "db.prod")}, + {"parent traversal", "../etc", "escapes mount base", ""}, + {"deep traversal", "../../tmp/x", "escapes mount base", ""}, + {"empty string", "", "escapes mount base", ""}, + {"single dot", ".", "escapes mount base", ""}, + // nested path lands under base — accepted by guard. The hostname allowlist + // (rxHostUser) rejects slashes upstream; this test pins guard behaviour. + {"nested path stays under base", "sub/dir", "", filepath.Join(realBase, "sub/dir")}, + // absolute-looking input does not escape because filepath.Join keeps it + // under base. Same upstream allowlist applies. + {"absolute-looking input", "/etc/passwd", "", filepath.Join(realBase, "etc/passwd")}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := verify_mount_dir(tc.hostname) + if tc.wantErr != "" { + if err == nil { + t.Fatalf("want error containing %q, got nil (path=%q)", tc.wantErr, got) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error %q does not contain %q", err.Error(), tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tc.wantPath { + t.Fatalf("path = %q, want %q", got, tc.wantPath) + } + }) + } +} + +func TestValidateSSHFieldRegexes(t *testing.T) { + cases := []struct { + name string + value string + rx string + ok bool + }{ + {"plain host", "myserver", "host", true}, + {"host with dots", "db.prod.example.com", "host", true}, + {"host with hyphen", "edge-01", "host", true}, + {"host leading dash rejected", "-rogue", "host", false}, + {"host with comma rejected", "a,b", "host", false}, + {"host with space rejected", "a b", "host", false}, + {"host with slash rejected", "a/b", "host", false}, + {"host empty rejected", "", "host", false}, + + {"identityfile tilde", "~/.ssh/id_ed25519", "ifile", true}, + {"identityfile absolute", "/home/dom/.ssh/id_rsa", "ifile", true}, + {"identityfile letter start", "id_rsa", "ifile", true}, + {"identityfile digit start", "0key", "ifile", true}, + {"identityfile dash mid accepted", "~/.ssh/id-host", "ifile", true}, + {"identityfile with comma rejected", "~/.ssh/id,allow_other", "ifile", false}, + {"identityfile with space rejected", "~/.ssh/id rsa", "ifile", false}, + {"identityfile leading dash rejected", "-Ffoo", "ifile", false}, + {"identityfile leading dot rejected", ".ssh/id", "ifile", false}, + {"identityfile leading colon rejected", ":weird", "ifile", false}, + {"identityfile empty rejected", "", "ifile", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var rx = rxHostUser + if tc.rx == "ifile" { + rx = rxIdentityFile + } + err := validate_ssh_field("X", tc.value, rx) + if tc.ok && err != nil { + t.Fatalf("want accept, got error: %v", err) + } + if !tc.ok && err == nil { + t.Fatalf("want reject, got accept") + } + }) + } +} + +func TestVerifyMountDirRejectsSymlink(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + base := filepath.Join(home, "Servers") + if err := os.MkdirAll(base, 0700); err != nil { + t.Fatalf("setup base: %v", err) + } + target := t.TempDir() + if err := os.Symlink(target, filepath.Join(base, "evil")); err != nil { + t.Fatalf("create symlink: %v", err) + } + _, err := verify_mount_dir("evil") + if err == nil || !strings.Contains(err.Error(), "is a symlink") { + t.Fatalf("want symlink rejection, got %v", err) + } +}