diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index fe8aaf66d08..0b5ba66fb54 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -13,6 +13,14 @@ let connectErrorCode = '' let destroyErrorMessage = '' let connectSequence: ('ready' | Error)[] = [] let connectAttempts = 0 +/** Lets a test present a real key blob instead of the placeholder. */ +const VALID_ED25519_HOST_KEY = Buffer.from( + 'AAAAC3NzaC1lZDI1NTE5AAAAIKqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq', + 'base64' +) +let mockPresentedHostKey: Buffer | undefined +/** What the verifier decided about the presented key on the most recent connect. */ +let lastHostKeyAccepted: boolean | undefined let execBehavior: 'callback' | 'pending' = 'callback' let pendingExecCallback: ((err: Error | undefined, channel: unknown) => void) | null = null let sftpBehavior: 'callback' | 'pending' = 'callback' @@ -86,9 +94,29 @@ vi.mock('ssh2', () => { connect(config?: unknown) { connectAttempts += 1 this.lastConnectConfig = config - const hostVerifier = (config as { hostVerifier?: (key: Buffer) => boolean } | undefined) - ?.hostVerifier - hostVerifier?.(Buffer.from('mock-ssh-host-key')) + // Why the callback form: ssh2 calls hostVerifier(key, verify) and only accepts synchronously + // when the return is not undefined. A mock that passed one argument and ignored the result + // would pass against a verifier that never decides — which is the regression this whole + // module exists to prevent. + const hostVerifier = ( + config as + | { hostVerifier?: (key: Buffer, verify: (ok: boolean) => void) => undefined } + | undefined + )?.hostVerifier + const presentedHostKey = mockPresentedHostKey ?? VALID_ED25519_HOST_KEY + lastHostKeyAccepted = undefined + hostVerifier?.(presentedHostKey, (ok) => { + lastHostKeyAccepted = ok + }) + if (lastHostKeyAccepted === false) { + // ssh2 aborts the handshake when the verifier denies; a mock that carried on to 'ready' + // would let a rejected host key look like a successful connect. + setTimeout( + () => emitSshEvent('error', new Error('All configured authentication methods failed')), + 0 + ) + return + } setTimeout(() => { const next = connectSequence.shift() if (next instanceof Error) { @@ -326,6 +354,8 @@ describe('SshConnection', () => { destroyErrorMessage = '' connectSequence = [] connectAttempts = 0 + mockPresentedHostKey = undefined + lastHostKeyAccepted = undefined execBehavior = 'callback' pendingExecCallback = null sftpBehavior = 'callback' @@ -378,6 +408,23 @@ describe('SshConnection', () => { expect(clientInstances[0].setNoDelay).toHaveBeenCalledWith(true) }) + // Proves the WIRING, not just the module: a verifier that decides correctly is worthless if the + // handshake never consults it, and until this change ssh-connection accepted every key. + it('refuses a host key whose own blob cannot be identified', async () => { + mockPresentedHostKey = Buffer.from('not-a-real-host-key-blob') + const conn = new SshConnection(createTarget(), createCallbacks()) + + await expect(conn.connect()).rejects.toThrow() + expect(lastHostKeyAccepted, 'the handshake accepted an unidentifiable key').toBe(false) + }) + + it('accepts a well-formed host key on first contact', async () => { + const conn = new SshConnection(createTarget(), createCallbacks()) + await conn.connect() + + expect(lastHostKeyAccepted).toBe(true) + }) + it('captures the negotiated SSH server key fingerprint', async () => { const conn = new SshConnection(createTarget(), createCallbacks()) await conn.connect() @@ -389,20 +436,28 @@ describe('SshConnection', () => { const conn = new SshConnection(createTarget(), createCallbacks()) await conn.connect() const firstVerifier = ( - clientInstances[0].lastConnectConfig as { hostVerifier?: (key: Buffer) => boolean } + clientInstances[0].lastConnectConfig as { + hostVerifier?: (key: Buffer, verify: (ok: boolean) => void) => undefined + } ).hostVerifier const privateConn = conn as unknown as { attemptConnect: () => Promise } await privateConn.attemptConnect() const secondVerifier = ( - clientInstances[1].lastConnectConfig as { hostVerifier?: (key: Buffer) => boolean } + clientInstances[1].lastConnectConfig as { + hostVerifier?: (key: Buffer, verify: (ok: boolean) => void) => undefined + } ).hostVerifier expect(firstVerifier).toBeTypeOf('function') expect(secondVerifier).toBeTypeOf('function') - secondVerifier?.(Buffer.from('newer-ssh-host-key')) + const newerKey = Buffer.from( + 'AAAAC3NzaC1lZDI1NTE5AAAAILu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7', + 'base64' + ) + secondVerifier?.(newerKey, () => {}) const currentFingerprint = conn.getHostKeyFingerprint() - firstVerifier?.(Buffer.from('obsolete-ssh-host-key')) + firstVerifier?.(VALID_ED25519_HOST_KEY, () => {}) expect(conn.getHostKeyFingerprint()).toBe(currentFingerprint) }) diff --git a/src/main/ssh/ssh-connection.ts b/src/main/ssh/ssh-connection.ts index c8db921537f..93a7481efa9 100644 --- a/src/main/ssh/ssh-connection.ts +++ b/src/main/ssh/ssh-connection.ts @@ -1,6 +1,5 @@ /* eslint-disable max-lines -- Why: SSH connection lifecycle, credential retries, reconnect policy, and transport fallback are intentionally co-located so state transitions stay auditable in one file. */ import * as net from 'node:net' -import { createHash } from 'node:crypto' import { Client as SshClient } from 'ssh2' import type { ChildProcess } from 'node:child_process' import type { ClientChannel, ConnectConfig, SFTPWrapper } from 'ssh2' @@ -39,6 +38,24 @@ import { type SshConnectionCallbacks } from './ssh-connection-utils' import { resolveEffectiveProxy, spawnProxyCommand } from './ssh-proxy-command' +import { createHostKeyVerifier, orderServerHostKeyAlgorithms } from './ssh-host-key-verifier' +import { sshGArgsForHost } from './ssh-g-config-resolution' +// ssh2's own default order, which leads with ed25519 and places RSA fifth through seventh. +const DEFAULT_SERVER_HOST_KEY_ALGORITHMS = [ + 'ssh-ed25519', + 'ecdsa-sha2-nistp256', + 'ecdsa-sha2-nistp384', + 'ecdsa-sha2-nistp521', + 'rsa-sha2-512', + 'rsa-sha2-256', + 'ssh-rsa' +] +import { loadTrustedHostKeys, trustHostKey } from './ssh-host-key-store' +import { + loadKnownHostsEntries, + resolveKnownHostsFiles, + resolveKnownHostsLookupHost +} from './ssh-known-hosts-source' import { createCancelledConnectAttemptError, isCancelledConnectAttemptError @@ -83,7 +100,14 @@ function cloneResolvedConfig(config: SshResolvedConfig | null): SshResolvedConfi if (!config) { return null } - return { ...config, identityFile: [...config.identityFile] } + // Every array field needs its own copy — a shared reference lets one connection's clone mutate + // another's view of which files were consulted. + return { + ...config, + identityFile: [...config.identityFile], + userKnownHostsFiles: [...config.userKnownHostsFiles], + globalKnownHostsFiles: [...config.globalKnownHostsFiles] + } } function isGitHubRestrictedShellProbeSuccess( @@ -127,6 +151,11 @@ export class SshConnection { private systemCommandChannels = new Set() private systemOperationAbortController = new AbortController() private systemSshResolvedConfig: SshResolvedConfig | null = null + /** Set by attemptConnect so doSsh2Connect can build a verifier without threading it through. */ + private hostKeyResolvedConfig: SshResolvedConfig | null = null + /** Set when the verifier denies, so the connect rejects with the reason rather than a generic + * handshake failure the reconnect ladder would retry forever. */ + private hostKeyRejection: string | null = null private systemSshControlMasterDisabledForSession = false private systemSshGssapiOnlyForSession = false private useSystemSshTransport = false @@ -660,6 +689,8 @@ export class SshConnection { const resolved = await resolveWithSshG(this.target.configHost || this.target.label).catch( () => null ) + this.hostKeyResolvedConfig = resolved + this.hostKeyRejection = null const usesConfiguredSystemTransport = shouldUseSystemSshTransport(this.target, resolved) const requiresSecurityKeyTransport = usesConfiguredSystemTransport ? false @@ -1174,19 +1205,87 @@ export class SshConnection { config.sock = p.sock } - private doSsh2Connect(config: ConnectConfig, connectGeneration: number): Promise { + private async doSsh2Connect(config: ConnectConfig, connectGeneration: number): Promise { + const hostKeyResolved = this.hostKeyResolvedConfig + const hostKeyLookupHost = resolveKnownHostsLookupHost(hostKeyResolved, config.host ?? '') + // `-F` suppresses /etc/ssh/ssh_config, so a site-wide policy is invisible to us on that path. + const siteConfigSuppressed = sshGArgsForHost( + this.target.configHost || this.target.label + ).includes('-F') + const knownHostsEntries = await loadKnownHostsEntries(resolveKnownHostsFiles(hostKeyResolved)) + // Preloaded because ssh2's verifier decides synchronously; the same reason known_hosts is read + // here rather than inside the callback. + const trustedHostKeys = await loadTrustedHostKeys().catch((err) => { + // Losing our own records is not a security regression: known_hosts is still consulted, so a + // changed key is still refused. A host trusted only by us degrades to first contact and is + // re-recorded, which reaches the same decision. Loud because it means the store is unwired. + console.warn('[ssh] host key store unavailable; falling back to known_hosts only:', err) + return [] + }) + const serverHostKeyOrder = orderServerHostKeyAlgorithms( + knownHostsEntries, + hostKeyLookupHost, + config.port ?? 22, + DEFAULT_SERVER_HOST_KEY_ALGORITHMS + ) return new Promise((resolve, reject) => { const client = new SshClient() let settled = false - // Why: the relay uses the negotiated server key to isolate shared-home - // install locks without comparing PIDs from an unrelated SSH host. - config.hostVerifier = (key: Buffer): boolean => { - if (!this.disposed && connectGeneration === this.connectGeneration) { - const digest = createHash('sha256').update(key).digest('base64').replace(/=+$/, '') - this.hostKeyFingerprint = `SHA256:${digest}` + // Why the fingerprint is still recorded: the relay uses the negotiated server key to isolate + // shared-home install locks without comparing PIDs from an unrelated SSH host. Its format is + // load-bearing across versions, which is a second reason never to set ssh2's `hostHash`. + config.hostVerifier = createHostKeyVerifier({ + host: hostKeyLookupHost, + port: config.port ?? 22, + displayHost: this.target.label || hostKeyLookupHost, + strictHostKeyChecking: hostKeyResolved?.strictHostKeyChecking ?? 'ask', + isEphemeralRuntimeTarget: this.target.owner?.type === 'on-demand-runtime', + siteConfigSuppressed, + entries: knownHostsEntries, + isTrusted: ({ host, port, keyType, key }) => { + const forEndpoint = trustedHostKeys.filter( + (record) => + record.host === host.toLowerCase() && + record.port === port && + record.keyType === keyType + ) + if (forEndpoint.length === 0) { + return 'unknown' + } + return forEndpoint.some((record) => Buffer.from(record.key, 'base64').equals(key)) + ? 'match' + : 'mismatch' + }, + rememberHostKey: (record) => { + void trustHostKey({ + host: record.host, + port: record.port, + keyType: record.keyType, + key: record.key + }).catch((err) => { + // A failed write must not fail the connection: we verified the key, we just could not + // record it, so the next connect re-derives the same answer from known_hosts. + console.warn('[ssh] failed to record accepted host key:', err) + }) + }, + onDecision: (decision) => { + if (!this.disposed && connectGeneration === this.connectGeneration) { + this.hostKeyFingerprint = decision.fingerprint + } + if (decision.action === 'reject') { + this.hostKeyRejection = decision.reason ?? 'Host key verification failed.' + } + } + }) + + if (serverHostKeyOrder) { + config.algorithms = { + ...config.algorithms, + serverHostKey: serverHostKeyOrder as NonNullable< + NonNullable['serverHostKey'] + > } - return true } const cleanupStartupListeners = (): void => { @@ -1239,7 +1338,10 @@ export class SshConnection { cleanupStartupListeners() settled = true client.destroy() - reject(err) + // Why replace the message: ssh2 reports a denied host key as a generic handshake failure, + // which the reconnect ladder cannot distinguish from a transient fault and would retry + // forever against a decision that will never change. + reject(this.hostKeyRejection ? new Error(this.hostKeyRejection) : err) } client.on('ready', onReady) diff --git a/src/main/ssh/ssh-host-key-store.test.ts b/src/main/ssh/ssh-host-key-store.test.ts index cefc449f06f..4da65f0ae88 100644 --- a/src/main/ssh/ssh-host-key-store.test.ts +++ b/src/main/ssh/ssh-host-key-store.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { mkdtemp, readdir, readFile, rm, stat, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -209,10 +209,24 @@ describe('ssh host key store', () => { expect(JSON.parse(await readFile(storeFile, 'utf-8'))).toMatchObject({ version: 1 }) - const { readdir } = await import('node:fs/promises') expect((await readdir(directory)).filter((name) => name.endsWith('.tmp'))).toEqual([]) }) + it('publishes every update by replacing the file, never by rewriting it in place', async () => { + await trustHostKey(query(), storeFile) + const before = await stat(storeFile) + + await trustHostKey(query({ keyType: 'ssh-rsa', key: RSA_A }), storeFile) + + // A temp-file + rename swaps the inode; an in-place write truncates and keeps it, which is the + // shape that can leave a half-written trust list readable after a crash. + const after = await stat(storeFile) + if (before.ino !== 0 && after.ino !== 0) { + expect(after.ino).not.toBe(before.ino) + } + expect(await isTrusted(query(), storeFile)).toBe('match') + }) + it('keeps both accepts when two hosts are trusted concurrently', async () => { await Promise.all([ trustHostKey(query({ host: 'host-a' }), storeFile),