fix(ssh): never block or over-read on an SSH identity file (#32274)

Every place Orca reads an SSH key or identity file now goes through one
reader that refuses directories, FIFOs and devices (including through
symlinks) with a nonblocking open, and caps the read at 1 MB. An auth
failure now names any identity file that was skipped and why.

Fixes #31965. Reported by @Huge, who also proposed the first fix in #31966.

Co-authored-by: Martin Vlach <mr.huge@seznam.cz>
This commit is contained in:
Neil
2026-10-23 02:55:16 -07:00
committed by GitHub
co-authored by Martin Vlach
parent 7b0a7ee79f
commit 699c17ca16
12 changed files with 261 additions and 91 deletions
@@ -15,8 +15,16 @@ function testHomePath(...parts: string[]): string {
}
vi.mock('fs', () => ({
existsSync: (...args: unknown[]) => mockExistsSync(...args),
readFileSync: (...args: unknown[]) => mockReadFileSync(...args)
existsSync: (...args: unknown[]) => mockExistsSync(...args)
}))
vi.mock('../ssh-identity-file-read', () => ({
readSshIdentityFileSync: (path: string) => {
try {
return { contents: mockReadFileSync(path) ?? null, problem: null }
} catch (error) {
return { contents: null, problem: String(error) }
}
}
}))
import { buildConnectConfig } from './ssh-connect-config'
@@ -7,7 +7,8 @@ import {
resolveAgentConfigValue,
resolveAgentSocket,
resolvePrivateKeys,
resolveUnencryptedExplicitPrivateKeys
resolveUnencryptedExplicitPrivateKeys,
type SkippedIdentityFiles
} from '../ssh-auth-resolution'
import { configurePrivateKeyAuthentication } from '../ssh-private-key-authentication'
import { isOpenSshConfigBackedTarget } from '../system-transport/system-ssh-args'
@@ -18,6 +19,8 @@ export type BuildConnectConfigOptions = {
includePrivateKey?: boolean
/** No agent and no key: only the host's keyboard-interactive challenge (and a cached password). */
keyboardInteractiveOnly?: boolean
/** Collects identity files this config could not use, for the attempt's final error. */
skippedIdentityFiles?: SkippedIdentityFiles
}
// Why: ssh2 tries privateKey before agent, but parses encrypted privateKey
@@ -62,8 +65,8 @@ export function buildConnectConfig(
const keys = options.keyboardInteractiveOnly
? []
: (options.includePrivateKey ?? !agent)
? resolvePrivateKeys(target, resolved)
: resolveUnencryptedExplicitPrivateKeys(target, resolved)
? resolvePrivateKeys(target, resolved, options.skippedIdentityFiles)
: resolveUnencryptedExplicitPrivateKeys(target, resolved, options.skippedIdentityFiles)
let deferralDecision: boolean | undefined
configurePrivateKeyAuthentication(
config as ConnectConfig,
+7 -3
View File
@@ -33,6 +33,7 @@ import {
isDefiniteSystemSshHostFailure
} from '../ssh-reconnect-error-classification'
import { buildConnectConfig } from './ssh-connect-config'
import { withSkippedIdentityFileDetail, type SkippedIdentityFiles } from '../ssh-auth-resolution'
import { createSshOperationAbortError, type SshExecOptions } from './ssh-exec-options'
import type { SshConnectionCallbacks, SshCredentialKind } from './ssh-connection-callbacks'
import { collectKeyboardInteractiveResponses } from '../ssh-keyboard-interactive'
@@ -469,7 +470,8 @@ export class SshConnection {
// Why: a synchronous spawn throw bypasses the probe's catch, so clear system-transport state here or exec/sftp keep routing through the failed transport.
this.resetSystemTransport()
const config = buildConnectConfig(this.target, resolved)
const skippedIdentityFiles: SkippedIdentityFiles = new Map()
const config = buildConnectConfig(this.target, resolved, { skippedIdentityFiles })
// Why: ssh2 doesn't support ProxyCommand/ProxyJump natively; spawn the resolved proxy and pipe its stdin/stdout as config.sock.
const effectiveProxy = resolveEffectiveProxy(this.target, resolved)
@@ -491,9 +493,11 @@ export class SshConnection {
} catch (err) {
await recoverFailedSsh2Attempt(
err,
{ config, resolved, effectiveProxy },
{ config, resolved, effectiveProxy, skippedIdentityFiles },
this.credentialFallbackHost(connectGeneration)
)
).catch((finalErr: unknown) => {
throw withSkippedIdentityFileDetail(finalErr, skippedIdentityFiles)
})
}
}
@@ -1,6 +1,7 @@
import type { ConnectConfig } from 'ssh2'
import type { SshTarget } from '../../../shared/ssh-types'
import type { SshResolvedConfig } from '../config/ssh-g-config-resolution'
import type { SkippedIdentityFiles } from '../ssh-auth-resolution'
import type { SshCredentialKind } from './ssh-connection-callbacks'
import { buildConnectConfig } from './ssh-connect-config'
import type { resolveEffectiveProxy } from '../ssh-proxy-command'
@@ -25,6 +26,7 @@ type FailedSsh2Attempt = {
config: ConnectConfig
resolved: SshResolvedConfig | null
effectiveProxy: EffectiveProxy
skippedIdentityFiles?: SkippedIdentityFiles
}
/** What the fallback ladder reads from and drives on its connection attempt; getters read live state. */
@@ -73,7 +75,7 @@ export async function recoverFailedSsh2Attempt(
attempt: FailedSsh2Attempt,
host: Ssh2CredentialFallbackHost
): Promise<void> {
const { config, resolved, effectiveProxy } = attempt
const { config, resolved, effectiveProxy, skippedIdentityFiles } = attempt
if (!(err instanceof Error)) {
host.killProxyIfCurrent()
throw err
@@ -127,7 +129,8 @@ export async function recoverFailedSsh2Attempt(
if (isAgentFallbackError(authError) && config.agent && !config.privateKey) {
const keyConfig = buildConnectConfig(host.target, resolved, {
includeAgent: false,
includePrivateKey: true
includePrivateKey: true,
skippedIdentityFiles
})
// Why: if the agent path failed, password/passphrase retries must not reuse the same agent-only config.
credentialRetryConfig = keyConfig
@@ -7,8 +7,14 @@ const mocks = vi.hoisted(() => ({
readFileSync: vi.fn()
}))
vi.mock('fs', () => ({
readFileSync: (...args: unknown[]) => mocks.readFileSync(...args)
vi.mock('./ssh-identity-file-read', () => ({
readSshIdentityFileSync: (path: string) => {
try {
return { contents: mocks.readFileSync(path) ?? null, problem: null }
} catch (error) {
return { contents: null, problem: String(error) }
}
}
}))
vi.mock('os', () => ({
+6 -2
View File
@@ -1,4 +1,3 @@
import { readFileSync } from 'node:fs'
import {
BaseAgent,
createAgent,
@@ -10,6 +9,7 @@ import {
type SigningRequestOptions
} from 'ssh2'
import { resolveSshConfigHomePath } from './config/ssh-config-path-expansion'
import { readSshIdentityFileSync } from './ssh-identity-file-read'
type AgentPublicKey = ParsedKey | Buffer | string | PublicKeyEntry
@@ -69,8 +69,12 @@ class IdentityFilteredAgent extends BaseAgent<ParsedKey | Buffer | string> {
}
function parseIdentityKeyFile(filePath: string): ParsedKey | undefined {
const { contents } = readSshIdentityFileSync(filePath)
if (!contents) {
return undefined
}
try {
const parsed = utils.parseKey(readFileSync(filePath)) as ParsedKey | ParsedKey[] | Error
const parsed = utils.parseKey(contents) as ParsedKey | ParsedKey[] | Error
if (parsed instanceof Error) {
return undefined
}
+66 -26
View File
@@ -1,10 +1,12 @@
import { existsSync, readFileSync } from 'node:fs'
import { existsSync } from 'node:fs'
import { utils, type BaseAgent, type ParsedKey } from 'ssh2'
import type { SshTarget } from '../../shared/ssh-types'
import type { SshResolvedConfig } from './config/ssh-g-config-resolution'
import { createIdentityFilteredAgent } from './ssh-agent-identity-filter'
import { resolveSshConfigHomePath } from './config/ssh-config-path-expansion'
import { isOpenSshConfigBackedTarget } from './system-transport/system-ssh-args'
import { readSshIdentityFileSync } from './ssh-identity-file-read'
import { isAuthError } from './ssh-reconnect-error-classification'
// Why: ssh2 only tries keys that are explicitly provided. Users with keys in
// standard locations (e.g. ~/.ssh/id_ed25519) but no SSH agent running would
@@ -28,17 +30,16 @@ export function listDefaultIdentityFilePaths(): string[] {
return [...DEFAULT_IDENTITY_PATHS]
}
export function findDefaultKeyFile(): PrivateKeyFile | undefined {
/** Identity files one connect attempt could not use, by configured path, with the reason. */
export type SkippedIdentityFiles = Map<string, string>
export function findDefaultKeyFile(skipped?: SkippedIdentityFiles): PrivateKeyFile | undefined {
for (const keyPath of DEFAULT_KEY_PATHS) {
const resolved = resolveSshConfigHomePath(keyPath)
try {
if (!existsSync(resolved)) {
continue
}
const contents = readFileSync(resolved)
return { path: keyPath, contents }
} catch {
continue
const key = existsSync(resolveSshConfigHomePath(keyPath))
? readPrivateKey(keyPath, skipped)
: undefined
if (key) {
return key
}
}
return undefined
@@ -118,19 +119,42 @@ export function resolveIdentityFilePaths(
return resolved?.identityFile ?? []
}
function readPrivateKey(keyPath: string): PrivateKeyFile | undefined {
try {
const resolvedPath = resolveSshConfigHomePath(keyPath)
return { path: keyPath, contents: readFileSync(resolvedPath) }
} catch {
return undefined
const MAX_WARNED_IDENTITY_FILE_PROBLEMS = 64
const warnedIdentityFileProblems = new Set<string>()
// Why: every reconnect and credential retry re-reads keys; one warning per path and problem is enough.
function warnSkippedIdentityFileOnce(keyPath: string, problem: string): void {
const key = `${keyPath}\0${problem}`
if (warnedIdentityFileProblems.has(key)) {
return
}
if (warnedIdentityFileProblems.size >= MAX_WARNED_IDENTITY_FILE_PROBLEMS) {
warnedIdentityFileProblems.clear()
}
warnedIdentityFileProblems.add(key)
console.warn(`[ssh] Skipping identity file "${keyPath}": ${problem}`)
}
function readPrivateKeys(keyPaths: string[]): PrivateKeyFile[] {
function readPrivateKey(
keyPath: string,
skipped: SkippedIdentityFiles | undefined
): PrivateKeyFile | undefined {
const read = readSshIdentityFileSync(resolveSshConfigHomePath(keyPath))
if (read.contents) {
skipped?.delete(keyPath)
return { path: keyPath, contents: read.contents }
}
if (read.problem) {
skipped?.set(keyPath, read.problem)
warnSkippedIdentityFileOnce(keyPath, read.problem)
}
return undefined
}
function readPrivateKeys(keyPaths: string[], skipped?: SkippedIdentityFiles): PrivateKeyFile[] {
const keys: PrivateKeyFile[] = []
for (const keyPath of keyPaths) {
const key = readPrivateKey(keyPath)
const key = readPrivateKey(keyPath, skipped)
if (key) {
keys.push(key)
}
@@ -140,23 +164,38 @@ function readPrivateKeys(keyPaths: string[]): PrivateKeyFile[] {
function resolveExplicitPrivateKeys(
target: SshTarget,
resolved: SshResolvedConfig | null
resolved: SshResolvedConfig | null,
skipped?: SkippedIdentityFiles
): PrivateKeyFile[] {
return readPrivateKeys(resolveExplicitPrivateKeyPaths(target, resolved))
return readPrivateKeys(resolveExplicitPrivateKeyPaths(target, resolved), skipped)
}
export function resolvePrivateKeys(
target: SshTarget,
resolved: SshResolvedConfig | null
resolved: SshResolvedConfig | null,
skipped?: SkippedIdentityFiles
): PrivateKeyFile[] {
const keyPaths = resolveIdentityFilePaths(target, resolved)
if (keyPaths.length > 0 || resolved || target.identityFile) {
return readPrivateKeys(keyPaths)
return readPrivateKeys(keyPaths, skipped)
}
const defaultKey = findDefaultKeyFile()
const defaultKey = findDefaultKeyFile(skipped)
return defaultKey ? [defaultKey] : []
}
/** Names the identity files auth had to skip, so a generic auth failure says which key and why. */
export function withSkippedIdentityFileDetail(
error: unknown,
skipped: SkippedIdentityFiles
): unknown {
if (!(error instanceof Error) || !isAuthError(error) || skipped.size === 0) {
return error
}
const detail = [...skipped].map(([keyPath, problem]) => `"${keyPath}" (${problem})`).join(', ')
error.message = `${error.message.replace(/\.$/, '')}. Orca skipped identity file ${detail}.`
return error
}
export function isUnencryptedPrivateKey(contents: Buffer): boolean {
const parsed = utils.parseKey(contents) as ParsedKey | ParsedKey[] | Error
if (parsed instanceof Error) {
@@ -168,9 +207,10 @@ export function isUnencryptedPrivateKey(contents: Buffer): boolean {
export function resolveUnencryptedExplicitPrivateKeys(
target: SshTarget,
resolved: SshResolvedConfig | null
resolved: SshResolvedConfig | null,
skipped?: SkippedIdentityFiles
): PrivateKeyFile[] {
return resolveExplicitPrivateKeys(target, resolved).filter((key) =>
return resolveExplicitPrivateKeys(target, resolved, skipped).filter((key) =>
isUnencryptedPrivateKey(key.contents)
)
}
@@ -0,0 +1,81 @@
import { afterAll, beforeAll, describe, expect, it } from 'vitest'
import { mkdir, mkdtemp, rm, symlink, truncate, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { runProcess } from '@orca/process-host'
import type { SshTarget } from '../../shared/ssh-types'
import {
resolvePrivateKeys,
withSkippedIdentityFileDetail,
type SkippedIdentityFiles
} from './ssh-auth-resolution'
import { readSshIdentityFile } from './ssh-identity-file-read'
const isWindows = process.platform === 'win32'
let root = ''
function targetFor(identityFile: string): SshTarget {
return { id: 't', label: 'host', host: 'example.com', port: 22, username: 'u', identityFile }
}
beforeAll(async () => {
root = await mkdtemp(join(tmpdir(), 'orca-ssh-identity-'))
await mkdir(join(root, 'directory'))
await writeFile(join(root, 'huge'), '')
await truncate(join(root, 'huge'), 2 * 1024 * 1024)
// Why: Windows has no mkfifo and symlinks there need Developer Mode.
if (!isWindows) {
expect((await runProcess({ program: 'mkfifo', args: [join(root, 'fifo')] })).code).toBe(0)
await symlink('/dev/zero', join(root, 'link-to-dev-zero'))
for (const name of ['directory', 'huge', 'fifo']) {
await symlink(join(root, name), join(root, `link-to-${name}`))
}
}
})
afterAll(async () => {
await rm(root, { recursive: true, force: true })
})
describe('SSH identity file reads', () => {
const cases: [string, string][] = [
['directory', 'it is not a regular file'],
['huge', 'it is larger than 1 MB'],
...(isWindows
? []
: ([
['fifo', 'it is not a regular file'],
['link-to-dev-zero', 'it is not a regular file'],
['link-to-directory', 'it is not a regular file'],
['link-to-huge', 'it is larger than 1 MB'],
['link-to-fifo', 'it is not a regular file']
] satisfies [string, string][]))
]
it.each(cases)('skips %s without reading it and names why auth failed', async (name, reason) => {
const keyPath = join(root, name)
const skipped: SkippedIdentityFiles = new Map()
expect(resolvePrivateKeys(targetFor(keyPath), null, skipped)).toEqual([])
expect((await readSshIdentityFile(keyPath)).contents).toBeNull()
const error = withSkippedIdentityFileDetail(
new Error('All configured authentication methods failed.'),
skipped
)
expect(error).toHaveProperty(
'message',
`All configured authentication methods failed. Orca skipped identity file "${keyPath}" (${reason}).`
)
})
it('leaves non-auth errors and missing files unannotated', () => {
const skipped: SkippedIdentityFiles = new Map()
expect(resolvePrivateKeys(targetFor(join(root, 'missing')), null, skipped)).toEqual([])
expect(skipped.size).toBe(0)
resolvePrivateKeys(targetFor(join(root, 'directory')), null, skipped)
const networkError = new Error('connect ECONNREFUSED')
withSkippedIdentityFileDetail(networkError, skipped)
expect(networkError.message).toBe('connect ECONNREFUSED')
})
})
+48
View File
@@ -0,0 +1,48 @@
import {
NodeFileNotRegularError,
NodeFileReadTooLargeError,
readNodeFileSyncWithinLimit,
readNodeFileWithinLimit
} from '../../shared/node-bounded-file-reader'
import { MAX_SSH_IDENTITY_FILE_BYTES } from './ssh-security-key-identity'
/** `problem` is null when the file is simply absent, which is normal for default key paths. */
export type SshIdentityFileRead = { contents: Buffer } | { contents: null; problem: string | null }
const READ_OPTIONS = { regularFileOnly: true }
// Why one reader: identity paths come from user config, so a FIFO or device must never block the
// main process and an oversized file must never be loaded whole.
export function readSshIdentityFileSync(filePath: string): SshIdentityFileRead {
try {
return {
contents: readNodeFileSyncWithinLimit(filePath, MAX_SSH_IDENTITY_FILE_BYTES, READ_OPTIONS)
.buffer
}
} catch (error) {
return { contents: null, problem: describeIdentityFileProblem(error) }
}
}
export async function readSshIdentityFile(filePath: string): Promise<SshIdentityFileRead> {
try {
const read = await readNodeFileWithinLimit(filePath, MAX_SSH_IDENTITY_FILE_BYTES, READ_OPTIONS)
return { contents: read.buffer }
} catch (error) {
return { contents: null, problem: describeIdentityFileProblem(error) }
}
}
function describeIdentityFileProblem(error: unknown): string | null {
const code = error instanceof Error && 'code' in error ? error.code : undefined
if (code === 'ENOENT' || code === 'ENOTDIR') {
return null
}
if (error instanceof NodeFileReadTooLargeError) {
return `it is larger than ${MAX_SSH_IDENTITY_FILE_BYTES / 1024 / 1024} MB`
}
if (error instanceof NodeFileNotRegularError) {
return 'it is not a regular file'
}
return `it could not be read (${typeof code === 'string' ? code : String(error)})`
}
@@ -18,8 +18,16 @@ const mockExistsSync = vi.fn().mockReturnValue(false)
const mockReadFileSync = vi.fn()
vi.mock('fs', () => ({
existsSync: (...args: unknown[]) => mockExistsSync(...args),
readFileSync: (...args: unknown[]) => mockReadFileSync(...args)
existsSync: (...args: unknown[]) => mockExistsSync(...args)
}))
vi.mock('./ssh-identity-file-read', () => ({
readSshIdentityFileSync: (path: string) => {
try {
return { contents: mockReadFileSync(path) ?? null, problem: null }
} catch (error) {
return { contents: null, problem: String(error) }
}
}
}))
import { buildConnectConfig } from './connection/ssh-connect-config'
+3 -45
View File
@@ -1,14 +1,12 @@
import { constants } from 'node:fs'
import { open, stat } from 'node:fs/promises'
import type { SshTarget } from '../../shared/ssh-types'
import type { SshResolvedConfig } from './config/ssh-g-config-resolution'
import { listDefaultIdentityFilePaths, resolveIdentityFilePaths } from './ssh-auth-resolution'
import { resolveSshConfigHomePath } from './config/ssh-config-path-expansion'
import {
MAX_SSH_IDENTITY_FILE_BYTES,
isOpenSshSecurityKeyPrivateKey,
isOpenSshSecurityKeyPublicKey
} from './ssh-security-key-identity'
import { readSshIdentityFile } from './ssh-identity-file-read'
import { isOpenSshConfigBackedTarget } from './system-transport/system-ssh-args'
import { findSystemSsh } from './system-transport/system-ssh-binary'
@@ -17,54 +15,14 @@ type TransportResolvedConfig = Pick<
'proxyUseFdpass' | 'proxyCommand' | 'proxyJump'
>
const READ_CHUNK_BYTES = 64 * 1024
const READ_OPEN_FLAGS =
constants.O_RDONLY | (process.platform === 'win32' ? 0 : constants.O_NONBLOCK)
async function readBoundedKeyFile(path: string): Promise<Buffer | null> {
let handle: Awaited<ReturnType<typeof open>> | undefined
try {
const pathStats = await stat(path)
if (!pathStats.isFile() || pathStats.size > MAX_SSH_IDENTITY_FILE_BYTES) {
return null
}
handle = await open(path, READ_OPEN_FLAGS)
const stats = await handle.stat()
if (!stats.isFile() || stats.size > MAX_SSH_IDENTITY_FILE_BYTES) {
return null
}
const chunks: Buffer[] = []
let offset = 0
while (offset <= MAX_SSH_IDENTITY_FILE_BYTES) {
const buffer = Buffer.alloc(
Math.min(READ_CHUNK_BYTES, MAX_SSH_IDENTITY_FILE_BYTES + 1 - offset)
)
const { bytesRead } = await handle.read(buffer, 0, buffer.length, offset)
if (bytesRead === 0) {
break
}
offset += bytesRead
if (offset > MAX_SSH_IDENTITY_FILE_BYTES) {
return null
}
chunks.push(buffer.subarray(0, bytesRead))
}
return Buffer.concat(chunks, offset)
} catch {
return null
} finally {
await handle?.close().catch(() => undefined)
}
}
async function identityRequiresSystemSsh(keyPath: string): Promise<boolean> {
const resolvedPath = resolveSshConfigHomePath(keyPath)
const identity = await readBoundedKeyFile(resolvedPath)
const identity = (await readSshIdentityFile(resolvedPath)).contents
// Why: a present private key wins; a `.pub` beside it may describe a key already replaced.
if (identity !== null) {
return isOpenSshSecurityKeyPublicKey(identity) || isOpenSshSecurityKeyPrivateKey(identity)
}
const publicIdentity = await readBoundedKeyFile(`${resolvedPath}.pub`)
const publicIdentity = (await readSshIdentityFile(`${resolvedPath}.pub`)).contents
return publicIdentity !== null && isOpenSshSecurityKeyPublicKey(publicIdentity)
}
+11 -4
View File
@@ -15,6 +15,13 @@ export class NodeFileReadTooLargeError extends Error {
}
}
export class NodeFileNotRegularError extends Error {
constructor() {
super('Expected a regular file')
this.name = 'NodeFileNotRegularError'
}
}
export type BoundedNodeFileRead = {
buffer: Buffer
stats: Stats
@@ -42,7 +49,7 @@ export async function readNodeFileWithinLimit(
): Promise<BoundedNodeFileRead> {
options.signal?.throwIfAborted()
if (options.regularFileOnly && !(await stat(filePath)).isFile()) {
throw new Error('Expected a regular file')
throw new NodeFileNotRegularError()
}
options.signal?.throwIfAborted()
// Nonblocking open fences replacement with a FIFO after the path check.
@@ -70,7 +77,7 @@ export async function readNodeFileHandleWithinLimit(
const stats = await handle.stat()
options.signal?.throwIfAborted()
if (options.regularFileOnly && !stats.isFile()) {
throw new Error('Expected a regular file')
throw new NodeFileNotRegularError()
}
if (options.window) {
validateSize(stats.size, Number.MAX_SAFE_INTEGER)
@@ -148,7 +155,7 @@ export function readNodeFileSyncWithinLimit(
}
if (options.regularFileOnly && !statSync(filePath).isFile()) {
throw new Error('Expected a regular file')
throw new NodeFileNotRegularError()
}
// Nonblocking open also fences replacement with a FIFO after the path check.
const flags = options.regularFileOnly
@@ -158,7 +165,7 @@ export function readNodeFileSyncWithinLimit(
try {
const stats = fstatSync(descriptor)
if (options.regularFileOnly && !stats.isFile()) {
throw new Error('Expected a regular file')
throw new NodeFileNotRegularError()
}
validateSize(stats.size, maxBytes)