From 137704033d5530dfeed34d0c99cc36ecb9a8bf53 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Tue, 13 Oct 2026 15:17:15 -0400 Subject: [PATCH 1/2] Add a direct SSH mode without ControlMaster Win32-OpenSSH cannot share a connection. A direct session runs each command over its own connection with BatchMode, keeps one ssh -D master per host, forwards ports in-process through its SOCKS port, and holds the installer's -R relay in an ssh of its own. Development builds can force it with DEV_FAST_REVIEW_SSH_DIRECT=1; nothing selects it yet. --- .../electron-main/remote/reviewRemoteHost.ts | 80 ++++++++++--- .../electron-main/remote/reviewRemoteHosts.ts | 28 +++-- .../remote/reviewSocksForward.test.ts | 88 ++++++++++++++ .../remote/reviewSocksForward.ts | 111 ++++++++++++++++++ .../remote/reviewSshCommand.test.ts | 21 ++++ .../electron-main/remote/reviewSshCommand.ts | 69 +++++++---- .../remote/reviewSshDirect.containers.test.ts | 70 +++++++++++ .../review/electron-main/reviewDesktopHost.ts | 5 +- .../scripts/e2e/journeys/remote-host.mjs | 3 +- .../scripts/e2e/journeys/remote-install.mjs | 3 +- 10 files changed, 426 insertions(+), 52 deletions(-) create mode 100644 apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSocksForward.test.ts create mode 100644 apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSocksForward.ts create mode 100644 apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshDirect.containers.test.ts diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts index 13242bfb2..5023b3eac 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts @@ -14,6 +14,7 @@ import type { ReviewRemoteInstallConsent } from "./reviewRemoteInstallConsent.js import type { ReviewRemoteInstallInput, ReviewRemoteInstallResult } from "./reviewRemoteInstaller.js"; import { reviewRemoteVersionDir } from "./reviewRemoteInstallScript.js"; import { judgeRemote, missingInstallTools, probeRemote, type ReviewRemoteProbe, type ReviewRemoteTarget } from "./reviewRemoteProbe.js"; +import { socksAnswers, socksForward, type ReviewSocksForward } from "./reviewSocksForward.js"; import { sshCancelForwardArgs, sshCheckArgs, @@ -301,6 +302,8 @@ export class ReviewRemoteHost { private master: SshChildProcess | undefined; private readonly closing = new Map>(); private forwarded: { local: number; remote: number } | undefined; + private socksPort: number | undefined; + private readonly socks = new Map(); private language: { local: number; remote: number; connectionToken: string; commit: string } | undefined; private pendingAttaches = 0; private cancelPending: (() => void) | undefined; @@ -331,6 +334,10 @@ export class ReviewRemoteHost { this.reported = { alias: this.alias }; } + private get direct(): boolean { + return !this.options.session.controlPath; + } + get state(): ReviewGatewayHost { return this.reported; } @@ -390,9 +397,9 @@ export class ReviewRemoteHost { this.reattaching = true; this.options.log(`${this.alias}: ${reason}; attaching again.`); try { - const check = await this.run(sshCheckArgs(this.options.session, env), this.timeouts.operation); + const check = await this.masterAnswers(env); if (stale()) return; - if (check.code !== 0) throw unreachable(`The SSH connection to ${this.alias} did not answer. ${firstLines(check.stderr)}`.trim()); + if (!check.ok) throw unreachable(`The SSH connection to ${this.alias} did not answer. ${firstLines(check.stderr)}`.trim()); await this.establish(env, stale, { script: this.attachScript, cli: this.cli }, reuse); } catch (error) { if (stale()) return; @@ -447,6 +454,7 @@ export class ReviewRemoteHost { this.installing?.abort(); const master = this.master; this.master = undefined; + this.closeForwards(); await this.close(master); } @@ -509,9 +517,10 @@ export class ReviewRemoteHost { this.env = env; await Promise.all(this.closing.values()); // None of this host's masters runs now, but one killed by a signal left its socket, and ssh does not multiplex on a path that exists. - await rm(this.options.session.controlPath, { force: true }); + if (this.options.session.controlPath) await rm(this.options.session.controlPath, { force: true }); + const socksPort = this.direct ? await this.options.freePort() : undefined; if (stale()) return; - const master = this.startMaster(env); + const master = this.startMaster(env, socksPort); await this.waitForMaster(master, env, stale); const prepared = await this.prepareAttach(env, stale); if (prepared === undefined) return; @@ -537,7 +546,7 @@ export class ReviewRemoteHost { const language = await this.forwardLanguage(env, attach, stale); if (stale()) return; for (const forward of [kept ? undefined : old, oldLanguage]) { - if (forward) await this.run(sshCancelForwardArgs(this.options.session, forward.local, forward.remote, env), this.timeouts.operation); + if (forward) await this.cancelForward(env, forward); if (stale()) return; } this.attachScript = prepared.script; @@ -554,10 +563,10 @@ export class ReviewRemoteHost { return this.failures++; } - private startMaster(env: NodeJS.ProcessEnv): SshChildProcess { + private startMaster(env: NodeJS.ProcessEnv, socksPort: number | undefined): SshChildProcess { let master: SshChildProcess; try { - master = this.options.spawn(sshMasterArgs(this.options.session, env), { + master = this.options.spawn(sshMasterArgs(this.options.session, env, socksPort), { env, detached: true, stdio: ["ignore", "ignore", "pipe"], @@ -566,6 +575,7 @@ export class ReviewRemoteHost { throw (error as NodeJS.ErrnoException).code === "ENOENT" ? unreachable(OPENSSH_NEEDED) : error; } this.master = master; + this.socksPort = socksPort; this.masterStderr = ""; master.stderr?.setEncoding("utf8"); master.stderr?.on("data", (chunk: string) => (this.masterStderr = (this.masterStderr + chunk).slice(-OUTPUT_LIMIT))); @@ -601,9 +611,9 @@ export class ReviewRemoteHost { let since = Date.now(); for (;;) { if (stale() || gone(master)) throw unreachable("The SSH connection ended."); - const check = await this.run(sshCheckArgs(this.options.session, env), this.timeouts.operation); + const check = await this.masterAnswers(env); if (stale()) throw unreachable("The SSH connection ended."); - if (check.code === 0) return; + if (check.ok) return; if (this.prompts > 0) since = Date.now(); if (Date.now() - since > this.timeouts.connect) { throw unreachable(`${this.alias} did not connect within ${this.timeouts.connect / 1000} seconds. ${firstLines(this.masterStderr)}`.trim()); @@ -612,6 +622,12 @@ export class ReviewRemoteHost { } } + private async masterAnswers(env: NodeJS.ProcessEnv): Promise<{ ok: boolean; stderr: string }> { + if (this.direct) return { ok: this.socksPort !== undefined && (await socksAnswers(this.socksPort)), stderr: "" }; + const check = await this.run(sshCheckArgs(this.options.session, env), this.timeouts.operation); + return { ok: check.code === 0, stderr: check.stderr }; + } + private async prepareAttach(env: NodeJS.ProcessEnv, stale: () => boolean): Promise { const onPath = { script: reviewRemoteAttachScript }; const flow = this.options.install; @@ -799,10 +815,8 @@ export class ReviewRemoteHost { private async forward(env: NodeJS.ProcessEnv, attach: ReviewRemoteAttach, stale: () => boolean): Promise { this.forwarded = undefined; - const port = await this.options.freePort(); - if (stale()) throw unreachable("The SSH connection ended."); - const forward = await this.run(sshForwardArgs(this.options.session, port, attach.port, env), this.timeouts.operation); - if (forward.code !== 0) throw unreachable(`Could not forward a local port to ${this.alias}: ${firstLines(forward.stderr) || `ssh exited with code ${forward.code}`}`); + const port = await this.openForward(env, attach.port, stale); + if (typeof port !== "number") throw unreachable(`Could not forward a local port to ${this.alias}: ${port.error}`); try { await probeHealth(port, this.timeouts.operation); } catch (error) { @@ -820,21 +834,48 @@ export class ReviewRemoteHost { if (!server) return unavailable(attach.languageServerDetail ?? "This host has no VS Code server."); const mismatch = languageCommitMismatch(this.alias, server.commit, this.options.desktopCommit); if (mismatch) return unavailable(mismatch); - const port = await this.options.freePort(); - if (stale()) throw unreachable("The SSH connection ended."); - const forward = await this.run(sshForwardArgs(this.options.session, port, server.port, env), this.timeouts.operation); - if (forward.code !== 0) return unavailable(`Could not forward a local port to the VS Code server on ${this.alias}: ${firstLines(forward.stderr) || `ssh exited with code ${forward.code}`}`); + const port = await this.openForward(env, server.port, stale); + if (typeof port !== "number") return unavailable(`Could not forward a local port to the VS Code server on ${this.alias}: ${port.error}`); try { const commit = await probeVersion(port, this.timeouts.operation); if (commit !== server.commit) throw new Error(`it reports ${commit}, not ${server.commit}`); } catch (error) { - await this.run(sshCancelForwardArgs(this.options.session, port, server.port, env), this.timeouts.operation); + await this.cancelForward(env, { local: port, remote: server.port }); return unavailable(`The VS Code server on ${this.alias} did not answer through the forward: ${(error as Error).message}.`); } this.language = { local: port, remote: server.port, connectionToken: server.connectionToken, commit: server.commit }; return { languageFeatures: true }; } + /** A loopback port that reaches `remote` on the host. */ + private async openForward(env: NodeJS.ProcessEnv, remote: number, stale: () => boolean): Promise { + if (this.direct) { + const socksPort = this.socksPort; + if (socksPort === undefined) throw unreachable("The SSH connection ended."); + const forward = await socksForward(socksPort, remote); + this.socks.set(forward.port, forward); + if (stale()) throw unreachable("The SSH connection ended."); + return forward.port; + } + const port = await this.options.freePort(); + if (stale()) throw unreachable("The SSH connection ended."); + const forward = await this.run(sshForwardArgs(this.options.session, port, remote, env), this.timeouts.operation); + return forward.code === 0 ? port : { error: firstLines(forward.stderr) || `ssh exited with code ${forward.code}` }; + } + + private async cancelForward(env: NodeJS.ProcessEnv, { local, remote }: { local: number; remote: number }): Promise { + const socks = this.socks.get(local); + this.socks.delete(local); + if (socks) await socks.close(); + else if (!this.direct) await this.run(sshCancelForwardArgs(this.options.session, local, remote, env), this.timeouts.operation); + } + + private closeForwards(): void { + for (const forward of this.socks.values()) void forward.close(); + this.socks.clear(); + this.socksPort = undefined; + } + private facts(): Pick { return { ...(this.declined && { declined: true as const }), ...(this.installFailure && { installFailure: this.installFailure }) }; } @@ -863,6 +904,7 @@ export class ReviewRemoteHost { this.master = undefined; this.forwarded = undefined; this.language = undefined; + this.closeForwards(); this.installing?.abort(); void this.close(master); } @@ -873,7 +915,7 @@ export class ReviewRemoteHost { if (pending) return pending; const closed = (async () => { // A master still connecting has no socket to take -O exit. - const asked = this.env && (await this.run(sshCloseArgs(this.options.session, this.env), this.timeouts.close)).code === 0; + const asked = !this.direct && this.env && (await this.run(sshCloseArgs(this.options.session, this.env), this.timeouts.close)).code === 0; if (!asked || !(await exitedWithin(master, this.timeouts.close))) master.kill("SIGTERM"); await exitedWithin(master, this.timeouts.close); })().finally(() => this.closing.delete(master)); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.ts index 609f0bdab..d45d23fb7 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.ts @@ -23,15 +23,18 @@ import { uninstallRemote } from "./reviewRemoteUninstall.js"; import { prepareSshControlDirectory, reviewSshInstancePrefix, + reviewSshDirectSession, reviewSshSession, sshCloseArgs, + type ReviewSshSession, } from "./reviewSshCommand.js"; import { validateSshAlias } from "../../common/reviewSshAlias.js"; export interface ReviewRemoteHostsOptions { readonly spawn: SpawnSsh; - readonly controlDirectory: string; + /** Absent: the connections are direct, and nothing prompts. */ + readonly controlDirectory?: string; readonly instance: string; environment(): Promise; createAskpass(input: { @@ -198,7 +201,7 @@ export class ReviewRemoteHosts { if (!this.options.install) throw new Error("This Desktop cannot install or remove Whiteboard on hosts."); const env = { ...(await this.options.environment()), ...(await this.askpass()).env(alias) }; const script = await this.options.install.script(); - await uninstallRemote({ session: reviewSshSession(alias, this.options.controlDirectory, this.options.instance), spawn: this.options.spawn, env, script }); + await uninstallRemote({ session: this.session(alias), spawn: this.options.spawn, env, script }); } catch (error) { this.options.log(`${alias}: Whiteboard was not removed; the host waits for the setting.`); throw error; @@ -255,7 +258,7 @@ export class ReviewRemoteHosts { private createHost(alias: string): ReviewRemoteHost { return new ReviewRemoteHost({ - session: reviewSshSession(alias, this.options.controlDirectory, this.options.instance), + session: this.session(alias), spawn: this.options.spawn, environment: async () => ({ ...(await this.options.environment()), ...(await this.askpass()).env(alias) }), desktopVersion: () => this.options.desktopVersion(), @@ -276,25 +279,32 @@ export class ReviewRemoteHosts { if (serverId && ![...this.hosts.values()].some((other) => other.serverId === serverId)) this.options.removed?.(serverId); } + private session(alias: string): ReviewSshSession { + const directory = this.options.controlDirectory; + return directory === undefined ? reviewSshDirectSession(alias) : reviewSshSession(alias, directory, this.options.instance); + } + private askpass(): Promise { return (this.prepared ??= (async () => { - await prepareSshControlDirectory(this.options.controlDirectory); - await this.sweepOrphans(); + const directory = this.options.controlDirectory; + if (directory === undefined) return { env: () => ({}), dispose() {} }; + await prepareSshControlDirectory(directory); + await this.sweepOrphans(directory); return this.options.createAskpass({ - directory: this.options.controlDirectory, + directory, prompt: (request) => this.prompt(request), log: this.options.log, }); })()); } - private async sweepOrphans(): Promise { + private async sweepOrphans(directory: string): Promise { const prefix = reviewSshInstancePrefix(this.options.instance); try { const env = await this.options.environment(); - for (const name of await readdir(this.options.controlDirectory)) { + for (const name of await readdir(directory)) { if (!name.startsWith(prefix)) continue; - const controlPath = join(this.options.controlDirectory, name); + const controlPath = join(directory, name); try { // -O exit works on any socket; the alias after -- is only a placeholder. const closed = await runSsh(this.options.spawn, env, sshCloseArgs({ alias: "orphan", controlPath }, env), REVIEW_REMOTE_TIMEOUTS.close); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSocksForward.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSocksForward.test.ts new file mode 100644 index 000000000..9a6f94e94 --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSocksForward.test.ts @@ -0,0 +1,88 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from "node:assert/strict"; +import { once } from "node:events"; +import { connect, createServer, type AddressInfo, type Server, type Socket } from "node:net"; +import test from "node:test"; + +import { socksAnswers, socksForward } from "./reviewSocksForward.js"; + +async function listen(server: Server): Promise { + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + return (server.address() as AddressInfo).port; +} + +/** Like OpenSSH's -D: no auth, CONNECT only, and the reply before the far end answers. */ +function fakeSocks(): Server { + return createServer({ allowHalfOpen: true }, (client) => { + client.once("data", () => { + client.write(Buffer.from([5, 0])); + client.once("data", (request: Buffer) => { + const port = request.readUInt16BE(8); + client.write(Buffer.from([5, 0, 0, 1, 0, 0, 0, 0, 0, 0])); + const far = connect(port, "127.0.0.1"); + far.on("error", () => client.destroy()); + client.on("error", () => far.destroy()); + client.pipe(far).pipe(client); + }); + }); + }); +} + +const echo = () => createServer((socket) => socket.pipe(socket)); + +async function roundTrip(port: number, text: string): Promise { + const socket = connect(port, "127.0.0.1"); + socket.end(text); + let answer = ""; + for await (const chunk of socket) answer += chunk; + return answer; +} + +test("a forward reaches the far port through the SOCKS port, and close ends its connections", async (t) => { + const socks = fakeSocks(); + const far = echo(); + t.after(() => { + socks.close(); + far.close(); + }); + const forward = await socksForward(await listen(socks), await listen(far)); + assert.equal(await roundTrip(forward.port, "hello"), "hello"); + + const open: Socket = connect(forward.port, "127.0.0.1"); + open.write("x"); + await once(open, "data"); + await forward.close(); + await once(open, "close"); + await assert.rejects(roundTrip(forward.port, "again")); +}); + +test("a far port that refuses closes the forwarded connection", async (t) => { + const socks = fakeSocks(); + const closed = echo(); + const closedPort = await listen(closed); + closed.close(); + t.after(() => socks.close()); + const forward = await socksForward(await listen(socks), closedPort); + t.after(() => forward.close()); + assert.equal(await roundTrip(forward.port, "hello").catch(() => ""), ""); +}); + +test("socksAnswers is true only for a SOCKS port", async (t) => { + const socks = fakeSocks(); + const silent = createServer((socket) => socket.end()); + t.after(() => { + socks.close(); + silent.close(); + }); + assert.equal(await socksAnswers(await listen(socks)), true); + assert.equal(await socksAnswers(await listen(silent)), false); + const gone = echo(); + const gonePort = await listen(gone); + gone.close(); + assert.equal(await socksAnswers(gonePort), false); +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSocksForward.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSocksForward.ts new file mode 100644 index 000000000..e334ed3bb --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSocksForward.ts @@ -0,0 +1,111 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { connect, createServer, type AddressInfo, type Socket } from "node:net"; + +const LOOPBACK = "127.0.0.1"; +const HANDSHAKE_MS = 10_000; +const noop = () => {}; + +function read(socket: Socket, length: number): Promise<{ head: Buffer; rest: Buffer }> { + return new Promise((resolve, reject) => { + let buffered = Buffer.alloc(0); + const onData = (chunk: Buffer) => { + buffered = Buffer.concat([buffered, chunk]); + if (buffered.length < length) return; + socket.pause(); + done(); + resolve({ head: buffered.subarray(0, length), rest: buffered.subarray(length) }); + }; + const onClose = () => { + done(); + reject(new Error("The SSH SOCKS port closed the connection.")); + }; + const done = () => { + socket.off("data", onData); + socket.off("close", onClose); + }; + socket.on("data", onData); + socket.once("close", onClose); + socket.resume(); + }); +} + +async function greet(socket: Socket): Promise { + socket.setTimeout(HANDSHAKE_MS, () => socket.destroy()); + socket.write(Buffer.from([5, 1, 0])); + const { head } = await read(socket, 2); + if (head[0] !== 5 || head[1] !== 0) throw new Error("The SSH SOCKS port refused the greeting."); +} + +/** ssh binds its SOCKS port only after logging in, so an answer means the connection is up. */ +export async function socksAnswers(socksPort: number): Promise { + const socket = connect(socksPort, LOOPBACK).on("error", noop); + try { + await greet(socket); + return true; + } catch { + return false; + } finally { + socket.destroy(); + } +} + +async function socksConnect(socksPort: number, port: number): Promise<{ socket: Socket; rest: Buffer }> { + const socket = connect({ port: socksPort, host: LOOPBACK, allowHalfOpen: true }).on("error", noop); + try { + await greet(socket); + socket.write(Buffer.from([5, 1, 0, 1, 127, 0, 0, 1, port >> 8, port & 255])); + // OpenSSH answers with an IPv4 address, and before the remote end accepts: a refusal closes the socket later. + const reply = await read(socket, 10); + if (reply.head[1] !== 0) throw new Error(`The SSH SOCKS port could not reach port ${port} (reply ${reply.head[1]}).`); + socket.setTimeout(0); + return { socket, rest: reply.rest }; + } catch (error) { + socket.destroy(); + throw error; + } +} + +export interface ReviewSocksForward { + readonly port: number; + close(): Promise; +} + +/** A loopback port whose connections reach 127.0.0.1:`remotePort` on the host, through ssh's SOCKS port. */ +export async function socksForward(socksPort: number, remotePort: number): Promise { + const sockets = new Set(); + const track = (socket: Socket) => { + sockets.add(socket); + socket.once("close", () => sockets.delete(socket)); + }; + // Half-open like ssh -L: a client that ends its request still gets the answer. + const server = createServer({ allowHalfOpen: true }, (local) => { + track(local); + local.on("error", noop).pause(); + socksConnect(socksPort, remotePort).then(({ socket, rest }) => { + track(socket); + if (local.destroyed) return void socket.destroy(); + // Ends pass through the pipes; destroying on a clean close could cut an answer still flushing. + socket.once("close", (failed: boolean) => failed && local.destroy()); + local.once("close", (failed: boolean) => failed && socket.destroy()); + if (rest.length) local.write(rest); + socket.pipe(local); + local.pipe(socket); + }, () => local.destroy()); + }); + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, LOOPBACK, () => resolve()); + }); + return { + port: (server.address() as AddressInfo).port, + close: () => + new Promise((resolve) => { + server.close(() => resolve()); + for (const socket of sockets) socket.destroy(); + }), + }; +} diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.test.ts index 17e9e28c9..08b98688a 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.test.ts @@ -13,8 +13,10 @@ import { prepareSshControlDirectory, reviewSshConfigPath, reviewSshControlDirectory, + reviewSshDirectSession, reviewSshInstancePrefix, reviewSshSession, + reviewSshSharesConnections, sshCancelForwardArgs, sshCheckArgs, sshCloseArgs, @@ -49,6 +51,25 @@ test("a session refuses an invalid alias", () => { assert.throws(() => reviewSshSession("-oProxyCommand=x", "/tmp/d", "test"), /starts with -/); }); +test("a direct session never prompts, keeps host-key checking, and has no control socket", () => { + const direct = reviewSshDirectSession("wb-test-a"); + const env = { DEV_FAST_REVIEW_SSH_CONFIG: "/tmp/c", VSCODE_DEV: "1" }; + for (const args of [sshMasterArgs(direct, env, 41000), sshExecArgs(direct, env)]) { + assert.deepEqual(args.slice(0, 3), ["-F", "/tmp/c", "-oBatchMode=yes"]); + for (const arg of args) assert.doesNotMatch(arg, /^-[SOM]$|StrictHostKeyChecking|UserKnownHostsFile|ControlMaster/); + } + assert.throws(() => sshMasterArgs(direct, env), /Invalid port/); + for (const build of [() => sshForwardArgs(direct, 1, 2, env), () => sshCheckArgs(direct, env), () => sshCloseArgs(direct, env)]) + assert.throws(build); +}); + +test("only Windows and the development switch make sessions direct", () => { + assert.equal(reviewSshSharesConnections({}, "darwin"), true); + assert.equal(reviewSshSharesConnections({}, "win32"), false); + assert.equal(reviewSshSharesConnections({ DEV_FAST_REVIEW_SSH_DIRECT: "1", VSCODE_DEV: "1" }, "linux"), false); + assert.equal(reviewSshSharesConnections({ DEV_FAST_REVIEW_SSH_DIRECT: "1" }, "linux"), true); +}); + test("a development config file is passed to every call, and ignored in a packaged build", () => { for (const args of allArgs({ DEV_FAST_REVIEW_SSH_CONFIG: "/tmp/c", VSCODE_DEV: "1" })) assert.deepEqual(args.slice(0, 2), ["-F", "/tmp/c"]); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.ts index 8c63a4e52..1e12903c3 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.ts @@ -16,9 +16,20 @@ import { validateSshAlias } from "../../common/reviewSshAlias.js"; */ export const REVIEW_SSH_CONFIG_ENV = "DEV_FAST_REVIEW_SSH_CONFIG"; +/** Development only: run the Windows connection model on macOS or Linux. */ +export const REVIEW_SSH_DIRECT_ENV = "DEV_FAST_REVIEW_SSH_DIRECT"; + +/** + * Without `controlPath` the session is direct: ssh never prompts, every command opens its own + * connection, and the master only serves a SOCKS port. Win32-OpenSSH has no ControlMaster. + */ export interface ReviewSshSession { readonly alias: string; - readonly controlPath: string; + readonly controlPath?: string; +} + +export function reviewSshSharesConnections(env: NodeJS.ProcessEnv = process.env, platform = process.platform): boolean { + return platform !== "win32" && !(env.VSCODE_DEV && env[REVIEW_SSH_DIRECT_ENV]); } /** Connection sharing needs Unix sockets and a uid; Windows has neither. */ @@ -38,13 +49,22 @@ export function reviewSshInstancePrefix(instance: string): string { return `${sha(instance).slice(0, 6)}-`; } -export function reviewSshSession(alias: string, controlDirectory: string, instance: string): ReviewSshSession { - const valid = validateSshAlias(alias); - if (!valid.ok) throw new Error(`SSH alias ${JSON.stringify(alias)} ${valid.reason}.`); +export function reviewSshSession(alias: string, controlDirectory: string, instance: string): Required { + checkAlias(alias); const name = `${reviewSshInstancePrefix(instance)}${sha(`${instance}\n${alias}`).slice(0, 12)}`; return { alias, controlPath: join(controlDirectory, name) }; } +export function reviewSshDirectSession(alias: string): ReviewSshSession { + checkAlias(alias); + return { alias }; +} + +function checkAlias(alias: string): void { + const valid = validateSshAlias(alias); + if (!valid.ok) throw new Error(`SSH alias ${JSON.stringify(alias)} ${valid.reason}.`); +} + export async function prepareSshControlDirectory(dir: string): Promise { await mkdir(dir, { mode: 0o700 }).catch((error: NodeJS.ErrnoException) => { if (error.code !== "EEXIST") throw error; @@ -66,24 +86,31 @@ export function reviewSshConfigPath(env: NodeJS.ProcessEnv = process.env, home = function base(session: ReviewSshSession, env: NodeJS.ProcessEnv): string[] { const config = developmentConfig(env); - return [...(config ? ["-F", config] : []), "-S", session.controlPath]; + return [...(config ? ["-F", config] : []), ...(session.controlPath ? ["-S", session.controlPath] : ["-oBatchMode=yes"])]; +} + +function shared(session: ReviewSshSession, env: NodeJS.ProcessEnv): string[] { + if (!session.controlPath) throw new Error(`The SSH connection to ${session.alias} is not shared.`); + return base(session, env); } -export function sshMasterArgs(session: ReviewSshSession, env: NodeJS.ProcessEnv = process.env): string[] { - return [ - ...base(session, env), - "-M", - "-N", - "-oServerAliveInterval=15", - "-oServerAliveCountMax=3", - "-oControlPersist=no", - "--", - session.alias, - ]; +function checkPort(port: number | undefined): number { + if (port === undefined || !Number.isInteger(port) || port < 1 || port > 65535) throw new Error(`Invalid port ${port}.`); + return port; +} + +const KEEPALIVE = ["-oServerAliveInterval=15", "-oServerAliveCountMax=3"]; + +/** A direct master only holds the connection and serves `socksPort`, which ssh binds after logging in. */ +export function sshMasterArgs(session: ReviewSshSession, env: NodeJS.ProcessEnv = process.env, socksPort?: number): string[] { + if (!session.controlPath) { + return [...base(session, env), "-N", "-T", "-D", `127.0.0.1:${checkPort(socksPort)}`, "-oExitOnForwardFailure=yes", ...KEEPALIVE, "--", session.alias]; + } + return [...base(session, env), "-M", "-N", ...KEEPALIVE, "-oControlPersist=no", "--", session.alias]; } export function sshExecArgs(session: ReviewSshSession, env: NodeJS.ProcessEnv = process.env, command: readonly string[] = ["sh", "-s"]): string[] { - return [...base(session, env), "-oControlMaster=no", "-T", "--", session.alias, ...command]; + return [...base(session, env), ...(session.controlPath ? ["-oControlMaster=no"] : []), "-T", "--", session.alias, ...command]; } function localForward(localPort: number, remotePort: number): string { @@ -96,7 +123,7 @@ export function sshForwardArgs( remotePort: number, env: NodeJS.ProcessEnv = process.env, ): string[] { - return [...base(session, env), "-O", "forward", "-L", localForward(localPort, remotePort), "--", session.alias]; + return [...shared(session, env), "-O", "forward", "-L", localForward(localPort, remotePort), "--", session.alias]; } export function sshCancelForwardArgs( @@ -105,13 +132,13 @@ export function sshCancelForwardArgs( remotePort: number, env: NodeJS.ProcessEnv = process.env, ): string[] { - return [...base(session, env), "-O", "cancel", "-L", localForward(localPort, remotePort), "--", session.alias]; + return [...shared(session, env), "-O", "cancel", "-L", localForward(localPort, remotePort), "--", session.alias]; } export function sshCheckArgs(session: ReviewSshSession, env: NodeJS.ProcessEnv = process.env): string[] { - return [...base(session, env), "-O", "check", "--", session.alias]; + return [...shared(session, env), "-O", "check", "--", session.alias]; } export function sshCloseArgs(session: ReviewSshSession, env: NodeJS.ProcessEnv = process.env): string[] { - return [...base(session, env), "-O", "exit", "--", session.alias]; + return [...shared(session, env), "-O", "exit", "--", session.alias]; } diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshDirect.containers.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshDirect.containers.test.ts new file mode 100644 index 000000000..2d62d4c6a --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshDirect.containers.test.ts @@ -0,0 +1,70 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from "node:assert/strict"; +import { execFile, execFileSync, spawn } from "node:child_process"; +import { randomBytes } from "node:crypto"; +import { connect } from "node:net"; +import { resolve } from "node:path"; +import { after, before, test } from "node:test"; +import { promisify } from "node:util"; + +import { runSsh, type SpawnSsh } from "./reviewRemoteHost.js"; +import { freeLoopbackPort } from "./reviewRemoteHosts.js"; +import { socksAnswers, socksForward } from "./reviewSocksForward.js"; +import { REVIEW_SSH_CONFIG_ENV, reviewSshDirectSession, sshExecArgs, sshMasterArgs } from "./reviewSshCommand.js"; + +const run = promisify(execFile); +const harness = resolve(import.meta.dirname, "../../../../../../scripts/e2e/remote/remote.mjs"); + +function skipReason(): string | undefined { + if (process.env.WB_TEST_CONTAINERS !== "1") return "set WB_TEST_CONTAINERS=1 to run direct SSH against a container"; + try { + execFileSync("docker", ["info"], { stdio: "ignore" }); + } catch { + return "Docker is not available"; + } + return undefined; +} + +const skip = skipReason(); +const runId = `direct${randomBytes(3).toString("hex")}`; +const session = reviewSshDirectSession("wb-test-a"); +const env = { ...process.env, VSCODE_DEV: "1", [REVIEW_SSH_CONFIG_ENV]: `/tmp/wbt.${runId}/ssh_config` }; +const realSsh: SpawnSsh = (args, options) => spawn("ssh", args, options); +const remote = (args: string[]) => run(process.execPath, [harness, ...args], { env: { ...process.env, WB_TEST_RUN: runId }, maxBuffer: 16 << 20 }); + +async function readAll(port: number): Promise { + let text = ""; + for await (const chunk of connect(port, "127.0.0.1")) { + text += chunk; + if (text.includes("\n")) break; + } + return text; +} + +before(async () => void (skip || (await remote(["up", "a", "--node", "none"]))), { timeout: 20 * 60_000 }); +after(async () => void (skip || (await remote(["down", "--all"]))), { timeout: 5 * 60_000 }); + +test("an exec opens its own connection without prompting", { skip }, async () => { + const result = await runSsh(realSsh, env, sshExecArgs(session, env), 30_000, "echo direct-ok\n"); + assert.equal(result.stdout.trim(), "direct-ok", result.stderr); +}); + +test("a direct master serves SOCKS once logged in, and a forward reaches the host", { skip }, async (t) => { + const socksPort = await freeLoopbackPort(); + const master = spawn("ssh", sshMasterArgs(session, env, socksPort), { env, stdio: ["ignore", "ignore", "pipe"] }); + t.after(() => master.kill()); + const deadline = Date.now() + 30_000; + while (!(await socksAnswers(socksPort))) { + assert.equal(master.exitCode, null, "the master exited"); + assert.ok(Date.now() < deadline, "no SOCKS port within 30 seconds"); + await new Promise((done) => setTimeout(done, 250)); + } + + const sshd = await socksForward(socksPort, 22); + t.after(() => sshd.close()); + assert.match(await readAll(sshd.port), /^SSH-2\.0-/); +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopHost.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopHost.ts index 7f4657fef..b4f552a63 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopHost.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopHost.ts @@ -65,6 +65,7 @@ import { createSshAskpass } from "./remote/reviewSshAskpass.js"; import { reviewSshConfigPath, reviewSshControlDirectory, + reviewSshSharesConnections, } from "./remote/reviewSshCommand.js"; import { listSshAliases } from "./remote/reviewSshConfigAliases.js"; import { reviewSshPromptRelay } from "./remote/reviewSshPromptRelay.js"; @@ -361,7 +362,9 @@ export class ReviewDesktopHost extends Disposable { let version: Promise | undefined; const manager = new ReviewRemoteHosts({ spawn: (args, options) => spawn("ssh", args, options), - controlDirectory: reviewSshControlDirectory(), + controlDirectory: reviewSshSharesConnections() + ? reviewSshControlDirectory() + : undefined, instance: this.environmentMainService.userDataPath, environment: async () => ({ ...process.env, diff --git a/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs b/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs index 73fbbd264..5ba0df93b 100644 --- a/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs @@ -685,6 +685,7 @@ const desktopSsh = async () => export async function masterPid() { return (await desktopSshProcesses()).find( - ([, args]) => args.includes(" -M -N ") && args.endsWith(`-- ${alias}`), + ([, args]) => + / -M -N | -N -T -D /.test(args) && args.endsWith(`-- ${alias}`), )?.[0]; } diff --git a/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs b/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs index 5c3ba9a19..534b98b65 100644 --- a/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs @@ -190,7 +190,8 @@ async function desktopSsh() { const masterPid = async (host) => (await desktopSsh()).find( - ([, args]) => args.includes(" -M -N ") && args.endsWith(`-- ${host}`), + ([, args]) => + / -M -N | -N -T -D /.test(args) && args.endsWith(`-- ${host}`), )?.[0]; function watcher(ctx) { From 0c1bd804264abb227d78be85dae3616eb510cf9b Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Wed, 14 Oct 2026 20:35:55 -0400 Subject: [PATCH 2/2] Tighten direct SSH mode: close a late forward, keep the master's stderr, give the probe the connect allowance, and read the dev switch as 1 --- .../review/electron-main/remote/reviewRemoteHost.ts | 13 +++++++++---- .../electron-main/remote/reviewSshCommand.test.ts | 1 + .../review/electron-main/remote/reviewSshCommand.ts | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts index 5023b3eac..06b3c0f31 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts @@ -13,7 +13,7 @@ import { installedAttachScript, parseRemoteAttach, reviewRemoteAttachScript, typ import type { ReviewRemoteInstallConsent } from "./reviewRemoteInstallConsent.js"; import type { ReviewRemoteInstallInput, ReviewRemoteInstallResult } from "./reviewRemoteInstaller.js"; import { reviewRemoteVersionDir } from "./reviewRemoteInstallScript.js"; -import { judgeRemote, missingInstallTools, probeRemote, type ReviewRemoteProbe, type ReviewRemoteTarget } from "./reviewRemoteProbe.js"; +import { judgeRemote, missingInstallTools, probeRemote, REVIEW_REMOTE_PROBE_TIMEOUT, type ReviewRemoteProbe, type ReviewRemoteTarget } from "./reviewRemoteProbe.js"; import { socksAnswers, socksForward, type ReviewSocksForward } from "./reviewSocksForward.js"; import { sshCancelForwardArgs, @@ -623,7 +623,7 @@ export class ReviewRemoteHost { } private async masterAnswers(env: NodeJS.ProcessEnv): Promise<{ ok: boolean; stderr: string }> { - if (this.direct) return { ok: this.socksPort !== undefined && (await socksAnswers(this.socksPort)), stderr: "" }; + if (this.direct) return { ok: this.socksPort !== undefined && (await socksAnswers(this.socksPort)), stderr: firstLines(this.masterStderr) }; const check = await this.run(sshCheckArgs(this.options.session, env), this.timeouts.operation); return { ok: check.code === 0, stderr: check.stderr }; } @@ -635,7 +635,9 @@ export class ReviewRemoteHost { if (!flow || mode === "never") return onPath; const origin = flow.origin(); const script = await flow.script(); - const probed = await probeRemote({ session: this.options.session, spawn: this.options.spawn, env, script, releases: origin && `${origin}/` }); + // A direct probe logs in afresh, so it gets the connect allowance too. + const timeout = this.direct ? REVIEW_REMOTE_PROBE_TIMEOUT + this.timeouts.connect : undefined; + const probed = await probeRemote({ session: this.options.session, spawn: this.options.spawn, env, script, releases: origin && `${origin}/`, timeout }); if (stale()) return; if ("error" in probed) throw unreachable(probed.error); const { probe } = probed; @@ -853,8 +855,11 @@ export class ReviewRemoteHost { const socksPort = this.socksPort; if (socksPort === undefined) throw unreachable("The SSH connection ended."); const forward = await socksForward(socksPort, remote); + if (stale()) { + await forward.close(); + throw unreachable("The SSH connection ended."); + } this.socks.set(forward.port, forward); - if (stale()) throw unreachable("The SSH connection ended."); return forward.port; } const port = await this.options.freePort(); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.test.ts index 08b98688a..d879e52dd 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.test.ts @@ -68,6 +68,7 @@ test("only Windows and the development switch make sessions direct", () => { assert.equal(reviewSshSharesConnections({}, "win32"), false); assert.equal(reviewSshSharesConnections({ DEV_FAST_REVIEW_SSH_DIRECT: "1", VSCODE_DEV: "1" }, "linux"), false); assert.equal(reviewSshSharesConnections({ DEV_FAST_REVIEW_SSH_DIRECT: "1" }, "linux"), true); + assert.equal(reviewSshSharesConnections({ DEV_FAST_REVIEW_SSH_DIRECT: "0", VSCODE_DEV: "1" }, "linux"), true); }); test("a development config file is passed to every call, and ignored in a packaged build", () => { diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.ts index 1e12903c3..c5bc37fbc 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewSshCommand.ts @@ -29,7 +29,7 @@ export interface ReviewSshSession { } export function reviewSshSharesConnections(env: NodeJS.ProcessEnv = process.env, platform = process.platform): boolean { - return platform !== "win32" && !(env.VSCODE_DEV && env[REVIEW_SSH_DIRECT_ENV]); + return platform !== "win32" && !(env.VSCODE_DEV && env[REVIEW_SSH_DIRECT_ENV] === "1"); } /** Connection sharing needs Unix sockets and a uid; Windows has neither. */