mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 08:37:49 +00:00
fix(ssh): expand %d, ${NAME} and other tokens in ssh -G IdentityFile paths (#33981)
* fix(ssh): expand %d and other tokens in ssh -G IdentityFile paths OpenSSH prints IdentityFile values from `ssh -G` with their percent tokens unexpanded (it expands them only at connect time), so a config line like `IdentityFile %d/keys/work.pem` reached Orca as a literal relative path. The key was silently skipped and the connection fell back to a password prompt even though plain `ssh <alias>` worked. Expand the tokens OpenSSH supports for IdentityFile that can be derived locally or from the ssh -G output (%%, %d, %u, %i, %l, %L, %h, %n, %p, %r). Paths with tokens that need OpenSSH internals (%C, %j, %k) are left unchanged. A configured, non-default identity file that does not exist is now named in the auth error instead of being skipped silently. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(ssh): reproduce #33849 end to end and keep tests off the real ~/.ssh Adds a Docker e2e: a host from ~/.ssh/config whose IdentityFile uses %r and %p connects with that key. It fails on v1.4.241 and passes with the token expansion. Drops a unit assertion that read the real ~/.ssh/id_xmss. * fix(ssh): expand ${NAME} in ssh -G IdentityFile paths too ssh -G also prints IdentityFile's ${NAME} environment references unexpanded, so a key at ${KEYS}/work.pem fell back to a password prompt in the same way as #33849's %d. Expand the braced form first, and keep the raw path when the variable is unset, as OpenSSH refuses it. --------- Co-authored-by: william <wanglv93@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
william
Claude Opus 5.5
parent
6b831180e3
commit
1d0d3f5e94
@@ -154,6 +154,17 @@ export const PR_E2E_SOURCE_ROUTES = [
|
||||
file
|
||||
))
|
||||
},
|
||||
{
|
||||
id: 'ssh.orcad-identity-file-tokens',
|
||||
specs: ['tests/e2e/ssh-orcad-identity-file-tokens.spec.ts'],
|
||||
matches: (file) =>
|
||||
MANAGED_SSH_HARNESS.test(file) ||
|
||||
/^tests\/e2e\/helpers\/ssh-config-host-picker\.ts$/.test(file) ||
|
||||
(isProductSource(file) &&
|
||||
/^src\/main\/ssh\/(?:config\/(?:ssh-g-config-resolution|ssh-identity-file-tokens|ssh-config-path-expansion)|ssh-auth-resolution|ssh-identity-file-read)\.ts$/.test(
|
||||
file
|
||||
))
|
||||
},
|
||||
{
|
||||
id: 'ssh.orcad-update-anyway',
|
||||
specs: ['tests/e2e/ssh-orcad-update-anyway.spec.ts'],
|
||||
|
||||
@@ -9,7 +9,9 @@ type NodeOsModule = typeof NodeOs
|
||||
const { existsSyncMock, homedirMock, userInfoMock } = vi.hoisted(() => ({
|
||||
existsSyncMock: vi.fn<(path: string) => boolean>(() => true),
|
||||
homedirMock: vi.fn<() => string>(() => '/home/env'),
|
||||
userInfoMock: vi.fn<() => { homedir: string }>(() => ({ homedir: '/home/env' }))
|
||||
userInfoMock: vi.fn<() => { homedir: string; username?: string }>(() => ({
|
||||
homedir: '/home/env'
|
||||
}))
|
||||
}))
|
||||
|
||||
vi.mock('node:fs', () => ({ existsSync: existsSyncMock }))
|
||||
@@ -21,6 +23,7 @@ vi.mock('node:os', async (importOriginal) => ({
|
||||
}))
|
||||
|
||||
import {
|
||||
parseSshGOutput,
|
||||
siteConfigMayRestrictHostKeys,
|
||||
splitIncludeArguments,
|
||||
sshGArgsForHost
|
||||
@@ -75,6 +78,56 @@ describe('sshGArgsForHost', () => {
|
||||
})
|
||||
|
||||
/** -F excludes user and system config; read site policy separately and fail closed on uncertainty. */
|
||||
describe('parseSshGOutput IdentityFile tokens', () => {
|
||||
beforeEach(() => {
|
||||
homedirMock.mockReset().mockReturnValue('/home/env')
|
||||
userInfoMock.mockReset().mockReturnValue({ homedir: '/home/passwd', username: 'alice' })
|
||||
})
|
||||
|
||||
const sshG = (...identityFiles: string[]): string =>
|
||||
[
|
||||
'host work',
|
||||
...identityFiles.map((file) => `identityfile ${file}`),
|
||||
'user bob',
|
||||
'hostname example.com',
|
||||
'port 2200'
|
||||
].join('\n')
|
||||
|
||||
it('expands the tokens OpenSSH 10.3 leaves raw in ssh -G output', () => {
|
||||
expect(parseSshGOutput(sshG('%d/keys/%h-%r-%p-%n-%u-%%.pem')).identityFile).toEqual([
|
||||
'/home/passwd/keys/example.com-bob-2200-work-alice-%.pem'
|
||||
])
|
||||
})
|
||||
|
||||
it('still expands a leading tilde', () => {
|
||||
expect(parseSshGOutput(sshG('~/.ssh/id_work')).identityFile).toEqual([
|
||||
join('/home/env', '.ssh', 'id_work')
|
||||
])
|
||||
})
|
||||
|
||||
it('expands ${NAME} from the environment, and keeps the path raw when NAME is unset', () => {
|
||||
vi.stubEnv('ORCA_TEST_KEYS', '/srv/keys-%r')
|
||||
try {
|
||||
expect(
|
||||
parseSshGOutput(sshG('${ORCA_TEST_KEYS}/%r', '${ORCA_TEST_UNSET_VAR}/k')).identityFile
|
||||
).toEqual(['/srv/keys-%r/bob', '${ORCA_TEST_UNSET_VAR}/k'])
|
||||
} finally {
|
||||
vi.unstubAllEnvs()
|
||||
}
|
||||
})
|
||||
|
||||
it('reads `IdentityFile none` as no key, not a missing file', () => {
|
||||
expect(parseSshGOutput(sshG('none')).identityFile).toEqual([])
|
||||
})
|
||||
|
||||
it('keeps a path with a token it cannot resolve unchanged', () => {
|
||||
expect(parseSshGOutput(sshG('%d/%C.pem', '/keys/100%')).identityFile).toEqual([
|
||||
'%d/%C.pem',
|
||||
'/keys/100%'
|
||||
])
|
||||
})
|
||||
})
|
||||
|
||||
describe('siteConfigMayRestrictHostKeys', () => {
|
||||
let dir: string
|
||||
|
||||
|
||||
@@ -4,6 +4,7 @@ import { readdir, readFile } from 'node:fs/promises'
|
||||
import { dirname, isAbsolute, join } from 'node:path'
|
||||
import { homedir, userInfo } from 'node:os'
|
||||
import { resolveSshConfigHomePath } from './ssh-config-path-expansion'
|
||||
import { expandIdentityFileTokens } from './ssh-identity-file-tokens'
|
||||
|
||||
export type SshResolvedConfig = {
|
||||
hostname: string
|
||||
@@ -204,7 +205,7 @@ export async function resolveWithSshG(host: string): Promise<SshResolvedConfig |
|
||||
|
||||
export function parseSshGOutput(stdout: string): SshResolvedConfig {
|
||||
const map = new Map<string, string>()
|
||||
const identityFiles: string[] = []
|
||||
const rawIdentityFiles: string[] = []
|
||||
|
||||
for (const line of stdout.split('\n')) {
|
||||
const spaceIdx = line.indexOf(' ')
|
||||
@@ -214,12 +215,19 @@ export function parseSshGOutput(stdout: string): SshResolvedConfig {
|
||||
const key = line.substring(0, spaceIdx).toLowerCase()
|
||||
const value = line.substring(spaceIdx + 1).trim()
|
||||
if (key === 'identityfile') {
|
||||
identityFiles.push(resolveSshConfigHomePath(value))
|
||||
// `IdentityFile none` loads no key; it is not a path.
|
||||
if (value.toLowerCase() !== 'none') {
|
||||
rawIdentityFiles.push(value)
|
||||
}
|
||||
} else {
|
||||
map.set(key, value)
|
||||
}
|
||||
}
|
||||
|
||||
// Tokens need hostname/user/port, which ssh -G may print after identityfile lines.
|
||||
const identityFiles = rawIdentityFiles.map((value) =>
|
||||
resolveSshConfigHomePath(expandIdentityFileTokens(value, map))
|
||||
)
|
||||
return buildSshResolvedConfig(map, identityFiles)
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,69 @@
|
||||
import { homedir, hostname, userInfo } from 'node:os'
|
||||
|
||||
type TokenSource = (resolved: ReadonlyMap<string, string>) => string | undefined
|
||||
|
||||
// OpenSSH expands %d/%u from the passwd entry, which can differ from HOME.
|
||||
function localUserInfo(): { homedir: string; username: string } | undefined {
|
||||
try {
|
||||
return userInfo()
|
||||
} catch {
|
||||
return undefined
|
||||
}
|
||||
}
|
||||
|
||||
const TOKEN_SOURCES: Record<string, TokenSource> = {
|
||||
'%': () => '%',
|
||||
d: () => localUserInfo()?.homedir || homedir(),
|
||||
u: () => localUserInfo()?.username || process.env.USER || process.env.USERNAME || undefined,
|
||||
i: () => {
|
||||
const uid = process.getuid?.()
|
||||
return uid === undefined ? undefined : String(uid)
|
||||
},
|
||||
l: () => hostname(),
|
||||
L: () => hostname().split('.')[0],
|
||||
h: (resolved) => resolved.get('hostname'),
|
||||
n: (resolved) => resolved.get('host'),
|
||||
p: (resolved) => resolved.get('port'),
|
||||
r: (resolved) => resolved.get('user')
|
||||
}
|
||||
|
||||
/** ssh -G prints IdentityFile `%x` and `${NAME}` raw (OpenSSH expands them at connect time), e.g. `%d/key.pem`. */
|
||||
export function expandIdentityFileTokens(
|
||||
value: string,
|
||||
resolved: ReadonlyMap<string, string>
|
||||
): string {
|
||||
if (!value.includes('%') && !value.includes('${')) {
|
||||
return value
|
||||
}
|
||||
// One pass, like OpenSSH: a replacement is never scanned for further tokens.
|
||||
let expanded = ''
|
||||
for (let index = 0; index < value.length; index += 1) {
|
||||
const char = value[index]
|
||||
if (char === '$' && value[index + 1] === '{') {
|
||||
const end = value.indexOf('}', index + 2)
|
||||
const envValue = end === -1 ? undefined : process.env[value.slice(index + 2, end)]
|
||||
// OpenSSH refuses the path when NAME is unset; keep it raw so it reads as missing.
|
||||
if (envValue === undefined) {
|
||||
return value
|
||||
}
|
||||
expanded += envValue
|
||||
index = end
|
||||
continue
|
||||
}
|
||||
if (char !== '%') {
|
||||
expanded += char
|
||||
continue
|
||||
}
|
||||
const source = Object.hasOwn(TOKEN_SOURCES, value[index + 1] ?? '')
|
||||
? TOKEN_SOURCES[value[index + 1]]
|
||||
: undefined
|
||||
const replacement = source?.(resolved)
|
||||
// Unknown tokens (%C, %j, %k) need OpenSSH internals; keep the raw path rather than guess.
|
||||
if (replacement === undefined) {
|
||||
return value
|
||||
}
|
||||
expanded += replacement
|
||||
index += 1
|
||||
}
|
||||
return expanded
|
||||
}
|
||||
@@ -142,6 +142,9 @@ function readPrivateKey(
|
||||
if (read.problem) {
|
||||
skipped?.set(keyPath, read.problem)
|
||||
warnSkippedIdentityFileOnce(keyPath, read.problem)
|
||||
} else if (!EXPANDED_DEFAULT_KEY_PATHS.includes(resolveSshConfigHomePath(keyPath))) {
|
||||
// Absent defaults are normal; a configured key that is missing explains a later password prompt.
|
||||
skipped?.set(keyPath, 'it does not exist')
|
||||
}
|
||||
return undefined
|
||||
}
|
||||
|
||||
@@ -68,11 +68,15 @@ describe('SSH identity file reads', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('leaves non-auth errors and missing files unannotated', () => {
|
||||
it('names a missing configured key', () => {
|
||||
const skipped: SkippedIdentityFiles = new Map()
|
||||
expect(resolvePrivateKeys(targetFor(join(root, 'missing')), null, skipped)).toEqual([])
|
||||
expect(skipped.size).toBe(0)
|
||||
const missing = join(root, 'missing')
|
||||
expect(resolvePrivateKeys(targetFor(missing), null, skipped)).toEqual([])
|
||||
expect([...skipped]).toEqual([[missing, 'it does not exist']])
|
||||
})
|
||||
|
||||
it('leaves non-auth errors unannotated', () => {
|
||||
const skipped: SkippedIdentityFiles = new Map()
|
||||
resolvePrivateKeys(targetFor(join(root, 'directory')), null, skipped)
|
||||
const networkError = new Error('connect ECONNREFUSED')
|
||||
withSkippedIdentityFileDetail(networkError, skipped)
|
||||
|
||||
@@ -0,0 +1,84 @@
|
||||
// #33849: an ~/.ssh/config host whose IdentityFile uses OpenSSH tokens connects with that key, not a password prompt.
|
||||
// ORCA_E2E_ORCAD_CONVERT_HOST=docker; ORCA_E2E_ORCAD_CONVERT_TEMPLATE must be built for the Docker host.
|
||||
import { copyFileSync, mkdtempSync, rmSync } from 'node:fs'
|
||||
import os from 'node:os'
|
||||
import path from 'node:path'
|
||||
import { expect, test } from './helpers/orca-app'
|
||||
import { waitForSessionReady } from './helpers/store'
|
||||
import { managedServer } from './helpers/orcad-convert-flow'
|
||||
import { openSshHostSettings, seedIsolatedSshConfig } from './helpers/ssh-config-host-picker'
|
||||
import {
|
||||
ORCAD_CONVERT_HOST_ENV,
|
||||
startOrcadConvertHost,
|
||||
type OrcadConvertHost
|
||||
} from './helpers/orcad-convert-host'
|
||||
|
||||
const HOST = process.env[ORCAD_CONVERT_HOST_ENV]
|
||||
const TEMPLATE_SOURCE = process.env.ORCA_E2E_ORCAD_CONVERT_TEMPLATE
|
||||
|
||||
test.skip(
|
||||
HOST !== 'docker' || !TEMPLATE_SOURCE,
|
||||
`Set ${ORCAD_CONVERT_HOST_ENV}=docker and ORCA_E2E_ORCAD_CONVERT_TEMPLATE`
|
||||
)
|
||||
test.use({
|
||||
seedTestRepo: false,
|
||||
orcaAppExtraEnv: { ORCA_ORCAD_TEMPLATE_PATH: TEMPLATE_SOURCE ?? '', SSH_AUTH_SOCK: '' }
|
||||
})
|
||||
|
||||
let host: OrcadConvertHost | null = null
|
||||
let keyDir: string | null = null
|
||||
test.afterEach(() => {
|
||||
host?.cleanup()
|
||||
host = null
|
||||
if (keyDir) {
|
||||
rmSync(keyDir, { recursive: true, force: true })
|
||||
keyDir = null
|
||||
}
|
||||
})
|
||||
|
||||
test('an IdentityFile path with %r and %p tokens is the key Connect uses', async ({
|
||||
electronApp,
|
||||
orcaPage
|
||||
}, testInfo) => {
|
||||
// Starts a Docker host and installs Orca's server there.
|
||||
test.setTimeout(6 * 60_000)
|
||||
host = startOrcadConvertHost('docker', testInfo)
|
||||
keyDir = mkdtempSync(path.join(os.tmpdir(), 'orca-identity-tokens-'))
|
||||
// %r and %p expand to root and the mapped port; ssh -G prints them unexpanded.
|
||||
copyFileSync(host.input.identityFile!, path.join(keyDir, `id-root-${host.input.port}`))
|
||||
const alias = `orca-tokens-${host.input.port}`
|
||||
await seedIsolatedSshConfig(
|
||||
electronApp,
|
||||
[
|
||||
`Host ${alias}`,
|
||||
` HostName ${host.input.host}`,
|
||||
` Port ${host.input.port}`,
|
||||
' User root',
|
||||
` IdentityFile ${keyDir}/id-%r-%p`,
|
||||
' IdentitiesOnly yes',
|
||||
''
|
||||
].join('\n')
|
||||
)
|
||||
await waitForSessionReady(orcaPage)
|
||||
const targetId = await orcaPage.evaluate(async (configHost) => {
|
||||
const { target } = await window.api.ssh.addTarget({
|
||||
target: {
|
||||
label: configHost,
|
||||
configHost,
|
||||
host: configHost,
|
||||
port: 22,
|
||||
username: '',
|
||||
source: 'ssh-config'
|
||||
}
|
||||
})
|
||||
return target.id
|
||||
}, alias)
|
||||
const section = await openSshHostSettings(orcaPage)
|
||||
const card = section.locator(`[data-ssh-target-card][data-ssh-target-label="${alias}"]`)
|
||||
await card.hover()
|
||||
await card.getByRole('button', { name: 'Connect', exact: true }).click()
|
||||
await expect
|
||||
.poll(() => managedServer(orcaPage, targetId), { timeout: 4 * 60_000 })
|
||||
.toMatchObject({ kind: 'managed' })
|
||||
await expect(orcaPage.getByRole('dialog', { name: 'SSH Password' })).toHaveCount(0)
|
||||
})
|
||||
Reference in New Issue
Block a user