fix(ssh): verify host keys instead of accepting every one (STA-4319)

The actual fix. ssh-connection's verifier recorded a fingerprint and returned
true, so every ssh2 connection accepted every host key — no known_hosts consult,
no change detection. It now consults the user's known_hosts plus our own store
and refuses a changed, revoked or unverifiable key.

Phase 1 by design: no dialog. Unknown hosts are accepted and recorded
(accept-new semantics), because startup restore opens every previously-active
target at once, ephemeral VM targets present a new key each launch, and
paired-web connects run on the host desktop where a prompt would appear on
someone else's screen. The MITM defence lands now; the prompt is Phase 2.

Also sets algorithms.serverHostKey to lead with the types already known for the
host. Without it the type-scoped matching is a downgrade — an attacker who cannot
forge the key on file just presents another type and turns a hard failure into
first contact. Verified from ssh2's kex.js that the client list decides.

Denial replaces ssh2's generic handshake error with the specific reason, because
the reconnect ladder cannot distinguish a generic failure from a transient fault
and would retry forever against a decision that will never change.

An unreadable trust store degrades to known_hosts only rather than failing the
connect: a changed key is still refused, and a host trusted only by us falls back
to first contact and is re-recorded, reaching the same decision.

The ssh2 mock now uses the callback form and aborts the handshake on denial. As
written it called hostVerifier(key) with one argument and ignored the result, so
it would have passed against a verifier that never decides — flagged in the
design as a mock that had to change, not a test to quietly rewrite. Two new tests
pin the wiring rather than the module: an unidentifiable blob is refused, and a
well-formed key is accepted.

Note for review: commit 2d2a0880ba unintentionally swept in two modules built
concurrently (ssh-known-hosts-source, ssh-host-key-store) because I staged with
'git add -A'; its message describes only the verifier. Both are covered by their
own tests, but the attribution in that commit is wrong.

1461 SSH tests pass.

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<void> }
     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)
   })

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<ClientChannel>()
   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<void> {
+  private async doSsh2Connect(config: ConnectConfig, connectGeneration: number): Promise<void> {
+    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<void>((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<ConnectConfig['algorithms']>['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)

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),