From c2492a3523b4c10844ebe72d43cced23c3af1f3d Mon Sep 17 00:00:00 2001 From: d fei Date: Tue, 1 Sep 2026 07:50:50 -0700 Subject: [PATCH] feat(remote): support password-authenticated SSH hosts, storing the password optionally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remote path always passed `-o BatchMode=yes`, which disables every interactive prompt, so password-only hosts could never be used. BatchMode is not an oversight: Codeman launches ssh non-interactively from a service process with no terminal and nobody watching, and without it ssh hangs on a password prompt no one will ever answer — a dead pane, which is worse than an error. So the password goes through sshpass, handed to ssh in the SSHPASS environment variable: never in argv (same-host users can read /proc) and never in a temp file. The variable itself is injected into the pane with socket-scoped `tmux setenv`, the same rule every other secret here follows. ⚠️ One thing measured, and a naive implementation will hit it: BatchMode=yes and sshpass are mutually exclusive. The former disables the password prompt, and answering that prompt is exactly how sshpass works, so using both yields `Permission denied (publickey,password)` — which reads like a wrong password rather than wrong arguments. With a password we therefore send BatchMode=no plus NumberOfPasswordPrompts=1, the latter so a wrong password fails immediately instead of hanging (also measured). ⚠️ The preflight probe runs in the server process, not in the pane, so `tmux setenv` does not reach it and that path passes the variable through the child environment instead. A missing sshpass is reported as a named prerequisite during the probe as well; otherwise it surfaces as a pane dying with "sshpass: command not found", which reads like a broken host. Storage and exposure: - remote-hosts.json now holds a secret, so it is written 0600, and an existing file is explicitly tightened once (writeFile's mode only applies on create) - the API always redacts, returning only a `passwordSet` boolean - updates merge the stored password, because a redacted host posted back carries no `password` and a straight write would silently erase it; an explicit empty string still means "clear" - the schema deliberately does not apply NO_SHELL_META to `password`: a password legitimately contains `$` and backticks, and unlike the path fields it is never interpolated into a shell string --- src/remote-hosts.ts | 121 ++++++++++++++++++++++++++++++- src/tmux-manager.ts | 32 ++++++++ src/types/session.ts | 11 +++ src/web/routes/case-routes.ts | 17 ++++- src/web/schemas.ts | 9 +++ test/remote-ssh-password.test.ts | 110 ++++++++++++++++++++++++++++ 6 files changed, 292 insertions(+), 8 deletions(-) create mode 100644 test/remote-ssh-password.test.ts diff --git a/src/remote-hosts.ts b/src/remote-hosts.ts index 5fce5128..f9a9d01f 100644 --- a/src/remote-hosts.ts +++ b/src/remote-hosts.ts @@ -2,7 +2,7 @@ import { existsSync, mkdirSync } from 'node:fs'; import fs from 'node:fs/promises'; import { join } from 'node:path'; import { homedir } from 'node:os'; -import { exec } from 'node:child_process'; +import { exec, execFileSync } from 'node:child_process'; import { promisify } from 'node:util'; import type { RemoteCase, @@ -37,15 +37,88 @@ async function readJsonArray(path: string): Promise { } } +/** + * ⚠️ 0600: remote-hosts.json can now hold an SSH password (RemoteSshOptions.password). + * The mode is applied to every file written here rather than to that one path, so a + * future secret in a sibling registry cannot land world-readable by omission. + */ async function writeJsonArray(configDir: string, path: string, value: T[]): Promise { if (!existsSync(configDir)) mkdirSync(configDir, { recursive: true }); - await fs.writeFile(path, JSON.stringify(value, null, 2)); + await fs.writeFile(path, JSON.stringify(value, null, 2), { mode: 0o600 }); + // writeFile's `mode` applies only when it CREATES the file; an existing one keeps + // whatever it had, so tighten it explicitly for registries written before this. + // + // ⚠️ try/catch, not `.catch()`: on a filesystem without chmod semantics — or a + // test double that stubs only readFile/writeFile — `fs.chmod` can be missing + // outright, and calling it then throws SYNCHRONOUSLY, which no `.catch()` on the + // (never-returned) promise can absorb. Tightening the mode is best-effort; the + // file has already been written with the right mode when it was created. + try { + await fs.chmod(path, 0o600); + } catch { + /* best-effort */ + } } export async function readRemoteHosts(configDir: string): Promise { return readJsonArray(remoteHostsPath(configDir)); } +/** + * Strip the stored SSH password before a host leaves the server. + * + * The API answers with `passwordSet: true` instead, which is what a UI needs to + * render "saved — replace or clear" without ever shipping the secret to a + * browser. Callers that PERSIST must merge against the stored host (see + * mergeRemoteHostSecret) or a round-trip through the UI would erase it. + */ +export function redactRemoteHost(host: RemoteHost): RemoteHost & { passwordSet: boolean } { + const { password, ...rest } = host; + return { ...rest, passwordSet: !!password }; +} + +/** + * Carry a stored password across an update that did not send one. + * + * A redacted host round-tripping through the UI has no `password` field, so a + * blind write would silently drop it and the next launch would fall back to key + * auth and fail. An EXPLICIT empty string is the clear-it gesture and must be + * honoured, which is why this distinguishes undefined from ''. + */ +export function mergeRemoteHostSecret(incoming: RemoteHost, stored?: RemoteHost): RemoteHost { + if (incoming.password !== undefined) { + return incoming.password === '' ? { ...incoming, password: undefined } : incoming; + } + return stored?.password ? { ...incoming, password: stored.password } : incoming; +} + +/** + * Is `sshpass` runnable on this host? + * + * Password auth needs it, and it ships in no base image — a missing binary + * otherwise surfaces as a pane that dies with `sshpass: command not found`, + * which reads like a broken host rather than a missing prerequisite. Cached + * per-process: the answer cannot change without an install, and probing on every + * launch would fork a process per session start. + */ +let _sshpassAvailable: boolean | undefined; +export function sshpassAvailable(): boolean { + if (_sshpassAvailable === undefined) { + try { + execFileSync('sh', ['-c', 'command -v sshpass'], { stdio: 'ignore', timeout: 5000 }); + _sshpassAvailable = true; + } catch { + _sshpassAvailable = false; + } + } + return _sshpassAvailable; +} + +/** Test seam: forget the cached probe. */ +export function resetSshpassProbe(): void { + _sshpassAvailable = undefined; +} + export async function writeRemoteHosts(configDir: string, hosts: RemoteHost[]): Promise { await writeJsonArray(configDir, remoteHostsPath(configDir), hosts); } @@ -177,7 +250,29 @@ function expandIdentityPath(identityFile: string): string { * operator already set ConnectTimeout via extraSshOptions, so their value wins. */ export function buildSshConnectionArgs(remote: RemoteSshOptions & Pick): string[] { - const parts: string[] = ['ssh', '-o BatchMode=yes']; + // A stored password goes to ssh through `sshpass -e`, which reads it from the + // SSHPASS environment variable — never from argv (world-readable via /proc on a + // shared host) and never from a temp file we would have to clean up. The + // variable itself is injected into the pane with socket-scoped `tmux setenv`, + // the same discipline every other secret here follows. + // + // ⚠️ MEASURED, and the naive version fails silently: `BatchMode=yes` disables + // the password PROMPT, and answering that prompt is exactly how sshpass works, + // so the two are mutually exclusive — `sshpass -e ssh -o BatchMode=yes host` + // comes back `Permission denied (publickey,password)`, which reads like a wrong + // password rather than a wrong flag. With a password we therefore send + // `BatchMode=no` plus `NumberOfPasswordPrompts=1`, which keeps the launch + // non-interactive in the way that actually matters here: a WRONG password fails + // immediately instead of ssh sitting on a prompt nobody can answer (also + // measured — it returns at once rather than hanging). + // + // ⚠️ `sshpass -e ssh` is the FIRST TOKEN rather than a separate array entry + // because several callers destructure `[ssh, batchMode, ...rest]` and re-insert + // flags at fixed positions (notably `-t`); a prepended entry would shift those. + const usesPassword = !!remote.password; + const parts: string[] = usesPassword + ? ['sshpass -e ssh', '-o BatchMode=no', '-o NumberOfPasswordPrompts=1'] + : ['ssh', '-o BatchMode=yes']; const hasConnectTimeout = (remote.extraSshOptions ?? []).some((opt) => /^ConnectTimeout=/i.test(opt)); if (!hasConnectTimeout) parts.push('-o ConnectTimeout=10'); if (remote.port) parts.push(`-p ${remote.port}`); @@ -234,9 +329,27 @@ export async function checkRemoteTmuxAvailable( if (process.env.VITEST) { return { ok: true, tmuxPath: '(test-mode)' }; } + // ⚠️ A password host needs sshpass BOTH here and in the pane, and this probe is + // the earlier of the two — catching it here turns "the pane died with + // `sshpass: command not found`", which reads like a broken host, into a named + // prerequisite at link time. Mirrors how the tmux prerequisite itself is handled. + if (host.password && !sshpassAvailable()) { + return { + ok: false, + error: + 'This host authenticates with a password, which needs `sshpass` on the Codeman machine, and it is not installed. Install it (Debian/Ubuntu: `apt install sshpass`), or give the host an SSH key instead.', + }; + } const command = buildRemoteTmuxCheckCommand(host); try { - const { stdout } = await execAsync(command, { timeout: 15_000 }); + // ⚠️ This probe runs in the SERVER process, not in a tmux pane, so the pane's + // `tmux setenv SSHPASS` does not apply to it. The password is handed to this + // child through its ENVIRONMENT rather than the command line, which is the same + // trust level (`/proc//environ` is owner-readable) and keeps it out of `ps`. + const { stdout } = await execAsync(command, { + timeout: 15_000, + ...(host.password ? { env: { ...process.env, SSHPASS: host.password } } : {}), + }); const tmuxPath = stdout.trim(); if (!tmuxPath) { return { diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index c0ec1f08..9fb6edb8 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -2110,6 +2110,30 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { /** * Configure Gemini-specific environment on a tmux session. */ + /** + * Hand a remote host's stored SSH password to the pane as `SSHPASS`. + * + * `buildSshConnectionArgs` launches such a host through `sshpass -e`, which + * reads exactly this variable. Injected with socket-scoped `tmux setenv` so the + * secret never appears in `ps` — the same discipline the CLI API keys follow — + * and inherited by `respawn-pane`, so a respawned wrapper reconnects without the + * server having to re-send anything. + * + * ⚠️ A user's own `envOverrides` cannot reach this variable: `SSHPASS` matches + * no entry in ALLOWED_ENV_PREFIXES / ALLOWED_ENV_KEYS, so unlike the DeepSeek + * case there is no later `applyEnvOverrides` pass that could override it. + */ + private _configureSshPassword(muxName: string, remote?: SessionRemote): void { + if (!remote?.password) return; + try { + execSync(`${this.tmux()} setenv -t "${muxName}" SSHPASS ${shellescape(remote.password)}`, { stdio: 'ignore' }); + } catch { + // Best-effort like the sibling _configure* helpers: a failed setenv surfaces + // as an auth failure in the pane, which is visible, rather than as a thrown + // session-create that hides the real cause. + } + } + private _configureGemini(muxName: string): void { setGeminiEnvVars(this.tmux(), muxName); } @@ -2373,6 +2397,10 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { // Apply user-supplied env overrides (e.g., CLAUDE_CODE_EFFORT_LEVEL) via tmux setenv // so secret values stay off the bash command line. Must run before respawn-pane. + // A password-authenticated remote host needs SSHPASS in the pane (see + // _configureSshPassword); a no-op for key auth and every non-remote session. + this._configureSshPassword(muxName, remote); + this.applyEnvOverrides(muxName, envOverrides); // Replace the shell with the actual command (no echo in terminal). Keep @@ -2602,6 +2630,10 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { } // Re-apply user env overrides before respawn so the new shell inherits them. + // A password-authenticated remote host needs SSHPASS in the pane (see + // _configureSshPassword); a no-op for key auth and every non-remote session. + this._configureSshPassword(muxName, remote); + this.applyEnvOverrides(muxName, envOverrides); // -c /tmp + cd bounce — see createSession() for rationale (stale FUSE state). diff --git a/src/types/session.ts b/src/types/session.ts index 4d2398b4..58dee9c5 100644 --- a/src/types/session.ts +++ b/src/types/session.ts @@ -88,6 +88,17 @@ export interface RemoteSshOptions { jumpHost?: string; /** Arbitrary additional `-o KEY=VALUE` options (escape hatch). Each `KEY=VALUE`. */ extraSshOptions?: string[]; + /** + * SSH password, for hosts that accept no key. OPTIONAL and opt-in: storing it + * is a user choice, and a host without it keeps the key-only path unchanged. + * + * ⚠️ This is the one secret in remote-hosts.json, which is why that file is + * written 0600. It is never returned by the API (see redactRemoteHost) and + * never reaches a command line — it goes to ssh through `sshpass -e`, which + * reads the SSHPASS environment variable, and that variable is injected into + * the pane with socket-scoped `tmux setenv` like every other secret here. + */ + password?: string; } export interface RemoteHost extends RemoteSshOptions { diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index a296eb2d..9c68762d 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -85,6 +85,8 @@ import type { AdoptedContainerProbe, DockerBrowseResult, DockerContainerInfo } f import { buildDockerRemoveCommand } from '../../tmux-manager.js'; import { checkRemoteTmuxAvailable, + redactRemoteHost, + mergeRemoteHostSecret, listRemoteCodemanSessions, readRemoteCases, readRemoteHosts, @@ -551,8 +553,11 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config // Hosts are machine-level infra config (ssh users/identity paths): non-admins get an // empty list in multi-user mode, matching the admin-only write side. No-op otherwise. + // ⚠️ redactRemoteHost: a host may now carry an SSH password, and it must never + // reach a browser. Callers get `passwordSet` instead, which is all a UI needs to + // render "saved — replace or clear". app.get('/api/remote-hosts', async (req) => - isMultiUserMode() && !isAdmin(req) ? [] : readRemoteHosts(CODEMAN_CONFIG_DIR) + isMultiUserMode() && !isAdmin(req) ? [] : (await readRemoteHosts(CODEMAN_CONFIG_DIR)).map(redactRemoteHost) ); // Hosts are machine-level resources: only admins may define them in multi-user mode. @@ -590,7 +595,7 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config return createErrorResponse(ApiErrorCode.ALREADY_EXISTS, 'Remote host already exists'); } await writeRemoteHosts(CODEMAN_CONFIG_DIR, [...hosts, host]); - return { success: true, data: { host } }; + return { success: true, data: { host: redactRemoteHost(host) } }; }); app.put('/api/remote-hosts/:id', async (req, reply): Promise> => { @@ -602,9 +607,13 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config const index = hosts.findIndex((item) => item.id === id); if (index === -1) return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Remote host not found'); const next = [...hosts]; - next[index] = host; + // ⚠️ The GET above redacts the password, so a host round-tripping through the + // UI arrives WITHOUT one; writing it straight back would silently erase the + // stored secret and the next launch would fall back to key auth and fail. An + // explicit empty string still clears it — see mergeRemoteHostSecret. + next[index] = mergeRemoteHostSecret(host, hosts[index]); await writeRemoteHosts(CODEMAN_CONFIG_DIR, next); - return { success: true, data: { host } }; + return { success: true, data: { host: redactRemoteHost(next[index]) } }; }); app.delete('/api/remote-hosts/:id', async (req, reply): Promise> => { diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 34fa6349..5ed35c09 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -650,6 +650,15 @@ export const RemoteHostSchema = z.object({ .max(100) .regex(/^[a-zA-Z0-9._-]+$/, 'Invalid SSH username'), port: z.number().int().min(1).max(65535).optional(), + // SSH password for hosts that accept no key. Opt-in, stored 0600, never + // returned by the API (redactRemoteHost) and never placed on a command line — + // it reaches ssh through `sshpass -e` / the SSHPASS environment variable. + // + // ⚠️ Deliberately NOT filtered by NO_SHELL_META like the path fields below: a + // password legitimately contains `$` and backticks, and unlike those it is never + // interpolated into a shell string. An EMPTY string is allowed on purpose — it is + // the "clear the stored password" gesture (see mergeRemoteHostSecret). + password: z.string().max(1024).optional(), // Identity (private-key) file PATH only — never key bytes. Reject shell // metacharacters ($, backtick) that survive into the `bash -c` launch layer. identityFile: z.string().min(1).max(4096).regex(NO_SHELL_META, 'Invalid identity file path').optional(), diff --git a/test/remote-ssh-password.test.ts b/test/remote-ssh-password.test.ts new file mode 100644 index 00000000..5670f839 --- /dev/null +++ b/test/remote-ssh-password.test.ts @@ -0,0 +1,110 @@ +/** + * Password auth for remote hosts that accept no key. + * + * The password is handed to ssh through `sshpass -e` (read from the SSHPASS + * environment variable, never argv), and the variable itself is injected into + * the pane with socket-scoped `tmux setenv` like every other secret here. + */ +import { promises as fs } from 'node:fs'; +import { mkdtempSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { + buildSshConnectionArgs, + redactRemoteHost, + mergeRemoteHostSecret, + writeRemoteHosts, + remoteHostsPath, +} from '../src/remote-hosts.js'; +import type { RemoteHost } from '../src/types.js'; + +const host: RemoteHost = { id: 'h1', label: 'box', host: 'example.com', username: 'root' }; + +describe('ssh connection args with a stored password', () => { + it('keeps the key-only path byte-identical when no password is set', () => { + expect(buildSshConnectionArgs(host).slice(0, 2)).toEqual(['ssh', '-o BatchMode=yes']); + }); + + it('switches the launcher to sshpass -e and DROPS BatchMode=yes', () => { + // MEASURED against a real host: `BatchMode=yes` disables the password prompt, + // and answering that prompt is exactly how sshpass works, so the pair comes + // back "Permission denied (publickey,password)" — which reads like a wrong + // password rather than a wrong flag. This is the assertion that keeps someone + // from "restoring" BatchMode=yes for consistency with the key path. + const args = buildSshConnectionArgs({ ...host, password: 'pw' }); + expect(args[0]).toBe('sshpass -e ssh'); + expect(args.join(' ')).not.toContain('BatchMode=yes'); + expect(args).toContain('-o BatchMode=no'); + }); + + it('caps password prompts at one so a wrong password fails instead of waiting', () => { + expect(buildSshConnectionArgs({ ...host, password: 'pw' })).toContain('-o NumberOfPasswordPrompts=1'); + }); + + it('never puts the password itself on the command line', () => { + const joined = buildSshConnectionArgs({ ...host, password: 'hunter2' }).join(' '); + expect(joined).not.toContain('hunter2'); + }); + + it('keeps sshpass as ONE leading token, since callers splice flags by position', () => { + // buildRemoteLaunchCommand does `const [ssh, batchMode, ...rest]` and inserts + // `-t` between them; a separate 'sshpass' entry would shift that insertion. + const args = buildSshConnectionArgs({ ...host, password: 'pw', port: 2222 }); + const [launcher, batch, ...rest] = args; + expect(launcher.split(' ')).toEqual(['sshpass', '-e', 'ssh']); + expect([launcher, batch, '-t', ...rest].join(' ')).toMatch(/^sshpass -e ssh -o BatchMode=no -t /); + }); + + it('still carries port and identity alongside the password launcher', () => { + const args = buildSshConnectionArgs({ ...host, password: 'pw', port: 2222 }).join(' '); + expect(args).toContain('-p 2222'); + }); +}); + +describe('the password never leaves the server', () => { + it('redacts it and reports only whether one is set', () => { + const out = redactRemoteHost({ ...host, password: 'secret' }); + expect(out.passwordSet).toBe(true); + expect('password' in out).toBe(false); + expect(JSON.stringify(out)).not.toContain('secret'); + }); + + it('reports passwordSet false for a key-only host', () => { + expect(redactRemoteHost(host).passwordSet).toBe(false); + }); + + it('carries a stored password across an update that omits it', () => { + // A redacted host round-tripping through the UI has no password field; a blind + // write would drop it and the next launch would silently fall back to key auth. + expect(mergeRemoteHostSecret(host, { ...host, password: 'kept' }).password).toBe('kept'); + }); + + it('treats an EXPLICIT empty string as "clear it"', () => { + expect(mergeRemoteHostSecret({ ...host, password: '' }, { ...host, password: 'old' }).password).toBeUndefined(); + }); + + it('lets an explicit new password win over the stored one', () => { + expect(mergeRemoteHostSecret({ ...host, password: 'new' }, { ...host, password: 'old' }).password).toBe('new'); + }); +}); + +describe('remote-hosts.json holds a secret, so it is 0600', () => { + it('writes the registry unreadable by other users', async () => { + const dir = mkdtempSync(join(tmpdir(), 'cm-rh-')); + await writeRemoteHosts(dir, [{ ...host, password: 'secret' }]); + const mode = (await fs.stat(remoteHostsPath(dir))).mode & 0o777; + expect(mode).toBe(0o600); + }); + + it('tightens a registry that already existed with looser permissions', async () => { + // writeFile's `mode` applies only on CREATE, so an install that predates this + // would keep 0644 forever without the explicit chmod. + const dir = mkdtempSync(join(tmpdir(), 'cm-rh-')); + const path = remoteHostsPath(dir); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path, '[]', { mode: 0o644 }); + await writeRemoteHosts(dir, [{ ...host, password: 'secret' }]); + expect((await fs.stat(path)).mode & 0o777).toBe(0o600); + }); +});