From 11c323b715d6152b033fc876fa53a96773ad58ae Mon Sep 17 00:00:00 2001 From: Benjamin Diedrichsen Date: Thu, 30 Jul 2026 14:33:26 +0200 Subject: [PATCH] [keyman] phase 2: guard the directories nothing creates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit encrypt read ~/.ssh and the tmp directory, and decrypt read /keys, with no existsSync between them. main.ts created vaultRoot and tmpDir but never keysDir, so decrypt on a fresh vault threw ENOENT instead of printing the "no encrypted keys" message it already had — the message was unreachable until something else created the directory. Both functions now fall through to their warning. main.ts creates all three directories, 0700: the vault holds the age identity and tmp holds plaintext private keys. age spawns go through runTool, which separates "not installed" (ENOENT, whose message is `spawn age ENOENT`) from "age refused" (whose reason is on stderr and nowhere in the thrown message). Tested against real processes, not a mocked execa — the shape of the failure is the point. list.ts kept statSync rather than switching to withFileTypes as planned: withFileTypes reports a symlinked key directory as a link and would have silently dropped it. `throwIfNoEntry: false` fixes the dangling-symlink throw and keeps following the good ones. Both cases now have a test. Also deletes the three debug logs (encrypt.ts printed both key arrays, decrypt.ts printed every candidate path from inside a filter). Co-Authored-By: Claude Opus 5 (1M context) --- packages/keyman/src/keyman.decrypt.ts | 13 +++++---- packages/keyman/src/keyman.encrypt.ts | 29 ++++++++++++------- packages/keyman/src/keyman.list.ts | 7 +++-- packages/keyman/src/keyman.main.ts | 8 ++++-- packages/keyman/src/keyman.utils.ts | 33 +++++++++++++++++++++ packages/keyman/tests/decrypt.test.ts | 17 +++++++++++ packages/keyman/tests/encrypt.test.ts | 27 ++++++++++++++++++ packages/keyman/tests/list.test.ts | 19 +++++++++++++ packages/keyman/tests/main.test.ts | 11 +++++++ packages/keyman/tests/utils.test.ts | 41 ++++++++++++++++++++++++++- 10 files changed, 183 insertions(+), 22 deletions(-) diff --git a/packages/keyman/src/keyman.decrypt.ts b/packages/keyman/src/keyman.decrypt.ts index e278910..f499983 100644 --- a/packages/keyman/src/keyman.decrypt.ts +++ b/packages/keyman/src/keyman.decrypt.ts @@ -2,14 +2,15 @@ import fs from 'node:fs'; import path from 'node:path'; import { execa } from 'execa'; import inquirer from 'inquirer'; +import { runTool } from './keyman.utils.js'; export async function decryptKeys(sshDir: string, vaultDir: string, ageKey: string) { const keyDir = path.join(vaultDir, 'keys'); - const vaultKeys = fs.readdirSync(keyDir).filter((key) => { - const keyfile = path.join(keyDir, key, `id_${key}.age`); - console.log(keyfile); - return fs.existsSync(keyfile); - }); + // Guarded: nothing creates the keys directory until the first encrypt, so on a + // fresh vault this readdir threw instead of reporting an empty vault. + const vaultKeys = fs.existsSync(keyDir) + ? fs.readdirSync(keyDir).filter((key) => fs.existsSync(path.join(keyDir, key, `id_${key}.age`))) + : []; if (vaultKeys.length === 0) { console.log('⚠️ No encrypted keys found.'); @@ -44,7 +45,7 @@ export async function decryptKeys(sshDir: string, vaultDir: string, ageKey: stri : path.join(sshDir, `id_${key}.pub`); // Decrypt key - await execa('age', ['-d', '-i', ageKey, '-o', privateKeyOut, encryptedKey]); + await runTool('age', ['-d', '-i', ageKey, '-o', privateKeyOut, encryptedKey]); await execa('cp', [publicKey, publicKeyOut]); await execa('chmod', ['600', privateKeyOut]); diff --git a/packages/keyman/src/keyman.encrypt.ts b/packages/keyman/src/keyman.encrypt.ts index 62aebd8..7fb1c4a 100644 --- a/packages/keyman/src/keyman.encrypt.ts +++ b/packages/keyman/src/keyman.encrypt.ts @@ -1,7 +1,20 @@ import fs from 'node:fs'; import path from 'node:path'; -import { execa } from 'execa'; import inquirer from 'inquirer'; +import { runTool } from './keyman.utils.js'; + +/** + * Private keys in a directory that may not exist. + * + * A first run has neither `~/.ssh` nor the tmp directory, and an unguarded + * readdir there threw before the "nothing to encrypt" message could be reached. + */ +function privateKeysIn(dir: string): string[] { + if (!fs.existsSync(dir)) { + return []; + } + return fs.readdirSync(dir).filter((key) => key.startsWith('id_') && !key.endsWith('.pub')); +} export async function encryptKeys( sshDir: string, @@ -9,14 +22,8 @@ export async function encryptKeys( tmpDir: string, pubkey: string ) { - const sshKeys = fs - .readdirSync(sshDir) - .filter((key) => key.startsWith('id_') && !key.endsWith('.pub')); - const tmpKeys = fs - .readdirSync(tmpDir) - .filter((key) => key.startsWith('id_') && !key.endsWith('.pub')); - console.log(tmpKeys); - console.log(sshKeys); + const sshKeys = privateKeysIn(sshDir); + const tmpKeys = privateKeysIn(tmpDir); const keys = [...new Set([...sshKeys, ...tmpKeys])]; if (keys.length === 0) { @@ -36,10 +43,10 @@ export async function encryptKeys( for (const key of selectedKeys) { const keyPath = path.join(tmpKeys.includes(key) ? tmpDir : sshDir, key); const vaultPath = path.join(vaultDir, 'keys', key.replace('id_', '')); - fs.mkdirSync(vaultPath, { recursive: true }); + fs.mkdirSync(vaultPath, { recursive: true, mode: 0o700 }); // Encrypt key using `age` - await execa('age', ['-r', pubkey, '-o', path.join(vaultPath, `${key}.age`), keyPath]); + await runTool('age', ['-r', pubkey, '-o', path.join(vaultPath, `${key}.age`), keyPath]); // Copy public key and create README fs.copyFileSync(`${keyPath}.pub`, path.join(vaultPath, `${key}.pub`)); diff --git a/packages/keyman/src/keyman.list.ts b/packages/keyman/src/keyman.list.ts index c430c3c..fcb9eb1 100644 --- a/packages/keyman/src/keyman.list.ts +++ b/packages/keyman/src/keyman.list.ts @@ -76,9 +76,12 @@ export async function listKeys(sshDir: string, vaultDir: string, tmpDir: string) // Scan vault directory if (fs.existsSync(vaultDir)) { + // throwIfNoEntry keeps a dangling symlink from aborting the whole listing; + // the stat still follows a symlink to a real directory, which withFileTypes + // would have reported as a link and skipped. const vaultDirs = fs.readdirSync(vaultDir).filter((dir) => { - const stat = fs.statSync(path.join(vaultDir, dir)); - return stat.isDirectory(); + const stat = fs.statSync(path.join(vaultDir, dir), { throwIfNoEntry: false }); + return stat?.isDirectory() ?? false; }); for (const dir of vaultDirs) { diff --git a/packages/keyman/src/keyman.main.ts b/packages/keyman/src/keyman.main.ts index 6bf7af8..0b4d38a 100644 --- a/packages/keyman/src/keyman.main.ts +++ b/packages/keyman/src/keyman.main.ts @@ -37,8 +37,12 @@ export async function keyman() { } const sshDir = path.join(homeDir, '.ssh'); - fs.mkdirSync(paths.vaultRoot, { recursive: true }); - fs.mkdirSync(paths.tmpDir, { recursive: true }); + // 0700 because the vault holds the age identity and, in tmp, plaintext private + // keys. keysDir is created here too: decrypt used to read it before anything + // created it. + for (const dir of [paths.vaultRoot, paths.keysDir, paths.tmpDir]) { + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + } // Main loop - keep showing menu until user quits let running = true; diff --git a/packages/keyman/src/keyman.utils.ts b/packages/keyman/src/keyman.utils.ts index 880ded8..0b14c80 100644 --- a/packages/keyman/src/keyman.utils.ts +++ b/packages/keyman/src/keyman.utils.ts @@ -1,4 +1,37 @@ import fs from 'node:fs'; +import { execa, type Options } from 'execa'; + +/** + * Runs one of the external binaries keyman depends on. + * + * Two failures are worth telling apart, and an execa error tells a reader + * neither: the binary not being installed (`ENOENT`, whose message is + * `spawn ENOENT`) and the binary refusing (whose reason is on stderr and + * nowhere in the thrown message). `age` is a hard requirement, so its absence + * has to read as an instruction. + * + * Returns only `stdout` — annotated rather than inferred because execa's result + * type cannot be named from here (TS2883), and it is all any caller wants. Empty + * when the output went somewhere else, as with `stdio: 'inherit'`. + */ +export async function runTool( + binary: string, + args: string[], + options?: Options +): Promise<{ stdout: string }> { + try { + // Called without the third argument when there are no options, so a test + // asserting on the spawn sees the call it wrote. + const result = options ? await execa(binary, args, options) : await execa(binary, args); + return { stdout: typeof result.stdout === 'string' ? result.stdout : '' }; + } catch (error) { + const failure = error as { code?: string; stderr?: string; shortMessage?: string }; + if (failure.code === 'ENOENT') { + throw new Error(`\`${binary}\` was not found on PATH. Install it and try again.`); + } + throw new Error(`\`${binary}\` failed: ${failure.stderr?.trim() || failure.shortMessage}`); + } +} /** * Extracts the public key from an age key file. diff --git a/packages/keyman/tests/decrypt.test.ts b/packages/keyman/tests/decrypt.test.ts index 11b65a2..9f1f194 100644 --- a/packages/keyman/tests/decrypt.test.ts +++ b/packages/keyman/tests/decrypt.test.ts @@ -69,6 +69,23 @@ describe('decryptKeys', () => { expect(prompt).not.toHaveBeenCalled(); }); + it('warns instead of throwing when the vault has no keys directory', async () => { + fs.rmSync(keyDir, { recursive: true }); + + await expect(decryptKeys(sshDir, vaultDir, AGE_KEY)).resolves.toBeUndefined(); + expect(messages(logSpy)).toContain('No encrypted keys found.'); + }); + + it('reports a missing age binary rather than an ENOENT', async () => { + vaultKey('prod'); + prompt.mockResolvedValue({ selectedKeys: ['prod'], decryptMode: LOCAL }); + execa.mockRejectedValue(Object.assign(new Error('spawn age ENOENT'), { code: 'ENOENT' })); + + await expect(decryptKeys(sshDir, vaultDir, AGE_KEY)).rejects.toThrow( + '`age` was not found on PATH' + ); + }); + it('offers only directories that actually contain an encrypted key', async () => { vaultKey('prod'); fs.mkdirSync(path.join(keyDir, 'empty'), { recursive: true }); diff --git a/packages/keyman/tests/encrypt.test.ts b/packages/keyman/tests/encrypt.test.ts index 0b3e74f..e19f114 100644 --- a/packages/keyman/tests/encrypt.test.ts +++ b/packages/keyman/tests/encrypt.test.ts @@ -66,6 +66,33 @@ describe('encryptKeys', () => { expect(prompt).not.toHaveBeenCalled(); }); + it('warns instead of throwing when the .ssh directory does not exist', async () => { + fs.rmSync(sshDir, { recursive: true }); + + await expect(encryptKeys(sshDir, vaultDir, tmpDir, PUBKEY)).resolves.toBeUndefined(); + expect(messages(logSpy)).toContain('No private SSH keys found to encrypt.'); + }); + + it('still offers the .ssh keys when the tmp directory does not exist', async () => { + fs.rmSync(tmpDir, { recursive: true }); + key(sshDir, 'id_prod', 'ssh'); + prompt.mockResolvedValue({ selectedKeys: [] }); + + await encryptKeys(sshDir, vaultDir, tmpDir, PUBKEY); + + expect(choices()).toEqual(['id_prod']); + }); + + it('reports a missing age binary rather than an ENOENT', async () => { + key(sshDir, 'id_prod', 'ssh'); + prompt.mockResolvedValue({ selectedKeys: ['id_prod'] }); + execa.mockRejectedValue(Object.assign(new Error('spawn age ENOENT'), { code: 'ENOENT' })); + + await expect(encryptKeys(sshDir, vaultDir, tmpDir, PUBKEY)).rejects.toThrow( + '`age` was not found on PATH' + ); + }); + it('ignores public keys and unrelated files when building the list', async () => { fs.writeFileSync(path.join(sshDir, 'known_hosts'), ''); fs.writeFileSync(path.join(sshDir, 'id_orphan.pub'), 'PUBLIC'); diff --git a/packages/keyman/tests/list.test.ts b/packages/keyman/tests/list.test.ts index 013bffe..960eab1 100644 --- a/packages/keyman/tests/list.test.ts +++ b/packages/keyman/tests/list.test.ts @@ -182,6 +182,25 @@ describe('listKeys', () => { expect(row('id_real')).toBeDefined(); }); + it('keeps listing when the vault holds a dangling symlink', async () => { + vaultKey('real'); + fs.symlinkSync(path.join(root, 'gone'), path.join(vaultDir, 'broken')); + + await expect(listKeys(sshDir, vaultDir, tmpDir)).resolves.toBeUndefined(); + expect(row('id_real')).toBeDefined(); + }); + + it('follows a symlink pointing at a real vault directory', async () => { + const elsewhere = path.join(root, 'elsewhere', 'prod'); + touch(elsewhere, 'id_prod.age'); + fs.mkdirSync(vaultDir, { recursive: true }); + fs.symlinkSync(elsewhere, path.join(vaultDir, 'prod')); + + await listKeys(sshDir, vaultDir, tmpDir); + + expect(row('id_prod')).toBeDefined(); + }); + it('ignores loose files sitting next to the vault directories', async () => { vaultKey('real'); fs.writeFileSync(path.join(vaultDir, 'README.md'), ''); diff --git a/packages/keyman/tests/main.test.ts b/packages/keyman/tests/main.test.ts index aca6621..2519bad 100644 --- a/packages/keyman/tests/main.test.ts +++ b/packages/keyman/tests/main.test.ts @@ -106,6 +106,17 @@ describe('keyman', () => { expect(output()).toContain(paths.keyPath); expect(fs.existsSync(paths.vaultRoot)).toBe(true); expect(fs.existsSync(paths.tmpDir)).toBe(true); + // keysDir too: decrypt reads it, and nothing created it before the first + // encrypt, so a fresh vault could not be decrypted from. + expect(fs.existsSync(paths.keysDir)).toBe(true); + }); + + it('creates the vault directories private to the owner', async () => { + await keyman(); + + for (const dir of [paths.vaultRoot, paths.keysDir, paths.tmpDir]) { + expect(fs.statSync(dir).mode & 0o777, dir).toBe(0o700); + } }); it('quits without running any operation', async () => { diff --git a/packages/keyman/tests/utils.test.ts b/packages/keyman/tests/utils.test.ts index 15c4770..14dff26 100644 --- a/packages/keyman/tests/utils.test.ts +++ b/packages/keyman/tests/utils.test.ts @@ -9,7 +9,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { extractAgePublicKey } from '../src/keyman.utils.js'; +import { extractAgePublicKey, runTool } from '../src/keyman.utils.js'; describe('extractAgePublicKey', () => { let tmpDir: string; @@ -77,3 +77,42 @@ describe('extractAgePublicKey', () => { expect(errorSpy.mock.calls[0][0]).toContain('Failed to read key file'); }); }); + +/** + * These spawn real processes rather than mocking execa. The whole point of + * runTool is the shape of an execa failure, and a mock would only assert what + * this test already assumes. + */ +describe('runTool', () => { + it('returns the result on success', async () => { + const result = await runTool('node', ['-e', 'process.stdout.write("hi")']); + + expect(result.stdout).toBe('hi'); + }); + + it('passes options through', async () => { + const result = await runTool('node', ['-e', 'process.stdout.write(process.env.PROBE ?? "")'], { + env: { PROBE: 'from-options' }, + }); + + expect(result.stdout).toBe('from-options'); + }); + + it('reports a missing binary as an instruction rather than an ENOENT', async () => { + await expect(runTool('keyman-no-such-binary', [])).rejects.toThrow( + '`keyman-no-such-binary` was not found on PATH. Install it and try again.' + ); + }); + + it('surfaces what the binary wrote to stderr', async () => { + await expect( + runTool('node', ['-e', 'process.stderr.write("no recipient\\n"); process.exit(1)']) + ).rejects.toThrow('`node` failed: no recipient'); + }); + + it('falls back to the command summary when stderr is empty', async () => { + await expect(runTool('node', ['-e', 'process.exit(3)'])).rejects.toThrow( + /`node` failed: .*exit code 3/ + ); + }); +});