refactor: harden ssh_config handling, mount path, and CLI UX from audit findings
Update PKGBUILD version / update-pkgver (push) Successful in 3s
Update PKGBUILD version / update-pkgver (push) Successful in 3s
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 <Host> 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.
This commit is contained in:
@@ -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 `<Host>` 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)`.
|
||||
Reference in New Issue
Block a user