diff --git a/packages/keyman/src/keyman.encrypt.ts b/packages/keyman/src/keyman.encrypt.ts index fdf5a67..a26097e 100644 --- a/packages/keyman/src/keyman.encrypt.ts +++ b/packages/keyman/src/keyman.encrypt.ts @@ -1,7 +1,8 @@ import fs from 'node:fs'; import path from 'node:path'; import inquirer from 'inquirer'; -import { runTool } from './keyman.utils.js'; +import { ToolNotFoundError } from './keyman.utils.js'; +import { storeInVault } from './keyman.vault.js'; /** * Private keys in a directory that may not exist. @@ -35,17 +36,30 @@ export async function encryptKeys(sshDir: string, keysDir: string, tmpDir: strin }, ]); + const failed: string[] = []; + for (const key of selectedKeys) { const keyPath = path.join(tmpKeys.includes(key) ? tmpDir : sshDir, key); - const vaultPath = path.join(keysDir, key.replace('id_', '')); - fs.mkdirSync(vaultPath, { recursive: true, mode: 0o700 }); - // Encrypt key using `age` - await runTool('age', ['-r', pubkey, '-o', path.join(vaultPath, `${key}.age`), keyPath]); + try { + await storeInVault(keyPath, keysDir, pubkey); + } catch (error) { + // One bad key costs one key. Selecting ten and losing the last nine to an + // unreadable first one was the old behaviour, and nothing afterwards said + // which of the ten had made it into the vault. + if (error instanceof ToolNotFoundError) { + // Not a per-key problem: age is missing for all of them, so nine more + // identical failures would tell the user nothing new. + throw error; + } + failed.push(key); + console.error(`āŒ ${key}: ${error instanceof Error ? error.message : error}`); + } + } - // Copy public key and create README - fs.copyFileSync(`${keyPath}.pub`, path.join(vaultPath, `${key}.pub`)); - - console.log(`šŸ”’ Encrypted and stored: ${vaultPath}/${key}`); + if (failed.length > 0) { + console.log( + `\nāš ļø ${failed.length} of ${selectedKeys.length} selected keys were not stored: ${failed.join(', ')}` + ); } } diff --git a/packages/keyman/src/keyman.generate.ts b/packages/keyman/src/keyman.generate.ts index d938f2c..90fcdca 100644 --- a/packages/keyman/src/keyman.generate.ts +++ b/packages/keyman/src/keyman.generate.ts @@ -1,7 +1,8 @@ import fs from 'node:fs'; import path from 'node:path'; -import { execa } from 'execa'; import inquirer from 'inquirer'; +import { runTool } from './keyman.utils.js'; +import { storeInVault } from './keyman.vault.js'; export async function generateKey(tmpDir: string, keysDir: string, pubkey: string) { const { algorithm } = await inquirer.prompt<{ algorithm: string }>([ @@ -23,15 +24,6 @@ export async function generateKey(tmpDir: string, keysDir: string, pubkey: strin }, ]); - const { password } = await inquirer.prompt<{ password: string }>([ - { - type: 'password', - name: 'password', - message: 'Enter passphrase (leave empty for no passphrase):', - mask: '*', - }, - ]); - const { identity } = await inquirer.prompt<{ identity: string }>([ { type: 'input', @@ -48,30 +40,31 @@ export async function generateKey(tmpDir: string, keysDir: string, pubkey: strin return; } + const args = ['-t', algorithm, '-f', keyPath, '-C', identity]; + if (algorithm === 'rsa') { + args.push('-b', '4096'); + } + try { console.log(`Generating ${algorithm} key pair...`); - const args = ['-t', algorithm, '-f', keyPath, '-N', password, '-C', identity]; - - if (algorithm === 'rsa') { - args.push('-b', '4096'); - } - - await execa('ssh-keygen', args); + // No `-N`, and stdio inherited: ssh-keygen asks for the passphrase itself and + // confirms it. keyman used to prompt for it and pass it as `-N `, + // which put the passphrase in this process's argv — readable by any user on + // the box via `ps` for as long as the spawn lived, and in keyman's memory + // before that. A passphrase keyman never learns cannot be leaked by keyman. + await runTool('ssh-keygen', args, { stdio: 'inherit' }); console.log(`āœ… Key generated: ${keyPath}`); - - // Encrypt the key - const folderName = fileName.replace('id_', ''); - const vaultPath = path.join(keysDir, folderName); - fs.mkdirSync(vaultPath, { recursive: true }); - - // Encrypt key using `age` - await execa('age', ['-r', pubkey, '-o', path.join(vaultPath, `${fileName}.age`), keyPath]); - - // Copy public key - fs.copyFileSync(`${keyPath}.pub`, path.join(vaultPath, `${fileName}.pub`)); - - console.log(`šŸ”’ Encrypted and stored: ${vaultPath}`); } catch (error) { - console.error(`āŒ Error generating/encrypting key: ${error}`); + console.error(`āŒ Error generating key: ${error instanceof Error ? error.message : error}`); + return; + } + + try { + await storeInVault(keyPath, keysDir, pubkey); + } catch (error) { + // The private key is still in tmpDir, so this is recoverable by encrypting it + // — which is why it does not read as having lost the key. + console.error(`āŒ Error encrypting key: ${error instanceof Error ? error.message : error}`); + console.error(` ${keyPath} was generated; encrypt it once the problem is fixed.`); } } diff --git a/packages/keyman/src/keyman.vault.ts b/packages/keyman/src/keyman.vault.ts new file mode 100644 index 0000000..a0f8da1 --- /dev/null +++ b/packages/keyman/src/keyman.vault.ts @@ -0,0 +1,88 @@ +import fs from 'node:fs'; +import path from 'node:path'; +import { runTool } from './keyman.utils.js'; + +/** + * The public half of a private key, derived if the sibling file is missing. + * + * `encrypt` builds its selection list from private keys only, so a key whose + * `.pub` was deleted is offered like any other. Reading the sibling blindly meant + * finding out it was absent *after* `age` had written the encrypted key — a vault + * entry with no public key, and an exception that killed the rest of the batch. + * + * @returns the public key text, or null with the reason already reported + */ +async function publicKeyFor(keyPath: string): Promise { + const sibling = `${keyPath}.pub`; + if (fs.existsSync(sibling)) { + return fs.readFileSync(sibling, 'utf-8'); + } + + const fileName = path.basename(keyPath); + console.log(`ā„¹ļø ${fileName} has no .pub file — deriving it with ssh-keygen.`); + + try { + // stdin and stderr inherited, stdout piped: verified that `ssh-keygen -y` + // prompts for the passphrase of an encrypted key, and that it prompts on + // *stderr*. Capturing everything would hide the prompt and then fail on the + // passphrase nobody was asked for; inheriting everything would lose the key. + const { stdout } = await runTool('ssh-keygen', ['-y', '-f', keyPath], { + stdio: ['inherit', 'pipe', 'inherit'], + }); + const derived = stdout.trim(); + if (derived) { + return `${derived}\n`; + } + } catch (error) { + console.warn(`āš ļø ${fileName}: ${error instanceof Error ? error.message : error}`); + } + + console.warn(`āš ļø ${fileName}: no public key could be derived; storing the private key alone.`); + return null; +} + +/** + * Encrypts one private key into `//`, alongside its public half. + * + * Shared by `encrypt` and `generate`, which were two copies of it. + * + * @returns the vault directory the key was stored in + */ +export async function storeInVault( + keyPath: string, + keysDir: string, + pubkey: string +): Promise { + const fileName = path.basename(keyPath); + const vaultPath = path.join(keysDir, fileName.replace(/^id_/, '')); + + // Before the directory exists, so a key that cannot be read does not leave one. + const publicKey = await publicKeyFor(keyPath); + + fs.mkdirSync(vaultPath, { recursive: true, mode: 0o700 }); + const encryptedKey = path.join(vaultPath, `${fileName}.age`); + + try { + await runTool('age', ['-r', pubkey, '-o', encryptedKey, keyPath]); + } catch (error) { + // age writes into a directory that has to exist already, so a failure here + // leaves one behind — and possibly a truncated .age file, which `list` would + // count as a vault entry and `decrypt` would offer. Both are ours: the file + // because we named it, the directory only while it is empty, since one + // holding an earlier key is not. + fs.rmSync(encryptedKey, { force: true }); + try { + fs.rmdirSync(vaultPath); + } catch { + // ENOTEMPTY — something else was already stored here. + } + throw error; + } + + if (publicKey !== null) { + fs.writeFileSync(path.join(vaultPath, `${fileName}.pub`), publicKey); + } + + console.log(`šŸ”’ Encrypted and stored: ${path.join(vaultPath, `${fileName}.age`)}`); + return vaultPath; +} diff --git a/packages/keyman/tests/encrypt.test.ts b/packages/keyman/tests/encrypt.test.ts index bb6f006..6c89747 100644 --- a/packages/keyman/tests/encrypt.test.ts +++ b/packages/keyman/tests/encrypt.test.ts @@ -163,4 +163,109 @@ describe('encryptKeys', () => { expect(execa).not.toHaveBeenCalled(); expect(fs.existsSync(keysDir)).toBe(false); }); + + describe('a key with no .pub file', () => { + /** A private key without its sibling — what the selection list offers anyway. */ + const orphan = (dir: string, name: string) => { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, name), `PRIVATE ${name}`); + }; + + it('derives the public key with ssh-keygen', async () => { + orphan(sshDir, 'id_prod'); + prompt.mockResolvedValue({ selectedKeys: ['id_prod'] }); + execa.mockImplementation(async (binary: string, args: string[]) => { + if (binary === 'ssh-keygen') return { stdout: 'ssh-ed25519 AAAA derived' }; + fs.writeFileSync(args[args.indexOf('-o') + 1], 'ENCRYPTED'); + return { stdout: '' }; + }); + + await encryptKeys(sshDir, keysDir, tmpDir, PUBKEY); + + expect(execa).toHaveBeenCalledWith( + 'ssh-keygen', + ['-y', '-f', path.join(sshDir, 'id_prod')], + // stderr inherited so the passphrase prompt is visible, stdout piped so + // the derived key can be captured. + { stdio: ['inherit', 'pipe', 'inherit'] } + ); + expect(fs.readFileSync(path.join(keysDir, 'prod', 'id_prod.pub'), 'utf-8')).toBe( + 'ssh-ed25519 AAAA derived\n' + ); + }); + + it('stores the private key alone when the derivation fails', async () => { + orphan(sshDir, 'id_prod'); + prompt.mockResolvedValue({ selectedKeys: ['id_prod'] }); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + execa.mockImplementation(async (binary: string, args: string[]) => { + if (binary === 'ssh-keygen') { + throw Object.assign(new Error('bad passphrase'), { stderr: 'incorrect passphrase' }); + } + fs.writeFileSync(args[args.indexOf('-o') + 1], 'ENCRYPTED'); + return { stdout: '' }; + }); + + await encryptKeys(sshDir, keysDir, tmpDir, PUBKEY); + + // The encrypted key is what matters; the .pub is recoverable from it later. + expect(fs.existsSync(path.join(keysDir, 'prod', 'id_prod.age'))).toBe(true); + expect(fs.existsSync(path.join(keysDir, 'prod', 'id_prod.pub'))).toBe(false); + expect(messages(warnSpy)).toContain('no public key could be derived'); + }); + }); + + describe('when one key of several fails', () => { + beforeEach(() => { + key(sshDir, 'id_prod', 'ssh'); + key(sshDir, 'id_stage', 'ssh'); + prompt.mockResolvedValue({ selectedKeys: ['id_prod', 'id_stage'] }); + // age refuses the first key only. + execa.mockImplementation(async (_binary: string, args: string[]) => { + if (args.some((arg) => arg.endsWith('id_prod'))) { + throw Object.assign(new Error('age refused'), { stderr: 'no identity' }); + } + fs.writeFileSync(args[args.indexOf('-o') + 1], 'ENCRYPTED'); + return { stdout: '' }; + }); + }); + + it('still encrypts the rest', async () => { + await encryptKeys(sshDir, keysDir, tmpDir, PUBKEY); + + expect(fs.existsSync(path.join(keysDir, 'stage', 'id_stage.age'))).toBe(true); + }); + + it('reports which keys were not stored', async () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + await encryptKeys(sshDir, keysDir, tmpDir, PUBKEY); + + expect(messages(errorSpy)).toContain('id_prod'); + expect(messages(logSpy)).toContain('1 of 2 selected keys were not stored: id_prod'); + }); + + it('leaves no vault entry for the key that failed', async () => { + await encryptKeys(sshDir, keysDir, tmpDir, PUBKEY); + + // Not even an empty directory: list counts a directory with an .age in it, + // and a truncated .age would be offered for decryption. + expect(fs.existsSync(path.join(keysDir, 'prod'))).toBe(false); + }); + }); + + it('gives up immediately when age is not installed', async () => { + key(sshDir, 'id_prod', 'ssh'); + key(sshDir, 'id_stage', 'ssh'); + prompt.mockResolvedValue({ selectedKeys: ['id_prod', 'id_stage'] }); + execa.mockImplementation(async () => { + throw Object.assign(new Error('spawn age ENOENT'), { code: 'ENOENT' }); + }); + + // Not a per-key failure: nine more identical errors help nobody. + await expect(encryptKeys(sshDir, keysDir, tmpDir, PUBKEY)).rejects.toThrow( + '`age` was not found on PATH' + ); + expect(execa).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/keyman/tests/generate.test.ts b/packages/keyman/tests/generate.test.ts index dce0ec3..606d6f8 100644 --- a/packages/keyman/tests/generate.test.ts +++ b/packages/keyman/tests/generate.test.ts @@ -42,6 +42,10 @@ describe('generateKey', () => { const argsOf = (binary: string) => execa.mock.calls.find((c) => c[0] === binary)?.[1] as string[] | undefined; + /** The options of the mocked call to `binary`. */ + const optionsOf = (binary: string) => + execa.mock.calls.find((c) => c[0] === binary)?.[2] as { stdio?: unknown } | undefined; + const messages = (spy: ReturnType) => spy.mock.calls.map((c) => c.join(' ')).join('\n'); @@ -64,7 +68,7 @@ describe('generateKey', () => { return { exitCode: 0 }; }); - answer({ algorithm: 'ed25519', keyName: 'prod', password: 'pw', identity: 'me@host' }); + answer({ algorithm: 'ed25519', keyName: 'prod', identity: 'me@host' }); }); afterEach(() => { @@ -80,16 +84,24 @@ describe('generateKey', () => { 'ed25519', '-f', path.join(tmpDir, 'id_prod'), - '-N', - 'pw', '-C', 'me@host', ]); expect(messages(logSpy)).toContain('Key generated'); }); + it('never handles the passphrase itself', async () => { + await generateKey(tmpDir, keysDir, PUBKEY); + + // No -N, so ssh-keygen prompts and confirms; inherited stdio is what makes + // that prompt reach the terminal. The passphrase never touches argv. + expect(argsOf('ssh-keygen')).not.toContain('-N'); + expect(optionsOf('ssh-keygen')).toEqual({ stdio: 'inherit' }); + expect(prompt.mock.calls.map((c) => c[0][0].name)).not.toContain('password'); + }); + it('does not prefix a key name that already starts with id_', async () => { - answer({ algorithm: 'ed25519', keyName: 'id_prod', password: '', identity: '' }); + answer({ algorithm: 'ed25519', keyName: 'id_prod', identity: '' }); await generateKey(tmpDir, keysDir, PUBKEY); @@ -97,7 +109,7 @@ describe('generateKey', () => { }); it('requests a 4096 bit key for rsa', async () => { - answer({ algorithm: 'rsa', keyName: 'prod', password: '', identity: '' }); + answer({ algorithm: 'rsa', keyName: 'prod', identity: '' }); await generateKey(tmpDir, keysDir, PUBKEY); @@ -139,17 +151,22 @@ describe('generateKey', () => { expect(fs.readFileSync(path.join(tmpDir, 'id_prod'), 'utf-8')).toBe('EXISTING'); }); - it('reports a failure from ssh-keygen without leaving a vault entry', async () => { - execa.mockRejectedValue(new Error('ssh-keygen exploded')); + it('reports a failure from ssh-keygen without reaching age', async () => { + execa.mockImplementation(async () => { + throw Object.assign(new Error('ssh-keygen exploded'), { stderr: 'ssh-keygen exploded' }); + }); await expect(generateKey(tmpDir, keysDir, PUBKEY)).resolves.toBeUndefined(); - expect(messages(errorSpy)).toContain('Error generating/encrypting key'); + expect(messages(errorSpy)).toContain('Error generating key'); + expect(argsOf('age')).toBeUndefined(); expect(fs.existsSync(path.join(keysDir, 'prod'))).toBe(false); }); - it('reports a failure from age', async () => { + it('reports a failure from age and says the key is still there to encrypt', async () => { execa.mockImplementation(async (binary: string, args: string[]) => { - if (binary === 'age') throw new Error('age exploded'); + if (binary === 'age') { + throw Object.assign(new Error('age exploded'), { stderr: 'age exploded' }); + } const keyPath = args[args.indexOf('-f') + 1]; fs.writeFileSync(keyPath, 'PRIVATE'); fs.writeFileSync(`${keyPath}.pub`, 'ssh-ed25519 AAAA generated'); @@ -158,7 +175,11 @@ describe('generateKey', () => { await generateKey(tmpDir, keysDir, PUBKEY); - expect(messages(errorSpy)).toContain('Error generating/encrypting key'); - expect(fs.existsSync(path.join(keysDir, 'prod', 'id_prod.pub'))).toBe(false); + expect(messages(errorSpy)).toContain('Error encrypting key'); + // The generated key is the thing of value, and it survived. + expect(fs.existsSync(path.join(tmpDir, 'id_prod'))).toBe(true); + expect(messages(errorSpy)).toContain(path.join(tmpDir, 'id_prod')); + // And no half-made vault entry was left claiming to hold it. + expect(fs.existsSync(path.join(keysDir, 'prod'))).toBe(false); }); }); diff --git a/packages/keyman/tests/vault.test.ts b/packages/keyman/tests/vault.test.ts new file mode 100644 index 0000000..1b2c756 --- /dev/null +++ b/packages/keyman/tests/vault.test.ts @@ -0,0 +1,122 @@ +/** + * Tests for storeInVault, the write path encrypt and generate share. + * + * Its ordinary use is covered through those two callers; what is here is the + * behaviour that is awkward to reach from either — an ssh-keygen that succeeds + * without printing anything, and a failure over an entry that already exists. + */ + +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { execa } = vi.hoisted(() => ({ execa: vi.fn() })); + +vi.mock('execa', () => ({ execa })); + +import { storeInVault } from '../src/keyman.vault.js'; + +describe('storeInVault', () => { + let root: string; + let keysDir: string; + let keyPath: string; + let logSpy: ReturnType; + let warnSpy: ReturnType; + + const PUBKEY = 'age1recipient'; + + const messages = (spy: ReturnType) => + spy.mock.calls.map((c) => c.join(' ')).join('\n'); + + beforeEach(() => { + vi.clearAllMocks(); + root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'keyman-vault-'))); + keysDir = path.join(root, 'keys'); + keyPath = path.join(root, 'id_prod'); + fs.writeFileSync(keyPath, 'PRIVATE'); + logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + fs.rmSync(root, { recursive: true, force: true }); + }); + + it('warns when ssh-keygen succeeds but prints no key', async () => { + execa.mockImplementation(async (binary: string, args: string[]) => { + if (binary === 'ssh-keygen') return { stdout: ' \n' }; + fs.writeFileSync(args[args.indexOf('-o') + 1], 'ENCRYPTED'); + return { stdout: '' }; + }); + + await storeInVault(keyPath, keysDir, PUBKEY); + + // An exit code of 0 is not a public key: writing a .pub holding whitespace + // would put a file in the vault that no host would ever accept. + expect(fs.existsSync(path.join(keysDir, 'prod', 'id_prod.pub'))).toBe(false); + expect(messages(warnSpy)).toContain('no public key could be derived'); + expect(fs.existsSync(path.join(keysDir, 'prod', 'id_prod.age'))).toBe(true); + }); + + it('writes the public half at the same time as the encrypted key', async () => { + fs.writeFileSync(`${keyPath}.pub`, 'ssh-ed25519 AAAA sibling'); + execa.mockImplementation(async (_binary: string, args: string[]) => { + fs.writeFileSync(args[args.indexOf('-o') + 1], 'ENCRYPTED'); + return { stdout: '' }; + }); + + const vaultPath = await storeInVault(keyPath, keysDir, PUBKEY); + + expect(vaultPath).toBe(path.join(keysDir, 'prod')); + expect(fs.readFileSync(path.join(vaultPath, 'id_prod.pub'), 'utf-8')).toBe( + 'ssh-ed25519 AAAA sibling' + ); + // No ssh-keygen: the sibling was there, so nothing needed deriving. + expect(execa.mock.calls.every((c) => c[0] === 'age')).toBe(true); + }); + + it('creates the vault entry private to the owner', async () => { + fs.writeFileSync(`${keyPath}.pub`, 'PUBLIC'); + execa.mockImplementation(async (_binary: string, args: string[]) => { + fs.writeFileSync(args[args.indexOf('-o') + 1], 'ENCRYPTED'); + return { stdout: '' }; + }); + + await storeInVault(keyPath, keysDir, PUBKEY); + + expect(fs.statSync(path.join(keysDir, 'prod')).mode & 0o777).toBe(0o700); + }); + + describe('when age fails', () => { + beforeEach(() => { + fs.writeFileSync(`${keyPath}.pub`, 'PUBLIC'); + execa.mockImplementation(async (_binary: string, args: string[]) => { + // Half-written output, the way a failing age can leave it. + fs.writeFileSync(args[args.indexOf('-o') + 1], 'TRUNC'); + throw Object.assign(new Error('age refused'), { stderr: 'no recipient' }); + }); + }); + + it('leaves no truncated key behind for decrypt to offer', async () => { + await expect(storeInVault(keyPath, keysDir, PUBKEY)).rejects.toThrow('`age` failed'); + + expect(fs.existsSync(path.join(keysDir, 'prod'))).toBe(false); + }); + + it('keeps an entry that was already there', async () => { + const vaultPath = path.join(keysDir, 'prod'); + fs.mkdirSync(vaultPath, { recursive: true }); + fs.writeFileSync(path.join(vaultPath, 'id_prod.pub'), 'THE OLD PUBLIC KEY'); + + await expect(storeInVault(keyPath, keysDir, PUBKEY)).rejects.toThrow('`age` failed'); + + // Cleaning up after a failure must not take the previous key with it. + expect(fs.readFileSync(path.join(vaultPath, 'id_prod.pub'), 'utf-8')).toBe( + 'THE OLD PUBLIC KEY' + ); + expect(messages(logSpy)).not.toContain('Encrypted and stored'); + }); + }); +});