From b337db0936ecdacc8bca3b92cc006d50fed1aaef Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 1 Aug 2026 18:18:25 -0700 Subject: [PATCH] fix(ssh): gate FIDO2 system-transport on an OpenSSH binary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ssh -G` echoes OpenSSH's built-in default identity list for every host, so `usesDefaultPaths` was almost never true and the security-key gate returned `!usesDefaultPaths || findSystemSsh() !== null` — forcing system transport without checking that an `ssh` binary exists. `spawnSystemSsh()` then throws `No system ssh binary found`, hard-failing connections that worked on ssh2. The same flag also stopped the default scan at the first existing normal private key, so a host that only accepts a FIDO2 key never reached system OpenSSH when `~/.ssh/id_rsa` happened to exist. Both decisions are independent of where an identity path came from: always require `findSystemSsh() !== null` before forcing system transport, and scan every candidate identity instead of stopping on the first normal key. `shouldUseSystemSshTransport()` is untouched, so ProxyCommand / ProxyJump / ProxyUseFdpass keep their intentional system transport. --- .../ssh/ssh-security-key-identity.test.ts | 110 ++++++++++++------ src/main/ssh/ssh-transport-selection.ts | 37 +++--- 2 files changed, 92 insertions(+), 55 deletions(-) diff --git a/src/main/ssh/ssh-security-key-identity.test.ts b/src/main/ssh/ssh-security-key-identity.test.ts index c424bfa6891..a16f5d2d96d 100644 --- a/src/main/ssh/ssh-security-key-identity.test.ts +++ b/src/main/ssh/ssh-security-key-identity.test.ts @@ -53,6 +53,29 @@ async function writeKey(contents: Buffer, filename = 'security key'): Promise): Promise { + const directory = await mkdtemp(join(tmpdir(), 'orca-default-key-home-')) + tempDirs.push(directory) + await mkdir(join(directory, '.ssh')) + for (const [name, contents] of Object.entries(files)) { + await writeFile(join(directory, '.ssh', name), contents) + } + return directory +} + +// Why: `ssh -G` echoes this list, already home-expanded, for every host — configured or not. +function listBuiltInDefaultIdentityFiles(home: string): string[] { + return [ + 'id_rsa', + 'id_ecdsa', + 'id_ecdsa_sk', + 'id_ed25519', + 'id_ed25519_sk', + 'id_xmss', + 'id_dsa' + ].map((name) => join(home, '.ssh', name)) +} + afterEach(async () => { vi.unstubAllEnvs() await Promise.all(tempDirs.splice(0).map((directory) => rm(directory, { recursive: true }))) @@ -155,13 +178,9 @@ describe('isOpenSshSecurityKeyPrivateKey', () => { describe('requiresSystemSshForSecurityKey', () => { it('uses default FIDO2 identities only when config resolution is unavailable', async () => { - const directory = await mkdtemp(join(tmpdir(), 'orca-security-key-home-')) - tempDirs.push(directory) - await mkdir(join(directory, '.ssh')) - await writeFile( - join(directory, '.ssh', 'id_ed25519_sk'), - createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) - ) + const directory = await createDefaultKeyHome({ + id_ed25519_sk: createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + }) vi.stubEnv('ORCA_TEST_SSH_HOME', directory) await expect(requiresSystemSshForSecurityKey(createTarget(), null)).resolves.toBe(true) @@ -170,28 +189,58 @@ describe('requiresSystemSshForSecurityKey', () => { ).resolves.toBe(false) }) - it('keeps a regular default ahead of a dormant FIDO2 identity', async () => { - const directory = await mkdtemp(join(tmpdir(), 'orca-regular-key-home-')) - tempDirs.push(directory) - await mkdir(join(directory, '.ssh')) - await writeFile(join(directory, '.ssh', 'id_rsa'), createOpenSshPrivateKeyFixture(['ssh-rsa'])) - await writeFile( - join(directory, '.ssh', 'id_ed25519_sk'), - createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) - ) + it('reaches a default FIDO2 identity that a regular default key precedes', async () => { + const directory = await createDefaultKeyHome({ + id_rsa: createOpenSshPrivateKeyFixture(['ssh-rsa']), + id_ed25519_sk: createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + }) + vi.stubEnv('ORCA_TEST_SSH_HOME', directory) + + await expect(requiresSystemSshForSecurityKey(createTarget(), null)).resolves.toBe(true) + + findSystemSshMock.mockReturnValue(null) + await expect(requiresSystemSshForSecurityKey(createTarget(), null)).resolves.toBe(false) + }) + + it('leaves regular-only defaults on ssh2', async () => { + const directory = await createDefaultKeyHome({ + id_rsa: createOpenSshPrivateKeyFixture(['ssh-rsa']) + }) vi.stubEnv('ORCA_TEST_SSH_HOME', directory) await expect(requiresSystemSshForSecurityKey(createTarget(), null)).resolves.toBe(false) }) - it('keeps password and agent fallback when default FIDO2 needs unavailable OpenSSH', async () => { - const directory = await mkdtemp(join(tmpdir(), 'orca-no-system-ssh-home-')) - tempDirs.push(directory) - await mkdir(join(directory, '.ssh')) - await writeFile( - join(directory, '.ssh', 'id_ed25519_sk'), - createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + it('treats resolved built-in default identities as unconfigured, not as forced transport', async () => { + const directory = await createDefaultKeyHome({ + id_rsa: createOpenSshPrivateKeyFixture(['ssh-rsa']), + id_ed25519_sk: createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + }) + const identityFile = listBuiltInDefaultIdentityFiles(directory) + + await expect(requiresSystemSshForSecurityKey(createTarget(), { identityFile })).resolves.toBe( + true ) + + findSystemSshMock.mockReturnValue(null) + await expect(requiresSystemSshForSecurityKey(createTarget(), { identityFile })).resolves.toBe( + false + ) + }) + + it('keeps password and agent fallback when a configured FIDO2 identity has no OpenSSH', async () => { + const keyPath = await writeKey(createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY])) + findSystemSshMock.mockReturnValue(null) + + await expect( + requiresSystemSshForSecurityKey(createTarget({ identityFile: keyPath }), null) + ).resolves.toBe(false) + }) + + it('keeps password and agent fallback when default FIDO2 needs unavailable OpenSSH', async () => { + const directory = await createDefaultKeyHome({ + id_ed25519_sk: createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + }) vi.stubEnv('ORCA_TEST_SSH_HOME', directory) findSystemSshMock.mockReturnValue(null) @@ -199,17 +248,10 @@ describe('requiresSystemSshForSecurityKey', () => { }) it('ignores an orphan regular sidecar before a valid default FIDO2 identity', async () => { - const directory = await mkdtemp(join(tmpdir(), 'orca-orphan-sidecar-home-')) - tempDirs.push(directory) - await mkdir(join(directory, '.ssh')) - await writeFile( - join(directory, '.ssh', 'id_ed25519.pub'), - createOpenSshPublicKeyFixture('ssh-ed25519') - ) - await writeFile( - join(directory, '.ssh', 'id_ed25519_sk'), - createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) - ) + const directory = await createDefaultKeyHome({ + 'id_ed25519.pub': createOpenSshPublicKeyFixture('ssh-ed25519'), + id_ed25519_sk: createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + }) vi.stubEnv('ORCA_TEST_SSH_HOME', directory) await expect(requiresSystemSshForSecurityKey(createTarget(), null)).resolves.toBe(true) diff --git a/src/main/ssh/ssh-transport-selection.ts b/src/main/ssh/ssh-transport-selection.ts index fdbfb87075f..2c058fcac85 100644 --- a/src/main/ssh/ssh-transport-selection.ts +++ b/src/main/ssh/ssh-transport-selection.ts @@ -21,8 +21,6 @@ const READ_CHUNK_BYTES = 64 * 1024 const READ_OPEN_FLAGS = constants.O_RDONLY | (process.platform === 'win32' ? 0 : constants.O_NONBLOCK) -type IdentityInspection = { privateIdentityExists: boolean; requiresSystemSsh: boolean } - async function readBoundedKeyFile(path: string): Promise { let handle: Awaited> | undefined try { @@ -59,21 +57,15 @@ async function readBoundedKeyFile(path: string): Promise { } } -async function inspectIdentityPath(keyPath: string): Promise { +async function identityRequiresSystemSsh(keyPath: string): Promise { const resolvedPath = resolveSshConfigHomePath(keyPath) const identity = await readBoundedKeyFile(resolvedPath) + // Why: a present private key wins; a `.pub` beside it may describe a key already replaced. if (identity !== null) { - return { - privateIdentityExists: true, - requiresSystemSsh: - isOpenSshSecurityKeyPublicKey(identity) || isOpenSshSecurityKeyPrivateKey(identity) - } + return isOpenSshSecurityKeyPublicKey(identity) || isOpenSshSecurityKeyPrivateKey(identity) } const publicIdentity = await readBoundedKeyFile(`${resolvedPath}.pub`) - return { - privateIdentityExists: false, - requiresSystemSsh: publicIdentity !== null && isOpenSshSecurityKeyPublicKey(publicIdentity) - } + return publicIdentity !== null && isOpenSshSecurityKeyPublicKey(publicIdentity) } export function shouldUseSystemSshTransport( @@ -102,16 +94,19 @@ export async function requiresSystemSshForSecurityKey( target: SshTarget, resolved: Pick | null ): Promise { - const configuredPaths = resolveIdentityFilePaths(target, resolved) - const usesDefaultPaths = configuredPaths.length === 0 && !resolved && !target.identityFile - const identityPaths = usesDefaultPaths ? listDefaultIdentityFilePaths() : configuredPaths + const resolvedPaths = resolveIdentityFilePaths(target, resolved) + // Why: `ssh -G` already echoes OpenSSH's built-in defaults, so its list is the real candidate + // set; guess at the defaults only when config resolution failed outright. + const identityPaths = + resolvedPaths.length === 0 && !resolved && !target.identityFile + ? listDefaultIdentityFilePaths() + : resolvedPaths for (const keyPath of identityPaths) { - const inspection = await inspectIdentityPath(keyPath) - if (inspection.requiresSystemSsh) { - return !usesDefaultPaths || findSystemSsh() !== null - } - if (usesDefaultPaths && inspection.privateIdentityExists) { - return false + if (await identityRequiresSystemSsh(keyPath)) { + // Why: scan every candidate — an earlier normal key never rules out a security key the host + // requires — but forcing system transport with no binary hard-fails a connection ssh2 could + // still have served over agent or password auth. + return findSystemSsh() !== null } } return false