fix(infra): resolve ssh client from trusted system dirs (#83289)#93030
fix(infra): resolve ssh client from trusted system dirs (#83289)#93030manan-tech wants to merge 1 commit into
Conversation
resolveSshConfig and startSshPortForward spawned a hard-coded /usr/bin/ssh,
so SSH config discovery and gateway tunneling failed wherever the system ssh
client lives elsewhere: Windows (built-in OpenSSH under System32\OpenSSH) and
NixOS (/run/current-system/sw/bin).
Route both helpers through the existing resolveSystemBin("ssh", { trust:
"strict" }) resolver, and add the Windows built-in OpenSSH directory to the
resolver's trusted Windows locations (it had System32 but not the
System32\OpenSSH subdirectory where ssh.exe actually ships). Fail closed with
a clear diagnostic when no system ssh client is present.
Closes openclaw#83289.
openclaw-barnacle
Bot
added
size: S
proof: supplied
|
Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 10:42 PM ET / 02:42 UTC. Summary PR surface: Source +18, Tests +63. Total +81 across 6 files. Reproducibility: yes. Source inspection of current main and Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Rebase the trusted-resolver fix onto current main, preserve the existing SSH tunnel hardening, add redacted patched OpenClaw proof on Windows or NixOS, and get maintainer acceptance for the strict Windows trust-boundary change. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection of current main and Is this the best way to solve the issue? Yes for the code direction: reusing AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4ac5cf863673. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +18, Tests +63. Total +81 across 6 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Added live Windows Server 2022 verification to the PR description. On a real Windows host:
This is the after-fix behavior on a host where @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
This pull request has been automatically marked as stale due to inactivity. |
1 participant
Summary
resolveSshConfigandstartSshPortForwardspawned a hard-coded/usr/bin/ssh. On hosts where the system ssh client lives elsewhere, both SSH config discovery and gateway tunneling fail, notably:%SystemRoot%\System32\OpenSSH\ssh.exe, never/usr/bin/ssh./run/current-system/sw/bin, not/usr/bin.This routes both helpers through the repo's existing trusted-binary resolver,
resolveSystemBin("ssh", { trust: "strict" }), and adds the Windows built-in OpenSSH directory to the resolver's trusted Windows locations (it already trustedSystem32but not theSystem32\OpenSSHsubdirectory wheressh.exeactually ships).Closes #83289.
Why this shape
resolveSystemBin(the same resolver used foropenssl/ffmpeg) instead of adding a new path-resolution helper.sshTunnelErrorpath) when no system ssh client is present; the best-effort config reader returnsnull, consistent with its other failure branches.resolveSystemBinonly returns paths that exist and are executable,startSshPortForwardcan no longer reachspawnwith a non-existent binary.Trust-boundary note
The only new trusted location is
%SystemRoot%\System32\OpenSSH, the OS-managed home of the Windows built-in OpenSSH client. It is added understricttrust (OS-managed directories only); no user-writable or PATH-derived directory is trusted, preserving the trusted-subprocess boundary for a security-sensitive binary. Git-for-Windows / winget ssh under Program Files is intentionally not covered bystrict.Live Windows verification
Reproduced on a real Windows Server 2022 Datacenter host (built-in OpenSSH client,
OpenSSH_for_Windows_9.5p2). The old hard-coded path is absent, and the real client resolves exactly to the directory this PR adds to strict trust:So on a real Windows host the pre-fix code spawns a path that does not exist, while
resolveSystemBin("ssh", { trust: "strict" })(with theSystem32\OpenSSHentry this PR adds) resolves the genuine OS client.Real behavior proof
/usr/bin/ssh.node scripts/run-vitest.mjs src/infra/resolve-system-bin.test.ts src/infra/ssh-config.test.ts src/infra/ssh-tunnel.test.ts src/commands/gateway-status.test.ts;pnpm tsgo:coreandpnpm tsgo:core:test;node scripts/run-oxlint.mjsandoxfmt --checkon the changed files.Test-Pathof the old/new paths,Get-Command ssh,ssh -V(output above).resolveSystemBin("ssh")resolves...\System32\OpenSSH\ssh.exeunder a mocked Windows environment; helper tests assert the resolved path is what gets spawned and that the tunnel fails closed when none is found. Reverting only the resolver change makes the Windows test fail withexpected [...] to include 'System32\OpenSSH'. On the live Windows host,Get-Command sshresolves toC:\Windows\System32\OpenSSH\ssh.exewhile/usr/bin/sshdoes not exist.resolveSystemBin("ssh", { trust: "strict" })returns/usr/bin/ssh, identical to the removed literal (no Unix regression); on real Windows Server 2022 it resolvesC:\Windows\System32\OpenSSH\ssh.exe.Sysnative), which the resolver does not special-case for any binary.