fix(infra): resolve ssh client from trusted system dirs (#83289)

manan-tech · manan-tech · commit 97f40195c6be · 2026-06-14T22:43:18.000+05:30
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 #83289.
diff --git a/src/infra/resolve-system-bin.test.ts b/src/infra/resolve-system-bin.test.ts
@@ -202,6 +202,29 @@ describe("trusted directory list", () => {
     }
   });
 
+  it("includes the Windows built-in OpenSSH directory and resolves ssh.exe there", () => {
+    const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32");
+    resetWindowsInstallRootsForTests({
+      queryRegistryValue: (key, valueName) =>
+        key === "HKLM\\SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion" &&
+        valueName === "SystemRoot"
+          ? "D:\\Windows"
+          : null,
+    });
+    try {
+      const sshExe = path.win32.join("D:\\Windows", "System32", "OpenSSH", "ssh.exe");
+      resetResolveSystemBin((p: string) => p === sshExe);
+      expect(getTrustedDirsForTest()).toContain(
+        path.win32.join("D:\\Windows", "System32", "OpenSSH"),
+      );
+      expect(resolveSystemBin("ssh")).toBe(sshExe);
+    } finally {
+      platformSpy.mockRestore();
+      resetResolveSystemBin();
+      resetWindowsInstallRootsForTests();
+    }
+  });
+
   it("resolves machine-wide Chocolatey shims only with standard trust on Windows", () => {
     const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32");
     resetWindowsInstallRootsForTests({
diff --git a/src/infra/resolve-system-bin.ts b/src/infra/resolve-system-bin.ts
@@ -74,6 +74,9 @@ function buildWindowsTrustedDirs(): readonly string[] {
   const dirs: string[] = [];
   const { systemRoot } = getWindowsInstallRoots();
   dirs.push(path.win32.join(systemRoot, "System32"));
+  // Windows ships its built-in OpenSSH client in this System32 subdirectory;
+  // ssh.exe is not directly under System32, so it needs its own trusted entry.
+  dirs.push(path.win32.join(systemRoot, "System32", "OpenSSH"));
   dirs.push(path.win32.join(systemRoot, "SysWOW64"));
   dirs.push(path.win32.join(systemRoot, "System32", "WindowsPowerShell", "v1.0"));
 
diff --git a/src/infra/ssh-config.test.ts b/src/infra/ssh-config.test.ts
@@ -2,6 +2,7 @@
 import { spawn, type ChildProcess, type SpawnOptions } from "node:child_process";
 import { EventEmitter } from "node:events";
 import { beforeAll, describe, expect, it, vi } from "vitest";
+import { resolveSystemBin } from "./resolve-system-bin.js";
 
 type MockSpawnChild = EventEmitter & {
   stdout?: EventEmitter & { setEncoding?: (enc: string) => void };
@@ -45,7 +46,12 @@ vi.mock("node:child_process", async () => {
   );
 });
 
+vi.mock("./resolve-system-bin.js", () => ({
+  resolveSystemBin: vi.fn(() => "/usr/bin/ssh"),
+}));
+
 const spawnMock = vi.mocked(spawn);
+const resolveSystemBinMock = vi.mocked(resolveSystemBin);
 
 function requireSpawnArgs(index: number): string[] {
   const args = spawnMock.mock.calls[index]?.[1] as string[] | undefined;
@@ -98,6 +104,8 @@ describe("ssh-config", () => {
     expect(config?.port).toBe(2222);
     expect(config?.identityFiles).toEqual(["/tmp/id_ed25519"]);
     expect(requireSpawnArgs(0).slice(-2)).toEqual(["--", "me@alias"]);
+    expect(resolveSystemBinMock).toHaveBeenCalledWith("ssh", { trust: "strict" });
+    expect(spawnMock.mock.calls[0]?.[0]).toBe("/usr/bin/ssh");
   });
 
   it("adds non-default port and trimmed identity arguments", async () => {
@@ -139,6 +147,16 @@ describe("ssh-config", () => {
     await expect(resolveSshConfig({ user: "me", host: "bad-host", port: 22 })).resolves.toBeNull();
   });
 
+  it("returns null without spawning when no trusted ssh client is found", async () => {
+    resolveSystemBinMock.mockReturnValueOnce(null);
+    spawnMock.mockClear();
+
+    const config = await resolveSshConfig({ user: "me", host: "alias", port: 22 });
+
+    expect(config).toBeNull();
+    expect(spawnMock).not.toHaveBeenCalled();
+  });
+
   it("rejects oversized ssh -G output while preserving the parser contract", () => {
     expect(appendSshConfigOutput("user bob", "\nhostname example.com", 128)).toEqual({
       ok: true,
diff --git a/src/infra/ssh-config.ts b/src/infra/ssh-config.ts
@@ -1,6 +1,7 @@
 // Reads effective SSH target config from the local ssh client.
 import { spawn } from "node:child_process";
 import { parseStrictPositiveInteger } from "./parse-finite-number.js";
+import { resolveSystemBin } from "./resolve-system-bin.js";
 import type { SshParsedTarget } from "./ssh-tunnel.js";
 
 export const SSH_CONFIG_OUTPUT_MAX_CHARS = 64 * 1024;
@@ -76,7 +77,12 @@ export async function resolveSshConfig(
   target: SshParsedTarget,
   opts: { identity?: string; timeoutMs?: number } = {},
 ): Promise<SshResolvedConfig | null> {
-  const sshPath = "/usr/bin/ssh";
+  // Resolve ssh from trusted system directories only (never a PATH-controlled
+  // binary). Best-effort config read: skip enrichment when no system ssh exists.
+  const sshPath = resolveSystemBin("ssh", { trust: "strict" });
+  if (sshPath === null) {
+    return null;
+  }
   const args = ["-G"];
   if (target.port > 0 && target.port !== 22) {
     args.push("-p", String(target.port));
diff --git a/src/infra/ssh-tunnel.test.ts b/src/infra/ssh-tunnel.test.ts
@@ -1,6 +1,13 @@
-// Covers SSH target parsing.
-import { describe, expect, it } from "vitest";
-import { parseSshTarget } from "./ssh-tunnel.js";
+// Covers SSH target parsing and the trusted ssh client resolution guard.
+import { describe, expect, it, vi } from "vitest";
+import { resolveSystemBin } from "./resolve-system-bin.js";
+import { parseSshTarget, startSshPortForward } from "./ssh-tunnel.js";
+
+vi.mock("./resolve-system-bin.js", () => ({
+  resolveSystemBin: vi.fn(() => "/usr/bin/ssh"),
+}));
+
+const resolveSystemBinMock = vi.mocked(resolveSystemBin);
 
 describe("parseSshTarget", () => {
   it("parses user@host:port targets", () => {
@@ -30,3 +37,18 @@ describe("parseSshTarget", () => {
     expect(parseSshTarget("-oProxyCommand=echo")).toBeNull();
   });
 });
+
+describe("startSshPortForward", () => {
+  it("fails closed with a clear diagnostic when no trusted ssh client is found", async () => {
+    resolveSystemBinMock.mockReturnValueOnce(null);
+
+    await expect(
+      startSshPortForward({
+        target: "me@example.com",
+        localPortPreferred: 12345,
+        remotePort: 8080,
+        timeoutMs: 100,
+      }),
+    ).rejects.toThrow(/no trusted SSH client found/);
+  });
+});
diff --git a/src/infra/ssh-tunnel.ts b/src/infra/ssh-tunnel.ts
@@ -5,6 +5,7 @@ import { normalizeStringEntries } from "@openclaw/normalization-core/string-norm
 import { formatErrorMessage, isErrno } from "./errors.js";
 import { parseStrictPositiveInteger } from "./parse-finite-number.js";
 import { ensurePortAvailable } from "./ports.js";
+import { resolveSystemBin } from "./resolve-system-bin.js";
 
 export type SshParsedTarget = {
   user?: string;
@@ -117,6 +118,14 @@ export async function startSshPortForward(opts: {
     throw new Error(`invalid SSH target: ${opts.target}`);
   }
 
+  // Resolve ssh from trusted system directories only (never a PATH-controlled
+  // binary). Fail closed with a clear diagnostic when no system ssh client is
+  // present, e.g. a Windows host without the built-in OpenSSH feature installed.
+  const sshPath = resolveSystemBin("ssh", { trust: "strict" });
+  if (sshPath === null) {
+    throw new Error("no trusted SSH client found in system directories");
+  }
+
   let localPort = opts.localPortPreferred;
   try {
     await ensurePortAvailable(localPort);
@@ -157,7 +166,7 @@ export async function startSshPortForward(opts: {
   args.push("--", userHost);
 
   const stderr: string[] = [];
-  const child = spawn("/usr/bin/ssh", args, {
+  const child = spawn(sshPath, args, {
     stdio: ["ignore", "ignore", "pipe"],
   });
   child.stderr?.setEncoding("utf8");
