diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index db5287d2a..25bcaf447 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -148,17 +148,23 @@ vi.mock('ssh2', () => { }) const { + findSystemSshMock, getOrcaControlSocketPathMock, removeControlSocketPathMock, spawnSystemSshCommandMock, spawnSystemSshMock } = vi.hoisted(() => ({ + findSystemSshMock: vi.fn<() => string | null>(), getOrcaControlSocketPathMock: vi.fn(), removeControlSocketPathMock: vi.fn(), spawnSystemSshMock: vi.fn(), spawnSystemSshCommandMock: vi.fn() })) +// Why: security-key transport selection scans the real ~/.ssh defaults, so a developer's own +// FIDO2 key would otherwise decide which transport these tests take. +vi.mock('./system-ssh-binary', () => ({ findSystemSsh: findSystemSshMock })) + vi.mock('./ssh-system-fallback', () => ({ getOrcaControlSocketPath: getOrcaControlSocketPathMock, spawnSystemSsh: spawnSystemSshMock, @@ -331,6 +337,8 @@ describe('SshConnection', () => { vi.mocked(writeFileViaSystemSsh).mockResolvedValue(undefined) vi.mocked(resolveWithSshG).mockReset() vi.mocked(resolveWithSshG).mockResolvedValue(null) + findSystemSshMock.mockReset() + findSystemSshMock.mockReturnValue(null) vi.unstubAllEnvs() }) @@ -1697,6 +1705,7 @@ describe('SshConnection', () => { }) it('uses system SSH before ssh2 parses a security-key private key', async () => { + findSystemSshMock.mockReturnValue('/usr/bin/ssh') const directory = mkdtempSync(join(tmpdir(), 'orca-security-key-connect-')) const keyPath = join(directory, 'id_ed25519_sk') writeFileSync( @@ -1718,6 +1727,7 @@ describe('SshConnection', () => { }) it('uses system SSH for an agent-backed security-key public identity', async () => { + findSystemSshMock.mockReturnValue('/usr/bin/ssh') const directory = mkdtempSync(join(tmpdir(), 'orca-security-key-agent-connect-')) const identityPath = join(directory, 'id_ed25519_sk') writeFileSync( diff --git a/src/main/ssh/ssh-security-key-identity.test.ts b/src/main/ssh/ssh-security-key-identity.test.ts index c424bfa68..a16f5d2d9 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 fdbfb8707..2c058fcac 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