mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
fix(ssh): stop treating an absent known_hosts as a source we failed to read
The previous commit's "no readable source is not first contact" guard was right about the danger and wrong about how to detect it, and the version that shipped would have refused every connection a new profile ever makes. A file that does not exist and a file that refuses to open both arrive as a rejected readFile, and I counted them the same way. They are opposites. An absent known_hosts is the normal state — ssh creates it on its own first connect, and an Orca profile that has never connected has none — and it is real evidence that no host is known. A file that exists and will not open is evidence withheld: the entry that would have said "this key changed" may be sitting in it. The default list is the reason this was fatal rather than obscure. It always names known_hosts2, which essentially never exists, so on a machine with a normal known_hosts the count was 1-of-2 and everything worked; on a fresh profile it was 0-of-2 and every connect failed with "the system SSH configuration could not be read" — a message about a file the user does not have and an error they cannot act on. The suite passed only because the machine running it happens to have a known_hosts. Pointing HOME at an empty directory fails 62 connection tests, which is what the new wiring test does. So the count is now unreadableFileCount: files that exist and could not be read, which is the condition the guard was always trying to express. Any one of them takes the strict path; an absent or empty file takes none. The store clause is gone with it — a store hit already returns accept before this is consulted, so it never changed an outcome. Tests: absent, empty, permission-denied, directory, and all-parsed at the source; first contact with no known_hosts at all at the wiring level. 1,482 SSH tests pass. The 3 failures in ssh-connection-sftp-namespace.test.ts are pre-existing on origin/main and untouched here.
This commit is contained in:
@@ -461,6 +461,25 @@ describe('SshConnection', () => {
|
||||
expect(conn.getState().error).toMatch(/host key verification failed/i)
|
||||
})
|
||||
|
||||
// Everyone's first connection happens before ~/.ssh/known_hosts exists — ssh creates it on its
|
||||
// own first connect. Treating a file that was never there as a source we failed to read would
|
||||
// refuse every connection a new profile ever makes, which is how a fail-closed rule turns into a
|
||||
// product that does not work.
|
||||
it('connects on first contact when no known_hosts file exists yet', async () => {
|
||||
const emptyHome = mkdtempSync(join(tmpdir(), 'orca-ssh-home-'))
|
||||
vi.stubEnv('HOME', emptyHome)
|
||||
|
||||
try {
|
||||
const conn = new SshConnection(createTarget(), createCallbacks())
|
||||
await conn.connect()
|
||||
|
||||
expect(lastHostKeyAccepted).toBe(true)
|
||||
expect(conn.getState().status).toBe('connected')
|
||||
} finally {
|
||||
rmSync(emptyHome, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
// Retrying re-derives the same decision, so a ladder that treated this as transient would back off
|
||||
// against a host it has already refused until it gave up — burying the reason.
|
||||
it('does not retry a refused host key', async () => {
|
||||
|
||||
@@ -1265,14 +1265,12 @@ export class SshConnection {
|
||||
displayHost: hostKeyLookupHost,
|
||||
strictHostKeyChecking: hostKeyResolved?.strictHostKeyChecking ?? 'ask',
|
||||
isEphemeralRuntimeTarget: this.target.owner?.type === 'on-demand-runtime',
|
||||
// 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),
|
||||
// A source that exists but will not open is the absence of evidence, not evidence of a new
|
||||
// host: the entry that would have said "this key changed" may be in the file we could not
|
||||
// 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,
|
||||
entries: knownHostsEntries,
|
||||
isTrusted: ({ host, port, keyType, key }) => {
|
||||
const forHost = trustedHostKeys.filter(
|
||||
|
||||
@@ -7,6 +7,7 @@ import { matchKnownHosts, readHostKeyType, type KnownHostsEntry } from './ssh-kn
|
||||
import {
|
||||
defaultKnownHostsFiles,
|
||||
loadKnownHostsEntries,
|
||||
loadKnownHostsEvidence,
|
||||
resolveKnownHostsFiles,
|
||||
resolveKnownHostsLookupHost
|
||||
} from './ssh-known-hosts-source'
|
||||
@@ -215,6 +216,71 @@ describe('loadKnownHostsEntries', () => {
|
||||
})
|
||||
})
|
||||
|
||||
// The caller turns this count into a refusal, so the line between the two cases is the line between
|
||||
// "every connection works" and "no connection works".
|
||||
describe('loadKnownHostsEvidence', () => {
|
||||
// A profile that has never connected has no known_hosts — ssh writes one on its own first
|
||||
// connect. Counting that as a source we failed to read would refuse every first connection every
|
||||
// new user ever makes.
|
||||
it('does not count an absent file as unreadable', async () => {
|
||||
const root = await createRoot()
|
||||
|
||||
const evidence = await loadKnownHostsEvidence([
|
||||
join(root, 'known_hosts'),
|
||||
join(root, 'known_hosts2')
|
||||
])
|
||||
|
||||
expect(evidence.unreadableFileCount).toBe(0)
|
||||
expect(evidence.entries).toEqual([])
|
||||
})
|
||||
|
||||
it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)(
|
||||
'counts a file that exists but will not open',
|
||||
async () => {
|
||||
const root = await createRoot()
|
||||
const unreadable = join(root, 'known_hosts')
|
||||
await writeFile(unreadable, hostLine('secret.example', ED_B))
|
||||
await chmod(unreadable, 0o000)
|
||||
|
||||
const evidence = await loadKnownHostsEvidence([unreadable])
|
||||
|
||||
// The entry that would have said "this key changed" could be in here; we cannot tell.
|
||||
expect(evidence.unreadableFileCount).toBe(1)
|
||||
}
|
||||
)
|
||||
|
||||
it('counts a path that is a directory', async () => {
|
||||
const root = await createRoot()
|
||||
await mkdir(join(root, 'a_directory'))
|
||||
|
||||
const evidence = await loadKnownHostsEvidence([join(root, 'a_directory')])
|
||||
|
||||
expect(evidence.unreadableFileCount).toBe(1)
|
||||
})
|
||||
|
||||
it('reports nothing unreadable when every file parses', async () => {
|
||||
const root = await createRoot()
|
||||
const present = join(root, 'known_hosts')
|
||||
await writeFile(present, hostLine('alpha.example', ED_A))
|
||||
|
||||
const evidence = await loadKnownHostsEvidence([present])
|
||||
|
||||
expect(evidence.unreadableFileCount).toBe(0)
|
||||
expect(evidence.entries).toHaveLength(1)
|
||||
})
|
||||
|
||||
// An empty file is a file we read successfully. Nothing is withheld, so nothing is suppressed.
|
||||
it('does not count an empty file as unreadable', async () => {
|
||||
const root = await createRoot()
|
||||
const empty = join(root, 'known_hosts')
|
||||
await writeFile(empty, '')
|
||||
|
||||
const evidence = await loadKnownHostsEvidence([empty])
|
||||
|
||||
expect(evidence.unreadableFileCount).toBe(0)
|
||||
})
|
||||
})
|
||||
|
||||
describe('resolveKnownHostsLookupHost', () => {
|
||||
it('prefers HostKeyAlias over the resolved hostname', () => {
|
||||
// A bastion tunnelled through localhost:2200 would otherwise mismatch on every target.
|
||||
|
||||
@@ -43,12 +43,15 @@ export async function loadKnownHostsEntries(files: readonly string[]): Promise<K
|
||||
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.
|
||||
* Files that EXIST but could not be read — a permissions problem, a directory, an I/O error.
|
||||
*
|
||||
* Deliberately not "files that produced no entries": a file that is simply absent is the normal
|
||||
* state (ssh creates known_hosts on its own first connect, and most Orca profiles start without
|
||||
* one), and it is real evidence that no host is known. A file that exists and refuses to open is
|
||||
* the opposite — evidence withheld, so an entry that would have said "this key changed" may be
|
||||
* sitting in it. The caller must not record trust while any source is silent that way.
|
||||
*/
|
||||
readableFileCount: number
|
||||
configuredFileCount: number
|
||||
unreadableFileCount: number
|
||||
}
|
||||
|
||||
export async function loadKnownHostsEvidence(
|
||||
@@ -57,18 +60,16 @@ export async function loadKnownHostsEvidence(
|
||||
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: parseKnownHosts(await readFile(path, 'utf8')), unreadable: false }
|
||||
} catch (err) {
|
||||
const absent = (err as NodeJS.ErrnoException).code === 'ENOENT'
|
||||
return { entries: [] as KnownHostsEntry[], unreadable: !absent }
|
||||
}
|
||||
})
|
||||
)
|
||||
return {
|
||||
entries: perFile.flatMap((file) => file.entries),
|
||||
readableFileCount: perFile.filter((file) => file.readable).length,
|
||||
configuredFileCount: files.length
|
||||
unreadableFileCount: perFile.filter((file) => file.unreadable).length
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user