diff --git a/src/main/ssh/ssh-connection.ts b/src/main/ssh/ssh-connection.ts index 42d64714154..2604614b31d 100644 --- a/src/main/ssh/ssh-connection.ts +++ b/src/main/ssh/ssh-connection.ts @@ -38,19 +38,13 @@ import { type SshConnectionCallbacks } from './ssh-connection-utils' import { resolveEffectiveProxy, spawnProxyCommand } from './ssh-proxy-command' -import { createHostKeyVerifier, orderServerHostKeyAlgorithms } from './ssh-host-key-verifier' +import { + createHostKeyVerifier, + DEFAULT_SERVER_HOST_KEY_ALGORITHMS, + orderServerHostKeyAlgorithms +} from './ssh-host-key-verifier' import { HostKeyVerificationError, isHostKeyVerificationError } from './ssh-host-key-decision' 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 { loadKnownHostsEvidence, @@ -1219,7 +1213,7 @@ export class SshConnection { 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( + const verificationSourcesIncomplete = sshGArgsForHost( this.target.configHost || this.target.label ).includes('-F') const knownHostsEvidence = await loadKnownHostsEvidence(resolveKnownHostsFiles(hostKeyResolved)) @@ -1270,7 +1264,8 @@ export class SshConnection { // read, so recording trust would accept a changed key the one time we could not check. // Reuses the strict path rather than adding a fifth outcome. An ABSENT file is not this — // that is the normal state for a fresh profile and genuinely means nothing is known. - siteConfigSuppressed: siteConfigSuppressed || knownHostsEvidence.unreadableFileCount > 0, + verificationSourcesIncomplete: + verificationSourcesIncomplete || knownHostsEvidence.unreadableFileCount > 0, entries: knownHostsEntries, isTrusted: ({ host, port, keyType, key }) => { const forHost = trustedHostKeys.filter( diff --git a/src/main/ssh/ssh-host-key-decision.test.ts b/src/main/ssh/ssh-host-key-decision.test.ts index 323449aa366..ce6bc350d68 100644 --- a/src/main/ssh/ssh-host-key-decision.test.ts +++ b/src/main/ssh/ssh-host-key-decision.test.ts @@ -7,7 +7,7 @@ function input(overrides: Partial = {}): HostKeyDecisionIn storeOutcome: 'unknown', strictHostKeyChecking: 'ask', isEphemeralRuntimeTarget: false, - siteConfigSuppressed: false, + verificationSourcesIncomplete: false, displayHost: 'build-01', ...overrides } @@ -129,18 +129,52 @@ describe('deciding what to do with a presented host key', () => { expect(decision.action).toBe('reject') }) - // We could not read the system ssh_config, so a site-wide policy may forbid this and we cannot - // see it. Being laxer than ssh is the one outcome that is never acceptable. - it('denies an unknown host when the system SSH config could not be read', () => { - expect(decideHostKey(input({ siteConfigSuppressed: true })).action).toBe('reject') + // We could not read something that decides this — the system ssh_config, or a known_hosts file + // that exists and will not open. Being laxer than ssh is the one outcome that is never + // acceptable. + it('denies an unknown host when a source could not be read', () => { + expect(decideHostKey(input({ verificationSourcesIncomplete: true })).action).toBe('reject') }) - it('still accepts a known host when the system SSH config could not be read', () => { + // Only NEW trust is withheld. A host we already know is decided before this is reached, so + // being unable to read one source does not disconnect everything the user already verified. + it('still accepts a known host when a source could not be read', () => { const decision = decideHostKey( - input({ siteConfigSuppressed: true, knownHostsOutcome: 'match' }) + input({ verificationSourcesIncomplete: true, knownHostsOutcome: 'match' }) ) expect(decision.action).toBe('accept') }) + + // A VM provisioned a minute ago cannot be in known_hosts, so no policy — seen or unseen — is + // satisfiable by it. Refusing would not make the connection safer, it would turn on-demand + // runtimes off for everyone whose HOME diverges from their passwd home. + it('still accepts an ephemeral target when a source could not be read', () => { + const decision = decideHostKey( + input({ isEphemeralRuntimeTarget: true, verificationSourcesIncomplete: true }) + ) + expect(decision.action).toBe('accept') + }) + + // The exception to the exception: an explicit StrictHostKeyChecking=yes is a policy we can + // actually read and the user actually asked for, so it outranks the carve-out. + it('denies an ephemeral target under an explicit StrictHostKeyChecking=yes', () => { + const decision = decideHostKey( + input({ isEphemeralRuntimeTarget: true, strictHostKeyChecking: 'yes' }) + ) + expect(decision.action).toBe('reject') + }) + + // The carve-out is about first contact only; a key that CHANGED is still a change. + it('still rejects a changed key for an ephemeral target with sources incomplete', () => { + const decision = decideHostKey( + input({ + isEphemeralRuntimeTarget: true, + verificationSourcesIncomplete: true, + storeOutcome: 'mismatch' + }) + ) + expect(decision.action).toBe('reject') + }) }) // Phase 1 ships no dialog at all: startup restore opens many connections at once, ephemeral @@ -158,7 +192,7 @@ describe('deciding what to do with a presented host key', () => { { strictHostKeyChecking: 'yes' }, { strictHostKeyChecking: 'no' }, { isEphemeralRuntimeTarget: true }, - { siteConfigSuppressed: true } + { verificationSourcesIncomplete: true } ] for (const overrides of cases) { expect(decideHostKey(input(overrides)).action).not.toBe('prompt') diff --git a/src/main/ssh/ssh-host-key-decision.ts b/src/main/ssh/ssh-host-key-decision.ts index aafb18cce6d..ca3429f7d7b 100644 --- a/src/main/ssh/ssh-host-key-decision.ts +++ b/src/main/ssh/ssh-host-key-decision.ts @@ -32,10 +32,13 @@ export type HostKeyDecisionInput = { */ isEphemeralRuntimeTarget: boolean /** - * `ssh -G` ran on the HOME-divergent `-F` path, so /etc/ssh/ssh_config was suppressed and a - * site-wide StrictHostKeyChecking may be invisible to us. Never be laxer than ssh would be. + * Something that decides this was unreadable, so we cannot prove we are seeing what ssh sees: + * `ssh -G` ran on the HOME-divergent `-F` path and suppressed a possible site-wide + * StrictHostKeyChecking, or a known_hosts file exists and would not open. Extending NEW trust + * while blind is the one outcome that is never acceptable — a host we already know still + * connects, because a match is decided before this is consulted. */ - siteConfigSuppressed: boolean + verificationSourcesIncomplete: boolean displayHost: string } @@ -78,7 +81,7 @@ export function decideHostKey(input: HostKeyDecisionInput): HostKeyDecision { knownHostsOutcome, storeOutcome, isEphemeralRuntimeTarget, - siteConfigSuppressed, + verificationSourcesIncomplete, displayHost } = input const strict = input.strictHostKeyChecking.toLowerCase() @@ -164,23 +167,35 @@ export function decideHostKey(input: HostKeyDecisionInput): HostKeyDecision { ) } } - if (siteConfigSuppressed) { - // We could not read the system ssh_config, so we cannot prove a site policy does not forbid - // this. Failing strict here is the only way to avoid being laxer than ssh. + // Deliberately ABOVE the incomplete-sources check, and deliberately BELOW explicit strict. + // + // A machine provisioned a minute ago cannot be in known_hosts, by construction — no policy, seen + // or unseen, can be satisfied by it. So refusing here would not make the connection safer, it + // would turn on-demand runtimes off entirely for anyone whose HOME diverges from their passwd + // home (sandboxes, E2E isolation), and the reason we would print names a config file they cannot + // fix. Incomplete sources is a statement that we might be missing a policy; it is not a policy. + // An EXPLICIT StrictHostKeyChecking=yes still wins above, because that one we can actually read + // and the user asked for it. + // + // Accepting without recording is the rest of it: a new key every launch would otherwise grow a + // row per VM, and a stale row eventually reads as a mismatch against a host that did nothing + // wrong. + if (isEphemeralRuntimeTarget) { + return { action: 'accept', outcome: 'unknown' } + } + if (verificationSourcesIncomplete) { + // We could not read something that decides this, so we cannot prove a policy does not forbid it + // or that a known_hosts entry does not contradict it. Refusing to extend NEW trust while blind + // is the only way to avoid being laxer than ssh. return { action: 'reject', outcome: 'unknown', reason: rejection( displayHost, - 'The host is unknown and the system SSH configuration could not be read, so its host key policy is unknown.' + 'The host is unknown and part of the SSH configuration could not be read, so its host key policy cannot be checked.' ) } } - if (isEphemeralRuntimeTarget) { - // Expected to differ every launch; accepting without recording keeps the store from growing a - // row per VM and keeps a stale record from ever becoming a spurious mismatch. - return { action: 'accept', outcome: 'unknown' } - } if (strict === 'no' || strict === 'off') { // OpenSSH accepts here but does not write. Persisting would silently convert a deliberately // lax setting into a permanent trust record. diff --git a/src/main/ssh/ssh-host-key-verifier.test.ts b/src/main/ssh/ssh-host-key-verifier.test.ts index 43aa3cbe26a..748b8b31251 100644 --- a/src/main/ssh/ssh-host-key-verifier.test.ts +++ b/src/main/ssh/ssh-host-key-verifier.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import { parseKnownHosts } from './ssh-known-hosts' import { createHostKeyVerifier, + DEFAULT_SERVER_HOST_KEY_ALGORITHMS, hostKeyFingerprintOf, orderServerHostKeyAlgorithms, type HostKeyVerifierDeps @@ -21,7 +22,7 @@ function deps(overrides: Partial = {}): HostKeyVerifierDeps displayHost: 'example.com', strictHostKeyChecking: 'ask', isEphemeralRuntimeTarget: false, - siteConfigSuppressed: false, + verificationSourcesIncomplete: false, entries: [], isTrusted: () => 'unknown', rememberHostKey: vi.fn(), @@ -199,6 +200,37 @@ describe('the ssh2 host key verifier', () => { }) }) +// The list we reorder is a hand-copy of ssh2's internal default. If ssh2 changes it, our proposal +// silently stops matching what ssh2 supports — and ssh2 throws `Unsupported algorithm` on any entry +// outside its supported list, so the whole transport stops connecting. +describe('the copied ssh2 default algorithm list', () => { + // eslint-disable-next-line @typescript-eslint/no-require-imports -- the only way to read ssh2's internal constant + const ssh2Constants = require('ssh2/lib/protocol/constants.js') as { + DEFAULT_SERVER_HOST_KEY: string[] + SUPPORTED_SERVER_HOST_KEY: string[] + } + + it('still matches ssh2, in the same order', () => { + expect(DEFAULT_SERVER_HOST_KEY_ALGORITHMS).toEqual(ssh2Constants.DEFAULT_SERVER_HOST_KEY) + }) + + // The failure mode if it ever diverges: client.connect() throws synchronously before a socket is + // opened, so every target fails with a message about an algorithm the user never chose. + it('proposes nothing ssh2 would refuse', () => { + for (const algorithm of DEFAULT_SERVER_HOST_KEY_ALGORITHMS) { + expect(ssh2Constants.SUPPORTED_SERVER_HOST_KEY).toContain(algorithm) + } + }) + + // Type scoping is only safe because we can promote the type we hold, and RSA is negotiated under + // three different names for one key. + it('contains the RSA signature algorithms the ordering maps onto', () => { + expect(DEFAULT_SERVER_HOST_KEY_ALGORITHMS).toEqual( + expect.arrayContaining(['ssh-rsa', 'rsa-sha2-256', 'rsa-sha2-512']) + ) + }) +}) + describe('host key algorithm ordering', () => { const supported = ['ssh-ed25519', 'rsa-sha2-512', 'ssh-rsa', 'ecdsa-sha2-nistp256'] diff --git a/src/main/ssh/ssh-host-key-verifier.ts b/src/main/ssh/ssh-host-key-verifier.ts index c4cfb595b26..79d25a7dc6d 100644 --- a/src/main/ssh/ssh-host-key-verifier.ts +++ b/src/main/ssh/ssh-host-key-verifier.ts @@ -30,7 +30,7 @@ export type HostKeyVerifierDeps = { displayHost: string strictHostKeyChecking: string isEphemeralRuntimeTarget: boolean - siteConfigSuppressed: boolean + verificationSourcesIncomplete: boolean /** Already unioned across every known_hosts file. */ entries: readonly KnownHostsEntry[] isTrusted: TrustedHostKeyLookup @@ -51,6 +51,25 @@ export type HostKeyVerifierDeps = { isCurrentAttempt?: () => boolean } +/** + * ssh2's own default host-key proposal order, which we reorder rather than replace. + * + * A hand-copy, because ssh2 exports it only from a deep internal path. Two things make that safe: + * ssh2 THROWS `Unsupported algorithm` on any entry outside its supported list, so drift fails loudly + * at connect rather than silently; and a test pins this against the real list so an ssh2 upgrade + * that changes it is a review, not a surprise. Adopting a new proposal order automatically is the + * wrong default — the order is what makes type-scoped matching safe. + */ +export 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' +] + export function hostKeyFingerprintOf(key: Buffer): string { return formatHostKeyFingerprint(createHash('sha256').update(key).digest('base64')) } @@ -174,7 +193,7 @@ export function createHostKeyVerifier( storeOutcome: deps.isTrusted({ host: deps.host, port: deps.port, keyType, key }), strictHostKeyChecking: deps.strictHostKeyChecking, isEphemeralRuntimeTarget: deps.isEphemeralRuntimeTarget, - siteConfigSuppressed: deps.siteConfigSuppressed, + verificationSourcesIncomplete: deps.verificationSourcesIncomplete, displayHost: deps.displayHost })