diff --git a/src/agent.ts b/src/agent.ts index 4e1133c..ccde8dd 100644 --- a/src/agent.ts +++ b/src/agent.ts @@ -23,6 +23,9 @@ export function canonicalTarget(path: string): string { } const guidance = 'agent session missing, expired, or for another project/environment — run `insta agent setup`' +/** No usable agent session for the project, looked up from the current directory. */ +export class AgentSessionMissing extends Error {} + export async function issueAgentSession(api: SessionApi, projectId?: string): Promise { const pair = generateKeyPairSync('ed25519') const client = mode?.client ?? detectAgent(false)?.client ?? 'unknown' @@ -62,7 +65,7 @@ export async function loadAgentSession(apiUrl: string, projectId: string, cwd = if (session.projectId !== projectId || session.apiUrl !== apiUrl.replace(/\/+$/, '') || !session.token || !session.privateKey || !Number.isFinite(Date.parse(session.expiresAt)) || Date.parse(session.expiresAt) <= Date.now()) throw new Error() return session - } catch { throw new Error(guidance) } + } catch { throw new AgentSessionMissing(guidance) } } // Which project a request is scoped to, when the path itself does not say. Some project-owned diff --git a/src/commands/compute.ts b/src/commands/compute.ts index 1c70074..096cbf5 100644 --- a/src/commands/compute.ts +++ b/src/commands/compute.ts @@ -1,4 +1,6 @@ -import { ApiClient, ApiError, requireProject } from '../api.js' +import { AgentApprovalRequired, ApiClient, ApiError, requireProject } from '../api.js' +import { AgentSessionMissing } from '../agent.js' +import { safeText } from '../config.js' import { info, printJson, handleApproval, relayExitCode, writeFileAtomicSync, resolveThroughSymlink } from '../util.js' import { resolveComputeServiceId, resolveSoleService, q, parseVolumeGib, parseCount } from './services.js' @@ -824,7 +826,7 @@ import { dirname, join } from 'node:path' import { execFileSync } from 'node:child_process' import { createHash, randomUUID } from 'node:crypto' import { - aliasFor, certifiesPublicKey, hasOwnedBlock, isSafeAlias, isSafeConfigValue, isSafeSSHHost, isSafeSSHUsername, isSafeTimestamp, isSSHCertificateRecord, mayWidenCAHost, parseCAPublicKey, planCertAuthority, renderConfigBlock, revertCertAuthority, upsertConfigBlock, type HostEntry, ownedBlock, ownedBlockIsFirst } from './ssh-config.js' + ALIAS_SUFFIX, aliasFor, certifiesPublicKey, hasOwnedBlock, isSafeAlias, isSafeConfigValue, isSafeSSHHost, isSafeSSHUsername, isSafeTimestamp, isSSHCertificateRecord, mayWidenCAHost, parseCAPublicKey, planCertAuthority, renderConfigBlock, revertCertAuthority, upsertConfigBlock, type HostEntry, ownedBlock, ownedBlockIsFirst } from './ssh-config.js' /** Where this CLI keeps its own SSH material. Deliberately NOT ~/.ssh: we never * touch a key the user already had, and a dedicated key pairs with @@ -1068,6 +1070,7 @@ type MintedCert = CertResponse & { staged: StagedCertificate } async function mintCert(api: ApiClient, projectId: string, serviceId: string, publicKey: string, alias: string, signal?: AbortSignal): Promise { const res = await api.rawRequest('POST', `/projects/${projectId}/services/${serviceId}/ssh-cert`, { publicKey }, { signal }) + if (res.status === 202 && res.body?.status === 'approval_required') throw new AgentApprovalRequired(res.body) if (res.status < 200 || res.status >= 300) { throw new ApiError(res.status, res.body?.error ?? 'could not issue an ssh certificate') } @@ -1522,6 +1525,28 @@ function readUserText(path: string): string { return text } +/** Another process holds this alias's renewal lock and is renewing it now. */ +class RenewalInProgress extends Error {} + +/** The one line a missing or expired certificate that could not be renewed gets: the cause and the fix. */ +export function renewalFailureNotice(alias: string, err: unknown): string { + const head = `insta: the SSH certificate for ${alias} is missing or expired` + const fix = `insta compute ssh ${alias.slice(0, -ALIAS_SUFFIX.length)}` + if (err instanceof RenewalInProgress) { + return `${head} and another ssh may be renewing it right now. Retry in a moment, or run "${fix}".` + } + if (err instanceof AgentSessionMissing) { + return `${head}. Agent mode found no agent session for its project in this directory. Run ssh from the project directory, or ask a person to run "${fix}".` + } + if (err instanceof AgentApprovalRequired) { + return `${head} and renewing it needs approval. ${safeText(err.message.replace(/\s+/g, ' ')).trim()} Retry after it is approved.` + } + if (err instanceof ApiError && err.status === 403 && err.message.endsWith('denied by agent policy')) { + return `${head} and this project's agent policy does not allow renewing it.` + } + return `${head} and could not be renewed. Run "${fix}" to renew it.` +} + /** * The renewal hook OpenSSH runs while PARSING the config, before it connects. * @@ -1539,14 +1564,18 @@ function readUserText(path: string): string { * rule __update-check relies on), so the fast path is genuinely local only * when the hook enters through that name. * - Silent and fail-safe. An unlinked directory, an expired login or a network - * outage must not print anything or fail the parse: the existing certificate - * stays in place and the login then fails with SSH's own message, not a CLI - * error spliced into the middle of an ssh session. + * outage must not fail the parse: the existing certificate stays in place. + * Only an already expired one earns a single stderr line saying why. */ export async function ensureCertForAlias(alias: string, timeoutMs = RENEWAL_REQUEST_TIMEOUT_MS): Promise { let release: (() => void) | undefined + let explainOnExit = false + let cause: unknown try { if (!isSafeAlias(alias)) return + const managed = readAliasStore() + if (!managed[alias]) return // not an alias this CLI manages: stay silent + explainOnExit = true // set before the repair, which can throw too // Repair BEFORE the renewal gate, because the stanza can lag the store // with no certificate due: an alias set up before the Port line existed // dials :22 on every connection, and waiting for its certificate to age @@ -1554,13 +1583,16 @@ export async function ensureCertForAlias(alias: string, timeoutMs = RENEWAL_REQU // predicate is content drift, so the common case -- an up-to-date stanza -- // is one file read and a string compare, with no lock taken; only a stale // stanza takes the alias-store lock, re-checks under it and rewrites. - if (configBlockStale(readAliasStore())) { + if (configBlockStale(managed)) { withAliasStoreLock(() => { const store = readAliasStore() if (store[alias] && configBlockStale(store)) installConfigBlock(store) }, { waitMs: KNOWN_HOSTS_LOCK_WAIT_MS }) } - if (!certNeedsRenewal(instaCertPath(alias))) return + if (!certNeedsRenewal(instaCertPath(alias))) { + explainOnExit = false // the certificate works, so no second check on the way out + return + } // An IDE opens several connections at once and `scp` adds more, so the // near-expiry certificate is observed by every one of them simultaneously @@ -1572,7 +1604,10 @@ export async function ensureCertForAlias(alias: string, timeoutMs = RENEWAL_REQU // a lock that can hang `ssh` itself -- strictly worse than the duplicate // request it would prevent. The loser simply lets the winner renew. release = acquireRenewalLock(alias) - if (!release) return + if (!release) { + cause = new RenewalInProgress() + return + } // Re-checked after the lock. Without this the second process through the // door renews again over the certificate the first just wrote -- the lock // would serialise the stampede instead of collapsing it. @@ -1671,10 +1706,14 @@ export async function ensureCertForAlias(alias: string, timeoutMs = RENEWAL_REQU } finally { out.staged.discard() } - } catch { - // Deliberately swallowed. See above. + } catch (err) { + cause = err } finally { release?.() + // Silent unless this ssh is about to fail: any exit that left the certificate expired says why. + if (explainOnExit && certNeedsRenewal(instaCertPath(alias), { marginMs: 0 })) { + process.stderr.write(renewalFailureNotice(alias, cause) + '\n') + } } } diff --git a/src/commands/ssh-config.ts b/src/commands/ssh-config.ts index 79a76ae..0df540d 100644 --- a/src/commands/ssh-config.ts +++ b/src/commands/ssh-config.ts @@ -186,9 +186,7 @@ export function renderConfigBlock(o: ConfigBlockOpts): string { // full ssh-agent gets intermittent, unexplainable auth failures. ' IdentitiesOnly yes', ) - // Connection multiplexing collapses scp, an IDE's several connections and a - // second terminal onto ONE connection; without it a single developer can - // reach the per-service session cap in an afternoon. + // Multiplexing spares a handshake per ssh, and 60s keeps a master from outliving a pod that scaled to zero. // // The socket is keyed on %C -- a hash of (local host, remote host, port, // user) -- not %r@%h:%p. A ControlPath is a Unix-domain socket, whose path @@ -208,7 +206,7 @@ export function renderConfigBlock(o: ConfigBlockOpts): string { lines.push( ' ControlMaster auto', ' ControlPath ~/.insta/ssh/cm-%C', - ' ControlPersist 10m', + ' ControlPersist 60s', ) } if (o.ensureCertCommand) { diff --git a/src/config.ts b/src/config.ts index 4bbdb54..3abdb37 100644 --- a/src/config.ts +++ b/src/config.ts @@ -298,9 +298,10 @@ async function readLinkPlane(root: string): Promise<{ projectId: string; apiUrl: } /** Text safe to echo to a terminal: C0 and C1 control characters and DEL removed — ESC and the - * single-byte C1 introducers (U+009B CSI among them) alike, so no escape sequence survives. */ -function safeText(text: string): string { - return String(text).replace(/[\u0000-\u001f\u007f-\u009f]/g, '') + * single-byte C1 introducers (U+009B CSI among them) alike, so no escape sequence survives. + * Bidi marks, embeddings, overrides and isolates go too, so nothing can reorder what is shown. */ +export function safeText(text: string): string { + return String(text).replace(/[\u0000-\u001f\u007f-\u009f‎‏‪-‮⁦-⁩]/g, '') } /** A control-plane URL safe to persist and to print: control characters removed, surrounding diff --git a/test/ssh-config.test.ts b/test/ssh-config.test.ts index 2722e99..9123875 100644 --- a/test/ssh-config.test.ts +++ b/test/ssh-config.test.ts @@ -477,6 +477,11 @@ describe.skipIf(!ssh || process.platform === 'win32')('effective configuration ( expect(cp.length, `ControlPath is ${cp.length} bytes (limit 104): ${cp}`).toBeLessThan(104) }) + it('drops a multiplexed connection a minute after its last session', () => { + // A master outliving the 300s scale-to-zero idle stays pinned to a pod that is gone. + expect(effective('api.insta', rendered()).get('controlpersist')).toBe('60') + }) + it('does not let a % in the path reach the ControlPath tokens', () => { // ControlPath carries tokens we MEANT, and `ssh -G` does expand that one -- // so it is the check that the escaping did not spill outside the two path @@ -966,13 +971,18 @@ describe('renewal hook is silent and fail-safe', () => { await quietly('api.insta') }) - // A record exists and the certificate is missing, so this path goes all the - // way to the platform -- which is not there. Everything downstream of the - // certificate check lives inside the same silent boundary. - it('says nothing when the platform cannot be reached', async () => { + // No certificate and no platform, so this ssh will fail: the hook says why, on stderr only. + it('says only why, in one stderr line, when the platform cannot be reached', async () => { writeAliasStore({ 'api.insta': { projectId: 'p1', branch: 'main', serviceId: 's1', host: 'h', username: 'u' } }) expect(existsSync(instaCertPath('api.insta'))).toBe(false) - await quietly('api.insta') + const log = vi.spyOn(console, 'log').mockImplementation(() => {}) + const err = vi.spyOn(console, 'error').mockImplementation(() => {}) + const out = vi.spyOn(process.stdout, 'write').mockImplementation(() => true) + const errOut = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + await expect(computeSSH(undefined, { ensureCert: 'api.insta' })).resolves.toBeUndefined() + for (const s of [log, err, out]) expect(s, 'the renewal hook printed into the ssh session').not.toHaveBeenCalled() + expect(errOut.mock.calls.map((c) => String(c[0])).join('')).toBe( + 'insta: the SSH certificate for api.insta is missing or expired and could not be renewed. Run "insta compute ssh api" to renew it.\n') }) it('says nothing, and touches nothing, for an alias that is not ours', async () => { @@ -1288,7 +1298,7 @@ describe('the generated config works on Windows, where multiplexing does not', ( for (const platform of ['darwin', 'linux'] as const) { const out = renderConfigBlock({ entries: [entry()], identityFile: '/home/dev/.insta/ssh/id_ed25519', knownHostsFile: KNOWN_HOSTS, platform }) expect(out, `${platform} lost connection multiplexing`).toContain('ControlMaster auto') - expect(out).toContain('ControlPersist 10m') + expect(out).toContain('ControlPersist 60s') // Keyed on %C, never %r@%h:%p: a Unix-domain socket path caps at 104 bytes // and the route-key user plus the gateway host overflowed it. expect(out, `${platform} ControlPath must be the fixed-length %C form`).toContain('ControlPath ~/.insta/ssh/cm-%C') diff --git a/test/ssh-orchestration.test.ts b/test/ssh-orchestration.test.ts index f693b25..593f5a7 100644 --- a/test/ssh-orchestration.test.ts +++ b/test/ssh-orchestration.test.ts @@ -11,7 +11,9 @@ import { chmodSync, copyFileSync, existsSync, linkSync, lstatSync, mkdirSync, mk import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { execFileSync, spawn } from 'node:child_process' -import { computeSSH, installCertAuthority, instaCertPath, instaAliasStorePath, instaKeyPath, writeAliasStore, readAliasStore, validateCertResponse, acquireLockFile, acquireRenewalLock, ensureCertForAlias, hostPatternFor, stageCertificate } from '../src/commands/compute.js' +import { computeSSH, installCertAuthority, instaCertPath, instaAliasStorePath, instaKeyPath, writeAliasStore, readAliasStore, validateCertResponse, acquireLockFile, acquireRenewalLock, ensureCertForAlias, hostPatternFor, stageCertificate, renewalFailureNotice } from '../src/commands/compute.js' +import { configureAgent } from '../src/agent.js' +import { ApiError, AgentApprovalRequired } from '../src/api.js' import { isSSHCertificateRecord, mayWidenCAHost, parseCAPublicKey, BLOCK_BEGIN, } from '../src/commands/ssh-config.js' import { canSymlink } from './support/can-symlink.js' @@ -103,6 +105,13 @@ const EXPIRED_CERT = !keygen ? '' : (() => { execFileSync('ssh-keygen', ['-q', '-s', join(fixtures, 'ca'), '-I', 'stale', '-n', 'u-svc-1', '-V', '-2h:-1h', `${old}.pub`]) return readFileSync(`${old}-cert.pub`, 'utf8').trim() })() +/** Still valid but inside the 5 minute renewal margin; minted on call so a slow run cannot outlive it. */ +const nearExpiryCert = () => { + const near = join(mkdtempSync(join(fixtures, 'near-')), 'key') + execFileSync('ssh-keygen', ['-q', '-t', 'ed25519', '-N', '', '-f', near, '-C', 'near@insta']) + execFileSync('ssh-keygen', ['-q', '-s', join(fixtures, 'ca'), '-I', 'near', '-n', 'u-svc-1', '-V', '-1h:+2m', `${near}.pub`]) + return readFileSync(`${near}-cert.pub`, 'utf8').trim() +} /** A REAL certificate, in date, signed by the same CA -- for a key that is not * ours. `ssh-keygen -L` is perfectly happy with it, and it authenticates * nothing on this machine. */ @@ -1480,6 +1489,41 @@ d('an automatic renewal moves the alias with the certificate', () => { expect(readFileSync(instaCertPath('api.insta'), 'utf8'), 'the fresh certificate was replaced').toBe(certBefore) }) + it.skipIf(process.platform === 'win32')('repairs a stanza still keeping connections for ten minutes', async () => { + installTheKeyCertWasIssuedFor() + const { deps: d } = deps({ installCA: undefined, installConfig: undefined }) + await computeSSH('api', { setup: true }, d) + const cfgPath = configPath() + writeFileSync(cfgPath, readFileSync(cfgPath, 'utf8').replace(/ ControlPersist \S+/, ' ControlPersist 10m')) + expect(readFileSync(cfgPath, 'utf8'), 'no ControlPersist line to age').toContain(' ControlPersist 10m') + + await renew(() => { throw new Error('the hook minted for a certificate that was not due') }) + + const lines = readFileSync(cfgPath, 'utf8').split('\n') + expect(lines, 'the ten-minute stanza was not repaired').toContain(' ControlPersist 60s') + expect(lines).not.toContain(' ControlPersist 10m') + }) + + // Root can write into a 0500 directory, so the blocked repair this needs only happens for other users. + it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)('says why when an expired certificate cannot even get its stale stanza repaired', async () => { + await anInstalledAlias() + const cfgPath = configPath() + writeFileSync(cfgPath, readFileSync(cfgPath, 'utf8').replace(/ ControlPersist \S+/, ' ControlPersist 10m')) + const sshDir = join(home, '.ssh') + chmodSync(sshDir, 0o500) // the repair's write into ~/.ssh fails + const err = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + let said = '' + try { + await renew(() => { throw new Error('reached the platform past a failed repair') }) + said = err.mock.calls.map((c) => String(c[0])).join('') + } finally { + err.mockRestore() + chmodSync(sshDir, 0o700) + } + expect(readFileSync(cfgPath, 'utf8'), 'the repair was not actually blocked, so this proves nothing').toContain(' ControlPersist 10m') + expect(said).toBe('insta: the SSH certificate for api.insta is missing or expired and could not be renewed. Run "insta compute ssh api" to renew it.\n') + }) + it('moves a block that slid below other configuration back to the top', async () => { // OpenSSH takes the first obtained value per keyword, so a `Host *` that // ended up above our block -- a dotfiles tool, a hand edit -- overrides its @@ -1809,3 +1853,100 @@ d('the key sent for certification is derived from the PRIVATE key', () => { expect(readFileSync(instaCertPath('api.insta'), 'utf8')).toBe('the-working-certificate\n') }) }) + +d('an expired certificate that could not be renewed says why', () => { + const run = async (cert: string, respond: () => Promise<{ status: number; body: unknown }>) => { + mkdirSync(join(home, '.insta', 'ssh'), { recursive: true }) + writeFileSync(instaCertPath('api.insta'), cert + '\n') + writeAliasStore({ 'api.insta': { projectId: 'proj-1', serviceId: 'svc-1', host: 'ssh.us-west-1.compute.example', username: 'u-svc-1' } }) + let asked = 0 + const mod = await import('../src/api.js') + const fetchImpl = async () => { asked++; const r = await respond(); return { status: r.status, text: async () => JSON.stringify(r.body) } } + const load = vi.spyOn(mod.ApiClient, 'load').mockResolvedValue(new mod.ApiClient({ apiUrl: 'https://example.invalid', accessToken: 't' } as never, fetchImpl as never)) + const err = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + try { + await ensureCertForAlias('api.insta', 2_000) + return { said: err.mock.calls.map((c) => String(c[0])).join(''), asked } + } finally { + err.mockRestore() + load.mockRestore() + } + } + + it('says why when an expired certificate could not be renewed', async () => { + const { said } = await run(EXPIRED_CERT, () => Promise.reject(new Error('network down'))) + expect(said).toBe('insta: the SSH certificate for api.insta is missing or expired and could not be renewed. Run "insta compute ssh api" to renew it.\n') + }) + + it('stays silent while the certificate still works', async () => { + const { said, asked } = await run(nearExpiryCert(), () => Promise.reject(new Error('network down'))) + expect(asked, 'renewal was never attempted, so silence proves nothing').toBe(1) + expect(said).toBe('') + }) + + it('names the approval a restricted project is waiting on, in one line', async () => { + const body = { status: 'approval_required', message: 'Approval required.\nReview it at https://example.invalid/approvals/a1' } + const { said } = await run(EXPIRED_CERT, () => Promise.resolve({ status: 202, body })) + expect(said).toContain('needs approval') + expect(said).toContain('https://example.invalid/approvals/a1') + expect(said.split('\n'), 'the notice spans more than one line').toHaveLength(2) + }) + + it('strips terminal control sequences from the platform approval text', async () => { + const body = { status: 'approval_required', message: 'Approval required.\u001b[2J\u009b31m Review it at https://example.invalid/approvals/a1' } + const { said } = await run(EXPIRED_CERT, () => Promise.resolve({ status: 202, body })) + expect(said, 'an ESC reached the terminal').not.toContain('\u001b') + expect(said, 'a C1 CSI reached the terminal').not.toContain('\u009b') + expect(said).toContain('https://example.invalid/approvals/a1') + expect(said.split('\n'), 'the notice spans more than one line').toHaveLength(2) + }) + + it('strips bidi controls that could reorder the approval text on screen', async () => { + const body = { status: 'approval_required', message: 'Approval required. ‮Review it‬ at ⁦https://example.invalid/approvals/a1⁩' } + const { said } = await run(EXPIRED_CERT, () => Promise.resolve({ status: 202, body })) + expect(said, 'a bidi control reached the terminal').not.toMatch(/[‎‏‪-‮⁦-⁩]/) + expect(said).toContain('https://example.invalid/approvals/a1') + }) + + it('tells an agent with no session where to run ssh from', async () => { + configureAgent({ source: 'cli-detected', client: 'claude-code' }) + try { + const { said, asked } = await run(EXPIRED_CERT, () => Promise.reject(new Error('the request was sent'))) + expect(asked, 'a request was sent without an agent session').toBe(0) + expect(said).toContain('no agent session for its project in this directory') + } finally { + configureAgent(null) + } + }) + + it('says another ssh is renewing it when it loses the renewal lock', async () => { + mkdirSync(join(home, '.insta', 'ssh'), { recursive: true }) + const held = acquireRenewalLock('api.insta') + expect(held, 'the test could not take the renewal lock').toBeDefined() + try { + const { said, asked } = await run(EXPIRED_CERT, () => Promise.reject(new Error('the loser sent a request'))) + expect(asked, 'the loser of the lock race reached the platform').toBe(0) + expect(said).toBe('insta: the SSH certificate for api.insta is missing or expired and another ssh may be renewing it right now. Retry in a moment, or run "insta compute ssh api".\n') + } finally { + held!() + } + }) + + it('says why when a renewal is abandoned after the mint', async () => { + installTheKeyCertWasIssuedFor() + const good = { certificate: CERT, host: 'ssh.us-west-1.compute.example', username: 'u-svc-1', expiresAt: '2026-09-14T22:00:00Z', caPublicKey: CA } + // The record vanishes while the mint is in flight, so the commit gives up. + const { said, asked } = await run(EXPIRED_CERT, () => { writeAliasStore({}); return Promise.resolve({ status: 200, body: good }) }) + expect(asked, 'the mint was never reached, so this is not the abandoned-commit path').toBe(1) + expect(readFileSync(instaCertPath('api.insta'), 'utf8'), 'the abandoned renewal installed a certificate').toBe(EXPIRED_CERT + '\n') + expect(said).toBe('insta: the SSH certificate for api.insta is missing or expired and could not be renewed. Run "insta compute ssh api" to renew it.\n') + }) + + it('names a policy denial only when the platform says the agent policy denied it', () => { + const policy = new ApiError(403, 'compute.shell, secrets.read denied by agent policy') + const other = new ApiError(403, 'a shell needs an interactive login: run `insta login --oauth`') + expect(renewalFailureNotice('api.insta', policy)).toContain("this project's agent policy does not allow renewing it") + expect(renewalFailureNotice('api.insta', other)).toContain('could not be renewed') + expect(renewalFailureNotice('api.insta', new AgentApprovalRequired({ message: 'x' }))).toContain('needs approval') + }) +})