From 11b2f93cf8d5891cf360b3ef807c458776f78c66 Mon Sep 17 00:00:00 2001
From: Neil <4138956+nwparker@users.noreply.github.com>
Date: Fri, 14 Aug 2026 03:19:03 -0700
Subject: [PATCH] fix(ssh): close two downgrades the implementation review
 found
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Both were in my wiring, not the design, and two reviewers found the first
independently.

1. OUR STORE WAS TYPE-DOWNGRADABLE. The inline lookup filtered by key type first
and could only answer match/mismatch/unknown, so a record of a DIFFERENT type for
the same endpoint read as "unknown". A host learned on first contact — ed25519,
since ssh2 proposes it first — and absent from known_hosts could then be
impersonated by presenting RSA: both sources say unknown, so accept-and-remember,
silently. That is exactly the downgrade D3 says the design cannot ship without,
applied to the records we create ourselves. The store's own isTrusted already
computed the right answer and had no production caller. Stored types now also
feed the algorithm ordering, without which the guard is only half present.

2. WE KEYED ON THE ORCA LABEL, NOT THE DIALED HOST. "ssh -G" echoes its own
argument back as its hostname field when no Host block matches, so for a manual
target that field IS the Orca label — the one name D2 forbids keying on, and one
ssh never wrote. We consulted no entries at all, so an impersonated host read as
first contact. Now keys on the dialed host, which buildConnectConfig has already
resolved through HostName, with HostKeyAlias still winning.

That inverted an existing test rather than deleting it: "uses the resolved
hostname, never the Orca label" encoded an assumption disproved against OpenSSH
10.2p1, so it is renamed and reversed with the reason recorded in the test.

3. NO READABLE SOURCE IS NOT FIRST CONTACT. Every known_hosts file failing to
read was indistinguishable from "this host is unknown", so a changed key would be
accepted the one time we could not check. The loader now reports how many files
it could read, and zero readable sources with an empty store takes the strict
path instead of recording trust.

4. A superseded attempt's rejection could replace the live attempt's error, and
substituting a new Error drops ssh2's code, so a transient ECONNRESET would stop
being classified as retryable. The rejection is now local to its attempt.

5. displayHost was the Orca label, so a mismatch could print
"ssh-keygen -R <label>" — a remedy that removes nothing.

Also adds the tests that would have caught 1 and 2, the stale-attempt denial, and
IPv6 literals, which the design moved into scope and nothing covered.

1,469 SSH tests pass.
---
 src/main/ssh/ssh-connection.ts              | 62 ++++++++++++++-------
 src/main/ssh/ssh-host-key-decision.test.ts  |  7 +++
 src/main/ssh/ssh-host-key-decision.ts       | 10 +++-
 src/main/ssh/ssh-host-key-verifier.test.ts  | 21 +++++++
 src/main/ssh/ssh-host-key-verifier.ts       | 20 ++++++-
 src/main/ssh/ssh-known-hosts-source.test.ts | 20 ++++++-
 src/main/ssh/ssh-known-hosts-source.ts      | 54 +++++++++++++-----
 src/main/ssh/ssh-known-hosts.test.ts        | 19 +++++++
 8 files changed, 168 insertions(+), 45 deletions(-)

diff --git a/src/main/ssh/ssh-connection.ts b/src/main/ssh/ssh-connection.ts
index 93a7481efa9..cb0bcf4544a 100644
--- a/src/main/ssh/ssh-connection.ts
+++ b/src/main/ssh/ssh-connection.ts
@@ -52,7 +52,7 @@ const DEFAULT_SERVER_HOST_KEY_ALGORITHMS = [
 ]
 import { loadTrustedHostKeys, trustHostKey } from './ssh-host-key-store'
 import {
-  loadKnownHostsEntries,
+  loadKnownHostsEvidence,
   resolveKnownHostsFiles,
   resolveKnownHostsLookupHost
 } from './ssh-known-hosts-source'
@@ -153,9 +153,6 @@ export class SshConnection {
   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
@@ -690,7 +687,6 @@ export class SshConnection {
       () => null
     )
     this.hostKeyResolvedConfig = resolved
-    this.hostKeyRejection = null
     const usesConfiguredSystemTransport = shouldUseSystemSshTransport(this.target, resolved)
     const requiresSecurityKeyTransport = usesConfiguredSystemTransport
       ? false
@@ -1212,7 +1208,8 @@ export class SshConnection {
     const siteConfigSuppressed = sshGArgsForHost(
       this.target.configHost || this.target.label
     ).includes('-F')
-    const knownHostsEntries = await loadKnownHostsEntries(resolveKnownHostsFiles(hostKeyResolved))
+    const knownHostsEvidence = await loadKnownHostsEvidence(resolveKnownHostsFiles(hostKeyResolved))
+    const knownHostsEntries = knownHostsEvidence.entries
     // 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) => {
@@ -1222,15 +1219,26 @@ export class SshConnection {
       console.warn('[ssh] host key store unavailable; falling back to known_hosts only:', err)
       return []
     })
+    const storedKeyTypesForEndpoint = trustedHostKeys
+      .filter(
+        (record) =>
+          record.host === hostKeyLookupHost.toLowerCase() && record.port === (config.port ?? 22)
+      )
+      .map((record) => record.keyType)
     const serverHostKeyOrder = orderServerHostKeyAlgorithms(
       knownHostsEntries,
       hostKeyLookupHost,
       config.port ?? 22,
-      DEFAULT_SERVER_HOST_KEY_ALGORITHMS
+      DEFAULT_SERVER_HOST_KEY_ALGORITHMS,
+      storedKeyTypesForEndpoint
     )
     return new Promise<void>((resolve, reject) => {
       const client = new SshClient()
       let settled = false
+      // Local to this attempt: an instance field would let a superseded attempt's rejection replace
+      // the live attempt's error, and substituting a new Error drops ssh2's `code`, so a transient
+      // ECONNRESET would stop being classified as retryable.
+      let hostKeyRejection: string | null = null
 
       // 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
@@ -1238,24 +1246,35 @@ export class SshConnection {
       config.hostVerifier = createHostKeyVerifier({
         host: hostKeyLookupHost,
         port: config.port ?? 22,
-        displayHost: this.target.label || hostKeyLookupHost,
+        // The name we looked up, not the label: the mismatch message prints `ssh-keygen -R <host>`
+        // and a label removes nothing from known_hosts.
+        displayHost: hostKeyLookupHost,
         strictHostKeyChecking: hostKeyResolved?.strictHostKeyChecking ?? 'ask',
         isEphemeralRuntimeTarget: this.target.owner?.type === 'on-demand-runtime',
-        siteConfigSuppressed,
+        // No readable source at all is not evidence of a new host — it is the absence of evidence,
+        // and recording trust from it would let a changed key be accepted the one time we could not
+        // check. Reuses the strict path rather than adding a fifth outcome.
+        siteConfigSuppressed:
+          siteConfigSuppressed ||
+          (knownHostsEvidence.configuredFileCount > 0 &&
+            knownHostsEvidence.readableFileCount === 0 &&
+            trustedHostKeys.length === 0),
         entries: knownHostsEntries,
         isTrusted: ({ host, port, keyType, key }) => {
-          const forEndpoint = trustedHostKeys.filter(
-            (record) =>
-              record.host === host.toLowerCase() &&
-              record.port === port &&
-              record.keyType === keyType
+          const forHost = trustedHostKeys.filter(
+            (record) => record.host === host.toLowerCase() && record.port === port
           )
-          if (forEndpoint.length === 0) {
-            return 'unknown'
+          const sameType = forHost.filter((record) => record.keyType === keyType)
+          if (sameType.length > 0) {
+            return sameType.some((record) => Buffer.from(record.key, 'base64').equals(key))
+              ? 'match'
+              : 'mismatch'
           }
-          return forEndpoint.some((record) => Buffer.from(record.key, 'base64').equals(key))
-            ? 'match'
-            : 'mismatch'
+          // We hold a key for this endpoint, just not of the presented type. Reporting plain
+          // `unknown` here would let an attacker who cannot forge the type we recorded on first
+          // contact present another and be trusted-and-recorded silently — the same downgrade the
+          // known_hosts path already refuses.
+          return forHost.length > 0 ? 'unknown-type-known-host' : 'unknown'
         },
         rememberHostKey: (record) => {
           void trustHostKey({
@@ -1269,12 +1288,13 @@ export class SshConnection {
             console.warn('[ssh] failed to record accepted host key:', err)
           })
         },
+        isCurrentAttempt: () => !this.disposed && connectGeneration === this.connectGeneration,
         onDecision: (decision) => {
           if (!this.disposed && connectGeneration === this.connectGeneration) {
             this.hostKeyFingerprint = decision.fingerprint
           }
           if (decision.action === 'reject') {
-            this.hostKeyRejection = decision.reason ?? 'Host key verification failed.'
+            hostKeyRejection = decision.reason ?? 'Host key verification failed.'
           }
         }
       })
@@ -1341,7 +1361,7 @@ export class SshConnection {
         // 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)
+        reject(hostKeyRejection ? new Error(hostKeyRejection) : err)
       }
 
       client.on('ready', onReady)
diff --git a/src/main/ssh/ssh-host-key-decision.test.ts b/src/main/ssh/ssh-host-key-decision.test.ts
index 842f1bd7596..323449aa366 100644
--- a/src/main/ssh/ssh-host-key-decision.test.ts
+++ b/src/main/ssh/ssh-host-key-decision.test.ts
@@ -33,6 +33,13 @@ describe('deciding what to do with a presented host key', () => {
         'an unfamiliar key type for a host we know',
         { knownHostsOutcome: 'unknown-type-known-host' as const },
         'unknown-type-known-host'
+      ],
+      // Same downgrade, our own records. Without this the guard covers known_hosts only, so a key
+      // we learned on first contact could be sidestepped by presenting another type.
+      [
+        'an unfamiliar key type for a host only we know',
+        { storeOutcome: 'unknown-type-known-host' as const },
+        'unknown-type-known-host'
       ]
     ])('rejects %s', (_label, overrides, outcome) => {
       const decision = decideHostKey(input(overrides))
diff --git a/src/main/ssh/ssh-host-key-decision.ts b/src/main/ssh/ssh-host-key-decision.ts
index 965e9ea7510..dee0d486b5d 100644
--- a/src/main/ssh/ssh-host-key-decision.ts
+++ b/src/main/ssh/ssh-host-key-decision.ts
@@ -23,7 +23,7 @@ export type HostKeyDecisionInput = {
   /** From the user's known_hosts files, unioned. */
   knownHostsOutcome: KnownHostsOutcome
   /** From our own store: does it already hold this exact key for host+port+type? */
-  storeOutcome: 'match' | 'mismatch' | 'unknown'
+  storeOutcome: 'match' | 'mismatch' | 'unknown-type-known-host' | 'unknown'
   /** Effective `StrictHostKeyChecking`; anything unrecognised is treated as `ask`. */
   strictHostKeyChecking: string
   /**
@@ -115,11 +115,15 @@ export function decideHostKey(input: HostKeyDecisionInput): HostKeyDecision {
 
   // We hold a key for this host, just not of the presented type. Treating that as first contact is
   // the downgrade an attacker who cannot forge the known key would reach for.
-  if (knownHostsOutcome === 'unknown-type-known-host') {
+  if (
+    knownHostsOutcome === 'unknown-type-known-host' ||
+    storeOutcome === 'unknown-type-known-host'
+  ) {
     return {
       action: 'reject',
       outcome: 'unknown-type-known-host',
-      disagreeingSource: 'known-hosts',
+      disagreeingSource:
+        knownHostsOutcome === 'unknown-type-known-host' ? 'known-hosts' : 'orca-store',
       reason: rejection(
         displayHost,
         'The host offered a key of a type we have not seen for it before, while a key of another type is already known. This can mean the host was rebuilt, or that something is impersonating it.'
diff --git a/src/main/ssh/ssh-host-key-verifier.test.ts b/src/main/ssh/ssh-host-key-verifier.test.ts
index 0b8cc488c80..3efbdac073d 100644
--- a/src/main/ssh/ssh-host-key-verifier.test.ts
+++ b/src/main/ssh/ssh-host-key-verifier.test.ts
@@ -89,6 +89,19 @@ describe('the ssh2 host key verifier', () => {
     expect(returned).toBeUndefined()
   })
 
+  // A superseded attempt has nobody waiting on it; accepting would record trust for a connection
+  // that no longer exists.
+  it('denies once its connect attempt has been superseded', () => {
+    const { accepted } = run({ isCurrentAttempt: () => false })
+    expect(accepted).toBe(false)
+  })
+
+  it('does not record a key for a superseded attempt', () => {
+    const rememberHostKey = vi.fn()
+    run({ isCurrentAttempt: () => false, rememberHostKey })
+    expect(rememberHostKey).not.toHaveBeenCalled()
+  })
+
   describe('remembering', () => {
     it('records a first-contact key', () => {
       const rememberHostKey = vi.fn()
@@ -162,6 +175,14 @@ describe('host key algorithm ordering', () => {
     expect(ordered?.indexOf('ssh-ed25519')).toBeGreaterThan(1)
   })
 
+  // Types we recorded ourselves must be promoted too, or a host known only to us is left open to
+  // the same downgrade the known_hosts path is protected against.
+  it('leads with a type known only from our own store', () => {
+    const ordered = orderServerHostKeyAlgorithms([], 'example.com', 22, supported, ['ssh-rsa'])
+    expect(ordered?.[0]).toMatch(/rsa/)
+    expect(ordered?.indexOf('ssh-ed25519')).toBeGreaterThan(0)
+  })
+
   it('does not propose a type the transport does not support', () => {
     const entries = parseKnownHosts(`example.com ssh-ed25519 ${ED_A}`)
     const ordered = orderServerHostKeyAlgorithms(entries, 'example.com', 22, ['rsa-sha2-512'])
diff --git a/src/main/ssh/ssh-host-key-verifier.ts b/src/main/ssh/ssh-host-key-verifier.ts
index 848860ac020..cd35518bfaf 100644
--- a/src/main/ssh/ssh-host-key-verifier.ts
+++ b/src/main/ssh/ssh-host-key-verifier.ts
@@ -21,7 +21,7 @@ export type TrustedHostKeyLookup = (query: {
   port: number
   keyType: string
   key: Buffer
-}) => 'match' | 'mismatch' | 'unknown'
+}) => 'match' | 'mismatch' | 'unknown-type-known-host' | 'unknown'
 
 export type HostKeyVerifierDeps = {
   host: string
@@ -42,6 +42,12 @@ export type HostKeyVerifierDeps = {
   }) => void
   /** Called on every decision so accepts and rejections are auditable. */
   onDecision?: (decision: HostKeyDecision & { fingerprint: string; keyType: string }) => void
+  /**
+   * False once this connect attempt has been superseded or disposed. A late verifier must deny
+   * rather than accept: the attempt it belongs to is gone, so nothing will consume the result, and
+   * accepting would record trust on behalf of a connection nobody is waiting for.
+   */
+  isCurrentAttempt?: () => boolean
 }
 
 export function hostKeyFingerprintOf(key: Buffer): string {
@@ -62,9 +68,13 @@ export function orderServerHostKeyAlgorithms(
   entries: readonly KnownHostsEntry[],
   host: string,
   port: number,
-  supported: readonly string[]
+  supported: readonly string[],
+  /** Types we recorded ourselves. Without these, a host known only to us is never promoted, and
+   *  type scoping degrades into the downgrade it exists to prevent for exactly the hosts we
+   *  learned on first contact. */
+  storedKeyTypes: readonly string[] = []
 ): string[] | undefined {
-  const known = new Set<string>()
+  const known = new Set<string>(storedKeyTypes)
   for (const entry of entries) {
     if (entry.marker === 'revoked') {
       continue
@@ -114,6 +124,10 @@ export function createHostKeyVerifier(
 ): (key: Buffer, verify: VerifyCallback) => undefined {
   return (key, verify) => {
     try {
+      if (deps.isCurrentAttempt && !deps.isCurrentAttempt()) {
+        verify(false)
+        return undefined
+      }
       const keyType = readHostKeyType(key)
       if (!keyType) {
         // A key whose own header we cannot read is not something to reason about further.
diff --git a/src/main/ssh/ssh-known-hosts-source.test.ts b/src/main/ssh/ssh-known-hosts-source.test.ts
index 5c3335af852..68da1217b52 100644
--- a/src/main/ssh/ssh-known-hosts-source.test.ts
+++ b/src/main/ssh/ssh-known-hosts-source.test.ts
@@ -223,10 +223,24 @@ describe('resolveKnownHostsLookupHost', () => {
     expect(resolveKnownHostsLookupHost(resolved, '127.0.0.1')).toBe('bastion')
   })
 
-  it('uses the resolved hostname, never the Orca label', () => {
-    const resolved = resolvedConfig({ hostname: 'prod.internal' })
+  // INVERTED from 'uses the resolved hostname, never the Orca label'. That test encoded an
+  // assumption verified false against OpenSSH 10.2p1: `ssh -G` echoes its own argument back as
+  // `hostname` when no Host block matches, so for a manual target `resolved.hostname` IS the Orca
+  // label — the one name the design forbids keying on, and one `ssh` never wrote. Keying on it
+  // consults no entries at all, so an impersonated host reads as first contact.
+  //
+  // The dialed host is correct in both cases: buildConnectConfig has already applied HostName
+  // resolution, so a config alias dials the real host too.
+  it('uses the dialed host, because a resolved hostname can just echo the label', () => {
+    const resolved = resolvedConfig({ hostname: 'my-orca-label' })
 
-    expect(resolveKnownHostsLookupHost(resolved, 'my-orca-label')).toBe('prod.internal')
+    expect(resolveKnownHostsLookupHost(resolved, '10.0.0.5')).toBe('10.0.0.5')
+  })
+
+  it('still prefers an explicit HostKeyAlias over the dialed host', () => {
+    const resolved = resolvedConfig({ hostname: 'my-orca-label', hostKeyAlias: 'bastion' })
+
+    expect(resolveKnownHostsLookupHost(resolved, '10.0.0.5')).toBe('bastion')
   })
 
   it('falls back to the dialed host when nothing was resolved', () => {
diff --git a/src/main/ssh/ssh-known-hosts-source.ts b/src/main/ssh/ssh-known-hosts-source.ts
index 091d79b21bd..9bc1c58ca3b 100644
--- a/src/main/ssh/ssh-known-hosts-source.ts
+++ b/src/main/ssh/ssh-known-hosts-source.ts
@@ -32,25 +32,44 @@ export function resolveKnownHostsFiles(resolved: SshResolvedConfig | null): stri
   return [...new Set(reported.filter((path) => path !== EXPLICIT_NONE))]
 }
 
-async function readEntries(path: string): Promise<KnownHostsEntry[]> {
-  try {
-    return parseKnownHosts(await readFile(path, 'utf8'))
-  } catch {
-    // Missing, unreadable or a directory. Skipping is not failing open: a file we cannot read
-    // contributes no trust, and the remaining files still decide.
-    return []
-  }
-}
-
 /**
  * The union across every file. The caller runs `matchKnownHosts` once over the result, so an exact
  * hit in any file wins and a disagreeing entry in another file is not a mismatch.
  */
-export async function loadKnownHostsEntries(
+export async function loadKnownHostsEntries(files: readonly string[]): Promise<KnownHostsEntry[]> {
+  return (await loadKnownHostsEvidence(files)).entries
+}
+
+export type KnownHostsEvidence = {
+  entries: KnownHostsEntry[]
+  /**
+   * How many configured files we could actually read. Zero is NOT the same as "this host is
+   * unknown": if every source is unreadable we have no evidence at all, and treating that as first
+   * contact would silently accept a changed key. The caller must refuse to record trust then.
+   */
+  readableFileCount: number
+  configuredFileCount: number
+}
+
+export async function loadKnownHostsEvidence(
   files: readonly string[]
-): Promise<KnownHostsEntry[]> {
-  const perFile = await Promise.all(files.map((path) => readEntries(path)))
-  return perFile.flat()
+): Promise<KnownHostsEvidence> {
+  const perFile = await Promise.all(
+    files.map(async (path) => {
+      try {
+        return { entries: parseKnownHosts(await readFile(path, 'utf8')), readable: true }
+      } catch {
+        // Missing, unreadable or a directory. A file we cannot read contributes no trust; the
+        // remaining files still decide, and the count tells the caller whether any spoke at all.
+        return { entries: [] as KnownHostsEntry[], readable: false }
+      }
+    })
+  )
+  return {
+    entries: perFile.flatMap((file) => file.entries),
+    readableFileCount: perFile.filter((file) => file.readable).length,
+    configuredFileCount: files.length
+  }
 }
 
 /**
@@ -62,5 +81,10 @@ export function resolveKnownHostsLookupHost(
   resolved: SshResolvedConfig | null,
   dialedHost: string
 ): string {
-  return resolved?.hostKeyAlias || resolved?.hostname || dialedHost
+  // Deliberately NOT `resolved.hostname`: `ssh -G` echoes its own argument back as `hostname` when
+  // no Host block matches, which for a manual target is the Orca label. Keying on that consults a
+  // name `ssh` never wrote, so the real host's entries are missed entirely and an impersonated
+  // host reads as first contact. `dialedHost` is what ssh2 actually connects to, with HostName
+  // resolution already applied by buildConnectConfig.
+  return resolved?.hostKeyAlias || dialedHost
 }
diff --git a/src/main/ssh/ssh-known-hosts.test.ts b/src/main/ssh/ssh-known-hosts.test.ts
index de9e4c6ea7a..7fcb94e66a3 100644
--- a/src/main/ssh/ssh-known-hosts.test.ts
+++ b/src/main/ssh/ssh-known-hosts.test.ts
@@ -128,6 +128,25 @@ describe('matching a presented key', () => {
     })
   })
 
+  // The design moved IPv6 into scope: a literal that comes back `unknown` because we mangled the
+  // brackets is prompt-training, the harm the whole outcome vocabulary exists to avoid.
+  describe('IPv6 literals', () => {
+    it('matches a bare literal on the default port', () => {
+      const contents = line('2001:db8::1', ED_A)
+      expect(verdict(contents, { host: '2001:db8::1', key: ED_A })).toBe('match')
+    })
+
+    it('matches a bracketed literal on its port', () => {
+      const contents = line('[2001:db8::1]:2222', ED_A)
+      expect(verdict(contents, { host: '2001:db8::1', port: 2222, key: ED_A })).toBe('match')
+    })
+
+    it('reports a change for a literal', () => {
+      const contents = line('2001:db8::1', ED_A)
+      expect(verdict(contents, { host: '2001:db8::1', key: ED_B })).toBe('mismatch')
+    })
+  })
+
   describe('patterns', () => {
     it.each([
       ['a star glob', line('*.example.com', ED_A), 'host.example.com', 'match'],