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((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 ` + // 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() + const known = new Set(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 { - 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 { + 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 { - const perFile = await Promise.all(files.map((path) => readEntries(path))) - return perFile.flat() +): Promise { + 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'],