From fcf4fd3cf59fa6078f7391220e79833a160476da Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 30 Jul 2026 23:41:30 -0700 Subject: [PATCH] fix(ssh): gate the remaining #11297 config-backed branches on host-block evidence Why: `ssh -G ` still exits 0 with defaults, so the GitHub restricted-shell probe read the local account instead of a stored `git` username, and a defaulted `gssapiauthentication no` discarded a stored Kerberos flag. A resolved `yes` still wins so distro-wide defaults keep working. Also treats an undeterminable local account as no evidence, and matches ssh2's first-entry rule for the eager private key. --- src/main/ssh/ssh-connection.test.ts | 89 +++++++++++++++++++ src/main/ssh/ssh-connection.ts | 13 +-- src/main/ssh/ssh-g-host-block-match.test.ts | 17 ++++ src/main/ssh/ssh-g-host-block-match.ts | 21 +++-- .../ssh/ssh-multi-key-authentication.test.ts | 14 +++ .../ssh/ssh-private-key-authentication.ts | 9 +- 6 files changed, 146 insertions(+), 17 deletions(-) diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index b73cc1e1db3..79c5d93ed6f 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -180,6 +180,15 @@ function createTarget(overrides?: Partial): SshTarget { } } +const LOCAL_ACCOUNT = 'localdev' + +// Why: `ssh -G` echoes the local account as `user` for an alias with no Host block. +function stubLocalAccount(): void { + vi.stubEnv('USER', LOCAL_ACCOUNT) + vi.stubEnv('LOGNAME', LOCAL_ACCOUNT) + vi.stubEnv('USERNAME', LOCAL_ACCOUNT) +} + function createResolvedConfig(overrides?: Partial): SshResolvedConfig { return { hostname: 'example.com', @@ -1188,6 +1197,30 @@ describe('SshConnection', () => { expect(conn.usesSystemSshTransport()).toBe(true) }) + it('keeps the stored git username when the GitHub alias no longer has a Host block', async () => { + stubLocalAccount() + vi.mocked(resolveWithSshG).mockResolvedValueOnce( + createResolvedConfig({ hostname: 'github.com', user: LOCAL_ACCOUNT }) + ) + spawnSystemSshCommandMock.mockImplementation(() => + createFailingSystemCommandChannel(1, 'Invalid command: echo ORCA-SYSTEM-SSH-OK') + ) + const conn = new SshConnection( + createTarget({ + source: 'ssh-config', + configHost: 'github.com', + host: 'github.com', + username: 'git' + }), + createCallbacks() + ) + + await conn.connect() + + expect(conn.getState().status).toBe('connected') + expect(conn.usesSystemSshTransport()).toBe(true) + }) + it('accepts GitHub restricted-shell SSH probes with resolved host and target username', async () => { vi.mocked(resolveWithSshG).mockResolvedValueOnce( createResolvedConfig({ hostname: 'github.com', user: undefined }) @@ -1573,6 +1606,62 @@ describe('SshConnection', () => { expect(spawnSystemSshCommandMock).not.toHaveBeenCalled() }) + it('keeps the stored GSSAPI flag when the alias no longer has a Host block', async () => { + stubLocalAccount() + // `ssh -G krb-box` with no matching block: echoed alias, local user, port 22, GSSAPI off. + vi.mocked(resolveWithSshG).mockResolvedValue( + createResolvedConfig({ + hostname: 'krb-box', + user: LOCAL_ACCOUNT, + proxyUseFdpass: false, + gssapiAuthentication: false + }) + ) + const conn = new SshConnection( + createTarget({ + source: 'ssh-config', + configHost: 'krb-box', + host: 'krb-box', + gssapiAuthentication: true + }), + createCallbacks() + ) + + await conn.connect() + + expect(conn.usesSystemSshTransport()).toBe(true) + expect(spawnSystemSshCommandMock).toHaveBeenCalledWith( + expect.objectContaining({ gssapiAuthentication: true }), + 'echo ORCA-SYSTEM-SSH-OK', + expect.objectContaining({ gssapiOnly: true, wrapCommand: false }) + ) + }) + + it('still probes GSSAPI from a distro-wide default when the alias has no Host block', async () => { + stubLocalAccount() + vi.mocked(resolveWithSshG).mockResolvedValue( + createResolvedConfig({ + hostname: 'krb-box', + user: LOCAL_ACCOUNT, + proxyUseFdpass: false, + gssapiAuthentication: true + }) + ) + const conn = new SshConnection( + createTarget({ source: 'ssh-config', configHost: 'krb-box', host: 'krb-box' }), + createCallbacks() + ) + + await conn.connect() + + expect(conn.usesSystemSshTransport()).toBe(true) + expect(spawnSystemSshCommandMock).toHaveBeenCalledWith( + expect.anything(), + 'echo ORCA-SYSTEM-SSH-OK', + expect.objectContaining({ gssapiOnly: true, wrapCommand: false }) + ) + }) + it('falls back to system SSH after an ssh2 auth failure when resolved config enables GSSAPI', async () => { connectBehavior = 'error' connectErrorMessage = 'All configured authentication methods failed' diff --git a/src/main/ssh/ssh-connection.ts b/src/main/ssh/ssh-connection.ts index c1b73bbbb2d..3cd4ad7f6b3 100644 --- a/src/main/ssh/ssh-connection.ts +++ b/src/main/ssh/ssh-connection.ts @@ -19,6 +19,7 @@ import { } from './ssh-system-fallback' import { resolveWithSshG, type SshResolvedConfig } from './ssh-config-parser' import { removeControlSocketPath } from './ssh-control-socket' +import { hasOpenSshHostBlockMatch } from './ssh-g-host-block-match' import { isOpenSshConfigBackedTarget } from './system-ssh-args' import { INITIAL_RETRY_ATTEMPTS, @@ -85,8 +86,8 @@ function isGitHubRestrictedShellProbeSuccess( } const effectiveUser = ( - isOpenSshConfigBackedTarget(target) && resolvedConfig - ? resolvedConfig.user?.trim() || target.username?.trim() + hasOpenSshHostBlockMatch(target, resolvedConfig) + ? resolvedConfig?.user?.trim() || target.username?.trim() : target.username?.trim() || resolvedConfig?.user?.trim() )?.toLowerCase() if (effectiveUser !== 'git') { @@ -637,10 +638,12 @@ export class SshConnection { return } // Why: ssh2 lacks gssapi-with-mic; GSSAPIAuthentication hosts try Kerberos SSO via system OpenSSH first, then fall through to key/credential auth. + // Why: without Host-block evidence a resolved `no` is just the /etc/ssh default, so it must not discard the stored flag; a resolved `yes` still wins so distro-wide defaults keep working. if ( - isOpenSshConfigBackedTarget(this.target) && resolved - ? resolved.gssapiAuthentication === true - : this.target.gssapiAuthentication === true + hasOpenSshHostBlockMatch(this.target, resolved) + ? resolved?.gssapiAuthentication === true + : this.target.gssapiAuthentication === true || + (isOpenSshConfigBackedTarget(this.target) && resolved?.gssapiAuthentication === true) ) { try { await this.doSystemSshProbeWithControlMasterRetry(connectGeneration, resolved, true) diff --git a/src/main/ssh/ssh-g-host-block-match.test.ts b/src/main/ssh/ssh-g-host-block-match.test.ts index ad2e0e13300..a5ab136270a 100644 --- a/src/main/ssh/ssh-g-host-block-match.test.ts +++ b/src/main/ssh/ssh-g-host-block-match.test.ts @@ -188,6 +188,23 @@ describe('ssh -G host-block matching', () => { }) }) + // A launchd/systemd-spawned main process has no USER/LOGNAME/USERNAME, so the + // resolved user proves nothing and the stored fields have to win. + it('ignores the resolved user when the local account cannot be determined', () => { + vi.stubEnv('USER', '') + vi.stubEnv('LOGNAME', '') + vi.stubEnv('USERNAME', '') + + const config = buildConnectConfig( + storedTarget({ jumpHost: undefined }), + unmatchedDefaults({ user: 'whoever' }), + { includeAgent: false, includePrivateKey: true } + ) + + expect(config.username).toBe('deploy') + expect(config.host).toBe('10.0.0.5') + }) + it('keeps manual targets on their stored fields regardless of ssh -G output', () => { const config = buildConnectConfig( storedTarget({ source: 'manual', configHost: undefined }), diff --git a/src/main/ssh/ssh-g-host-block-match.ts b/src/main/ssh/ssh-g-host-block-match.ts index bfe85c57b41..01dcb4806b0 100644 --- a/src/main/ssh/ssh-g-host-block-match.ts +++ b/src/main/ssh/ssh-g-host-block-match.ts @@ -11,16 +11,17 @@ function normalize(value: string | undefined): string { return value?.trim().toLowerCase() ?? '' } -// Why: `ssh -G` fills `user` from the local account whenever no User directive matched. -function localAccountName(): string { - const fromEnv = process.env.USER || process.env.LOGNAME || process.env.USERNAME +// Why: `ssh -G` fills `user` from the local account whenever no User directive +// matched; null means the local account is unknown, so `user` carries no verdict. +function localAccountName(): string | null { + const fromEnv = normalize(process.env.USER || process.env.LOGNAME || process.env.USERNAME) if (fromEnv) { - return normalize(fromEnv) + return fromEnv } try { - return normalize(userInfo().username) + return normalize(userInfo().username) || null } catch { - return '' + return null } } @@ -37,7 +38,10 @@ function localAccountName(): string { * * Known limitation: the signals below read the *effective* config, so a wildcard * `Host *` supplying User/Port/ProxyCommand — or `CanonicalizeHostname yes` — - * still reads as a match for an alias whose own block is gone. + * still reads as a match for an alias whose own block is gone. The reverse + * verdict falls back to the stored snapshot, whose IdentityFile is used verbatim + * (only `~` is expanded), so a stored path with OpenSSH tokens (%d/%h/%r) stays + * unreadable — pre-existing behaviour, unchanged here. */ export function hasOpenSshHostBlockMatch( target: HostBlockMatchTarget, @@ -58,6 +62,7 @@ export function hasOpenSshHostBlockMatch( if (resolved.proxyCommand || resolved.proxyJump) { return true } + const localAccount = localAccountName() const user = normalize(resolved.user) - return user !== '' && user !== localAccountName() + return localAccount !== null && user !== '' && user !== localAccount } diff --git a/src/main/ssh/ssh-multi-key-authentication.test.ts b/src/main/ssh/ssh-multi-key-authentication.test.ts index 659edbecff6..9b30f4f6649 100644 --- a/src/main/ssh/ssh-multi-key-authentication.test.ts +++ b/src/main/ssh/ssh-multi-key-authentication.test.ts @@ -202,6 +202,20 @@ describe('ordered SSH private-key authentication', () => { expect(getPassphrasePrivateKeyPath(config)).toBe('/keys/encrypted-first') }) + it('falls back to the first identity when every candidate is encrypted', () => { + mockEncryptedKeyPaths(['/keys/encrypted-first', '/keys/encrypted-second']) + const config = buildConnectConfig( + makeTarget(), + makeResolved({ identityFile: ['/keys/encrypted-first', '/keys/encrypted-second'] }), + { includeAgent: false, includePrivateKey: true } + ) + + // Why: no parseable candidate means the eager parse must still throw, so the + // passphrase prompt fires instead of silently dropping to agent-only auth. + expect(config.privateKey).toEqual(Buffer.from('/keys/encrypted-first')) + expect(getPassphrasePrivateKeyPath(config)).toBe('/keys/encrypted-first') + }) + it('still hands ssh2 an encrypted key when it is the only identity', () => { mockEncryptedKeyPaths(['/keys/encrypted-only']) const config = buildConnectConfig( diff --git a/src/main/ssh/ssh-private-key-authentication.ts b/src/main/ssh/ssh-private-key-authentication.ts index 193c491a6f6..3704e97cf61 100644 --- a/src/main/ssh/ssh-private-key-authentication.ts +++ b/src/main/ssh/ssh-private-key-authentication.ts @@ -41,16 +41,17 @@ function isUsableAsEagerPrivateKey(key: PrivateKeyFile, passphrase?: string | Bu if (parsed instanceof Error) { return false } - const parsedKeys = Array.isArray(parsed) ? parsed : [parsed] - return parsedKeys.some( - (entry) => typeof entry?.isPrivateKey === 'function' && entry.isPrivateKey() - ) + // Why: ssh2 keeps only the first entry of a multi-key parse (client.js `privateKey[0]`). + const eager = Array.isArray(parsed) ? parsed[0] : parsed + return typeof eager?.isPrivateKey === 'function' && eager.isPrivateKey() } // Why: ssh2's Client.connect parses config.privateKey before any auth runs and // throws on an encrypted/unparseable one, so seeding it with the first candidate // would abort the connect instead of letting authHandler try the other keys. // Falling back to keys[0] keeps the passphrase prompt for an encrypted-only list. +// `config.passphrase` is still unset here on the first connect — callers only fill +// it after a prompt — so an encrypted key can only be picked on a retry pass. function selectEagerPrivateKey(config: ConnectConfig, keys: PrivateKeyFile[]): PrivateKeyFile { return keys.find((key) => isUsableAsEagerPrivateKey(key, config.passphrase)) ?? keys[0]! }