fix(ssh): let an ephemeral runtime outrank sources we could not read

Three things, all about the same question: when we cannot see everything that
decides a host key, what does that actually license us to refuse?

1. ON-DEMAND RUNTIMES WERE REFUSED FOR A POLICY THEY COULD NEVER SATISFY.

A machine provisioned a minute ago cannot be in known_hosts, by construction —
which is why it has a carve-out at all. But the carve-out sat BELOW the
incomplete-sources check, so anyone whose HOME diverges from their passwd home
(sandboxes, our own E2E isolation) took the `-F` path, and every on-demand
runtime connection was refused, pointing at a config file the user cannot fix.

Refusing there buys nothing. No policy, seen or unseen, is satisfiable by a host
that did not exist yesterday; the trust comes from the provisioning channel. So
the carve-out now outranks it. An EXPLICIT StrictHostKeyChecking=yes still wins
over both — that one we can read, and the user asked for it.

2. THE FLAG WAS NAMED FOR ONE OF ITS TWO MEANINGS.

`siteConfigSuppressed` started as "-F hid /etc/ssh/ssh_config" and had since
grown "a known_hosts file exists and would not open" — which is not a site
config, and reading the name in the decision function told you nothing about
why an unreadable file landed there. It is `verificationSourcesIncomplete` now:
we could not see something that decides this, so do not extend NEW trust. A host
we already know still connects, because a match is decided before this is
reached, and that is now pinned by name.

3. THE COPIED ssh2 ALGORITHM LIST HAD NO DRIFT ALARM.

We reorder ssh2's default host-key proposal, which meant hand-copying a list
ssh2 exports only from a deep internal path. ssh2 throws `Unsupported algorithm`
on anything outside its supported list, so drift does not degrade — every
target stops connecting, before a socket opens, with a message about an
algorithm the user never chose. Worth knowing that the list is also built
conditionally on ed25519 support.

Kept as a literal rather than a deep import, since silently adopting a new
proposal order is the wrong default — the order is what makes type-scoped
matching safe, so a change deserves review. A test now compares it against
ssh2's real constant and checks every entry is one ssh2 accepts. Moved next to
the ordering function it feeds, and out from between the import statements.

1,488 SSH tests pass. The 3 in ssh-connection-sftp-namespace.test.ts are
pre-existing on origin/main.
This commit is contained in:
Neil
2026-08-14 03:38:07 -07:00
parent 6dcfcdface
commit 03264f2d82
5 changed files with 132 additions and 37 deletions
+8 -13
View File
@@ -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(
+42 -8
View File
@@ -7,7 +7,7 @@ function input(overrides: Partial<HostKeyDecisionInput> = {}): 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')
+28 -13
View File
@@ -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.
+33 -1
View File
@@ -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> = {}): 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']
+21 -2
View File
@@ -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
})