diff --git a/packages/keyman/docs/AUDIT.md b/packages/keyman/docs/AUDIT.md new file mode 100644 index 0000000..e284600 --- /dev/null +++ b/packages/keyman/docs/AUDIT.md @@ -0,0 +1,656 @@ +# keyman audit + +A review of `packages/keyman` for defects, unimplemented features, and drift +between the code and the documents that describe it. + +Severity is about what it costs a user: + +- **šŸ”“ broken** ā€” normal use produces a crash, data loss, or a silently wrong result. +- **šŸŸ  misleading** ā€” the code or a document states something that is not true. +- **šŸŸ” gap** ā€” something real that nothing mentions, or dead weight nobody uses. + +Verified against `75983ab` with no uncommitted changes in the package. Line +numbers are from that state. Findings marked **verified** were reproduced by +running the code, not inferred from reading it; the reproduction is quoted. + +Baseline: 162 tests pass, 98.9 % lines / 96.2 % branches. High coverage is +context for Ā§1.2, not a defence of it. + +--- + +## Contents + +- [1. Defects](#1-defects) +- [2. Security](#2-security) +- [3. Unimplemented and dead](#3-unimplemented-and-dead) +- [4. Public API and packaging](#4-public-api-and-packaging) +- [5. Documentation drift](#5-documentation-drift) +- [6. Checked and accurate](#6-checked-and-accurate) +- [Suggested order of attack](#suggested-order-of-attack) + +--- + +## 1. Defects + +### 1.1 šŸ”“ `keysDir` and `tmpDir` are honoured by half the tool + +`resolveConfigPaths()` (`keyman.config.ts:249-259`) resolves all four paths from +the config, and `keyman.main.ts:18-21` prints them. But dispatch is inconsistent +about what it hands each operation: + +| Operation | receives | uses | +| --- | --- | --- | +| `listKeys` (`main.ts:67`) | `paths.keysDir` | the configured directory | +| `generateKey` (`main.ts:73`) | `paths.keysDir`, `paths.tmpDir` | the configured directories | +| `copyKey` (`main.ts:70`) | `paths.tmpDir` | the configured directory | +| `encryptKeys` (`main.ts:76`) | `paths.vaultRoot` | **hardcoded** `/keys` (`encrypt.ts:38`) | +| `decryptKeys` (`main.ts:84`) | `paths.vaultRoot` | **hardcoded** `/keys` (`decrypt.ts:7`) and `/tmp` (`decrypt.ts:39,43`) | + +With the defaults the two halves agree, which is why this is invisible. Set +either sub-directory and the vault splits in two. + +**Verified.** With `{"vaultRoot":"./v","keysDir":"encrypted","tmpDir":"plain"}`, +`keyman --print-config` reports: + +```json +{"...":"...","keysDir":"ā€¦/v/encrypted","tmpDir":"ā€¦/v/plain","keyPath":"ā€¦/v/age.key"} +``` + +so `generate` writes to `v/encrypted//` and `list` scans `v/encrypted`, +while `encrypt` writes to `v/keys`, and `decrypt` reads `v/keys` and writes +`v/tmp` ā€” never touching either configured directory. Concretely: + +- encrypt a key, then list it ā†’ the list shows nothing in `[Vault]`. +- generate a key, then decrypt it ā†’ "āš ļø No encrypted keys found." +- decrypt to local, then encrypt ā†’ the key is not offered, because `encrypt` + reads the configured `tmpDir` while `decrypt` wrote to the hardcoded one. + +No error at any point. The user has two vaults and one of them is invisible to +whichever operation they try next. + +The current behaviour is locked in by tests: `main.test.ts:164-187` asserts +`vaultRoot` is what encrypt and decrypt receive ("encrypts keys into the vault +root"), and `encrypt.test.ts:95` / `decrypt.test.ts:53` assert the literal +`keys` segment. Fixing this means changing those assertions. + +**Fix.** Pass `paths.keysDir` and `paths.tmpDir` into `encryptKeys` and +`decryptKeys` and delete the three `path.join(vaultDir, 'keys' | 'tmp')` calls. +Neither function has a use for `vaultRoot` once that is done, so the parameter +goes away rather than becoming a second source of truth. + +See also Ā§5.1 ā€” `DOCS-AUDIT.md` currently lists this layout under *checked and +accurate*. + +### 1.2 šŸ”“ Encrypt and decrypt crash with a raw stack trace on a first run + +Three `readdirSync` calls have no `existsSync` guard: + +- `encrypt.ts:13` ā€” `~/.ssh`, which nothing creates. +- `encrypt.ts:16` ā€” the tmp directory. +- `decrypt.ts:8` ā€” `/keys`, which nothing creates either. + +`keyman.main.ts:40-41` creates `vaultRoot` and `tmpDir`. It does **not** create +`keysDir`, so `decrypt` on a fresh vault throws instead of printing its +"āš ļø No encrypted keys found." message ā€” the message is unreachable until the +directory exists for some other reason. + +**Verified**, calling both functions directly against a vault laid out the way +`main.ts` lays it out: + +``` +--- A: decryptKeys with no vault/keys directory --- +THREW: Error ENOENT ENOENT: no such file or directory, scandir 'ā€¦/vault/keys' +--- B: encryptKeys with no ~/.ssh directory --- +THREW: Error ENOENT ENOENT: no such file or directory, scandir 'ā€¦/home/.ssh' +``` + +What the user sees is worse than the exception, because of `keyman.cli.ts:82`: + +```ts +keyman(); +``` + +Not awaited, no `.catch`. Any rejection anywhere in the menu loop becomes an +unhandled rejection: Node prints the stack and exits non-zero, and the menu loop +ā€” whose whole point (`README.md:97`) is that you can run several operations in +one session ā€” is gone. + +`copyKey` guards (`copy.ts:8`) and `listKeys` guards all three of its +directories (`list.ts:22,50,78`). Encrypt and decrypt are the outliers, not the +rule. + +Worth noting where the coverage numbers sit: `keyman.encrypt.ts` and +`keyman.decrypt.ts` are both at **100 % lines, 100 % branches**. Every test +creates the directories in `beforeEach` (`encrypt.test.ts:46-47`, +`decrypt.test.ts:54-55`), so the missing guard is not a branch that went +uncovered ā€” it is a branch that was never written. Line coverage measures lines +executed, not inputs considered. + +**Half closed (Phase 1).** `keyman()` is now awaited inside a `catch`, so a +rejection is one line rather than an unhandled-rejection stack trace. The missing +`existsSync` guards ā€” and with them the menu loop surviving a failed operation ā€” +are Phase 2. + +### 1.3 šŸ”“ A missing `age.key` becomes `age -r null` + +`keyman.main.ts:73` and `:80` assert away a null: + +```ts +extractAgePublicKey(paths.keyPath)! +``` + +`extractAgePublicKey` returns `string | null` (`utils.ts:8-22`) and returns null +in three cases: the file is missing, it is unreadable, or it parses but has no +`# public key:` line. In all three it prints an error and returns ā€” and the +non-null assertion carries that null straight into an `execa` argv. + +**Verified**, both halves: + +``` +āŒ ERROR: Age key file not found at /nope/age.key +extractAgePublicKey(missing) = null +execa with null recipient THREW: ExecaError | Command failed with exit code 1: age -r null -o /tmp/x.age /etc/hosts +``` + +execa stringifies the null, so the recipient becomes the literal `"null"`. + +The two call sites fail differently, and the generate path fails worse: + +- `generateKey` runs `ssh-keygen` **first** (`generate.ts:59`) and `age` second + (`generate.ts:68`). Its `try/catch` swallows the failure into "āŒ Error + generating/encrypting key", but by then the private key is on disk in `tmpDir` + in plaintext, and the user has been told the operation failed. Nothing tells + them a key was left behind. +- `encryptKeys` has no `try/catch` at all, so it takes the Ā§1.2 path: unhandled + rejection, stack trace, session over. + +**Fix.** Resolve the recipient once, before dispatch, and treat null as a +recoverable condition: print what to run (`age-keygen -o `) and return +to the menu. The type already says this is possible; the `!` is the only thing +claiming otherwise. + +### 1.4 šŸ”“ Decrypting into `~/.ssh` silently overwrites an existing key + +`decrypt.ts:47-49` writes the decrypted key and copies the public key with no +existence check, no confirmation, and no backup. + +**Verified** that `age -o` does not refuse an existing file: + +``` +before: PRECIOUS EXISTING KEY +age -o exit=0 (overwrote) +after: secret +``` + +So selecting `prod` with the `SSH (~/.ssh)` destination replaces +`~/.ssh/id_prod` outright. If the vault copy is stale, or the folder name +happens to collide with an unrelated local key, the local key is gone ā€” and this +is the one operation in the tool that writes outside the vault, into the +directory the user's actual SSH access depends on. + +The `Local (vault/tmp)` destination has the same behaviour but a much lower cost, +since `vault/tmp` is scratch space by design. + +**Fix.** Check both output paths before decrypting anything and prompt per +collision, or refuse and name the file. A `--force` equivalent can come later; +the current default should not be "overwrite". + +### 1.5 šŸŸ  `age` or `ssh-keygen` missing is unhandled in encrypt and decrypt + +Same missing `try/catch` as Ā§1.3. `generateKey` (`generate.ts:51-76`) and +`copyKey` (`copy.ts:43-57`) both wrap their `execa` calls and report a failure; +`encryptKeys` and `decryptKeys` do not. On a machine without `age` on `PATH` ā€” +the one hard external requirement, per `CLAUDE.md` ā€” choosing Encrypt from the +menu produces an `ENOENT` stack trace rather than "install age". + +### 1.6 šŸŸ  Encrypt copies `.pub` unconditionally and aborts the batch midway + +`encrypt.ts:45`: + +```ts +fs.copyFileSync(`${keyPath}.pub`, path.join(vaultPath, `${key}.pub`)); +``` + +The selection list is built from *private* keys only (`encrypt.ts:14,17` filter +out `.pub`), so a private key with no `.pub` sibling is offered ā€” and that is a +legal state, since `ssh-keygen -y` regenerates a public key on demand and people +do delete them. + +When it happens, `age` has already written the `.age` file, so the throw leaves +the vault holding an encrypted key with no public key. Worse, the throw escapes +the `for` loop: every remaining selected key is skipped, with no output saying +so, and the process dies via Ā§1.2. + +`generate.ts:71` has the same shape but is far less likely to fire, since +`ssh-keygen` just wrote the file. + +**Fix.** Derive the public key with `ssh-keygen -y -f ` when the sibling is +absent, and wrap the loop body so one bad key costs one key rather than the +batch. + +### 1.7 šŸŸ  `/home/` is hardcoded + +`keyman.main.ts:33`: + +```ts +const homeDir = user === '@current' ? process.env.HOME || '' : `/home/${user}`; +``` + +On macOS other users live under `/Users/`, and this tool is otherwise +macOS-specific (Ā§1.9). Nothing checks the directory exists, so a wrong guess +feeds a nonexistent `sshDir` into Ā§1.2 rather than into an error message. +`main.test.ts:198-204` locks in `/home/deploy/.ssh`. + +**Fix.** `os.userInfo()` for the current user, and for a named user either look +the home directory up (`getent passwd` / `dscl`) or ask for the path outright. +Failing that, check `existsSync` and say so. + +### 1.8 šŸŸ” Keys not named `id_*` are invisible, silently + +Every discovery filter requires the prefix: `copy.ts:9`, `encrypt.ts:14,17`, +`list.ts:23,51`, and `decrypt.ts:9` reconstructs `id_${dir}`. A key called +`deploy_ed25519` cannot be listed, copied, or encrypted, and nothing says why ā€” +it simply is not in the menu. + +`generateKey` enforces the prefix (`generate.ts:43`), so keys keyman creates are +always fine. The gap only bites pre-existing keys, which is exactly the +population a key manager is adopted to take over. + +### 1.9 šŸŸ” `pbcopy` is hardcoded + +`copy.ts:49`, with the comment above it admitting the shortcut: + +```ts +// Since the environment is Darwin, we prioritize pbcopy, but we can add others for completeness +``` + +On Linux or Windows, Copy public key always fails. It fails *cleanly* ā€” the +`try/catch` reports "āŒ Failed to copy to clipboard" ā€” but the package declares +only `"node": ">=22"` in `engines` and the README says nothing, so nothing warns +before install. `xclip`/`wl-copy`/`clip.exe` by platform is a handful of lines; +alternatively print the key to stdout as a fallback so the operation is never a +dead end. + +### 1.10 šŸŸ” Smaller things + +- **`listKeys` throws on a broken symlink.** `list.ts:80` calls `fs.statSync` on + every entry in the keys directory; a dangling symlink throws `ENOENT`, and + `listKeys` has no `try/catch`, so it exits via Ā§1.2. `lstatSync`, or a + `withFileTypes` readdir, or a guard. +- **`key.replace('id_', '')` is unanchored** (`encrypt.ts:38`, `generate.ts:63`). + Every input is prefix-filtered today, so the first match *is* the prefix and + the behaviour is correct ā€” it is a trap left for whoever loosens Ā§1.8. + `replace(/^id_/, '')` costs nothing. +- **An `age` failure leaves an empty vault directory.** `generate.ts:65` creates + `//` before `generate.ts:68` runs `age`. + `generate.test.ts:150-163` asserts the `.pub` is absent afterwards but not the + directory, so this passes today. It makes the folder show up in + `decrypt`'s scan as a candidate that filters back out ā€” harmless, untidy. +- **Debug output still in shipped code.** `encrypt.ts:18-19` + (`console.log(tmpKeys); console.log(sshKeys);`) is already tracked as + `DOCS-AUDIT.md` Ā§6.4. `decrypt.ts:10` ā€” a `console.log(keyfile)` *inside a + `filter` callback*, printing one line per vault directory ā€” is not, and is the + more visible of the two. + +--- + +## 2. Security + +### 2.1 šŸ”“ Decrypted private keys are world-readable before the chmod + +`decrypt.ts:47-50` decrypts, then copies, then chmods ā€” in three separate +processes: + +```ts +await execa('age', ['-d', '-i', ageKey, '-o', privateKeyOut, encryptedKey]); +await execa('cp', [publicKey, publicKeyOut]); +await execa('chmod', ['600', privateKeyOut]); +``` + +**Verified** what `age` creates, and what `mkdirSync` at `main.ts:41` creates: + +``` +-rw-r--r-- ā€¦/out ā† the decrypted private key, as age leaves it +drwxr-xr-x ā€¦/tmpdir ā† vault/tmp, as keyman creates it +``` + +So a plaintext private key exists at `0644` for the lifetime of two process +spawns, inside a `0755` directory any local user can traverse. If the `chmod` +fails or the process is killed in between, it stays `0644` ā€” and because +`decryptKeys` has no `try/catch` (Ā§1.5), a failing `chmod` also kills the +session before the next key is even attempted. + +**Fix.** `fs.chmodSync` immediately after `age` returns rather than a third +spawn; create `tmpDir` with `{mode: 0o700}` and `~/.ssh` likewise if it is +missing. Replacing `cp` and `chmod` with `fs.copyFileSync` / `fs.chmodSync` also +removes two shell-outs that do not work on Windows and cuts three spawns per key +to one. + +### 2.2 šŸŸ  The passphrase is passed on the `ssh-keygen` command line + +`generate.ts:53`: + +```ts +const args = ['-t', algorithm, '-f', keyPath, '-N', password, '-C', identity]; +``` + +argv is world-readable on both Linux (`/proc//cmdline`) and macOS +(`ps -o command`) for the lifetime of the process. Any other user on the machine +can read the passphrase of a key being generated. `generate.test.ts:78-87` +asserts this exact argv. + +**Fix.** Omit `-N` entirely and let `ssh-keygen` prompt on the tty ā€” it already +asks twice and confirms, so keyman's own password prompt (`generate.ts:26-33`) +can go away rather than being replaced. That keeps the passphrase off argv +without keyman ever holding it. + +### 2.3 šŸŸ  The age recipient is trusted from a comment, never verified + +`extractAgePublicKey` (`utils.ts:16`) regexes the recipient out of a comment +line in the identity file: + +```ts +fileContents.match(/^# public key:\s*(age1[^\s]+)/m) +``` + +Nothing checks it corresponds to the private key in that same file. Edit the +comment ā€” or concatenate two key files ā€” and every subsequent encryption goes to +a recipient the local identity cannot decrypt. The failure surfaces only later, +at decrypt time, on keys that may no longer exist in plaintext anywhere. + +**Fix.** `age-keygen -y ` derives the public key *from the private key* +and is exactly the tool for this. Note that `age-keygen` is currently not +invoked anywhere in the source, despite `CLAUDE.md` listing it among the +binaries keyman shells out to (Ā§5.5). + +### 2.4 šŸŸ” Nothing manages the plaintext left in `vault/tmp` + +Decrypted keys accumulate in `vault/tmp` indefinitely. There is no shred +operation, no warning on exit, and keyman never writes the `.gitignore` its own +README (`README.md:25-26`, `:148`) tells the user to write by hand. The only +signal is the šŸ”“ marker in `listKeys`, which the user has to go looking for. + +A "Clear decrypted keys" menu entry and a `.gitignore` written alongside the +vault on first run would cost little and close the most likely way a private key +reaches a public repository ā€” which is the threat this tool exists to address. + +--- + +## 3. Unimplemented and dead + +### 3.1 šŸŸ  There is no `--help` + +`keyman.cli.ts` handles `--print-config`, `--version`/`-V`, and +`self-update`/`upgrade`, then falls through to the interactive session. `--help` +is not among them, and neither is any unknown-flag handling. + +**Verified.** `keyman --help` with no tty: + +``` +šŸ“ Vault Root: ā€¦ +? Specify USER (default: @current): (@current) +ā€¦/@inquirer/core/dist/lib/create-prompt.js:67 + reject(new ExitPromptError(`User force closed the prompt with ${code} ${signal}`)); +``` + +Two problems in one output. `--help` starts a session instead of describing the +tool, and because of Ā§1.2 the resulting `ExitPromptError` is an unhandled +rejection with a stack trace. That second half is what a user gets from **Ctrl-C +at any prompt** ā€” the normal way to leave an interactive CLI produces a crash +dump. + +`keyman --vault foo` is likewise accepted and ignored. + +**Fix.** `--help` listing the flags, the two subcommands, and the `KEYMAN_*` +environment variables (Ā§5.3); an unknown-flag error; and a `catch` in +`keyman.cli.ts` that treats `ExitPromptError` as "goodbye" and anything else as +a one-line error. nopy uses Commander for this; keyman need not, but it does +need the behaviour. + +**Closed (Phase 1).** `src/keyman.args.ts` owns the parse and the help text; the +`catch` around `keyman()` in `keyman.cli.ts` turns `ExitPromptError` into +"šŸ‘‹ Goodbye!" and exit 0, and anything else into one line and exit 1. + +### 3.2 šŸŸ” `flagValue` accepts things that are not values + +`keyman.cli.ts:24-27` is `args.indexOf(name)` and `args[index + 1]`: + +- `--channel=main` is not recognised. +- `--channel` as the last argument yields `undefined`. +- `keyman self-update --channel --force` sets the channel to `"--force"`, which + is cast to `Channel` (`cli.ts:47`) and flows into the dist-tag lookup at + `update.ts:176` as a key that cannot exist. The registry answers, the tag is + absent, and the user is told "Could not reach " ā€” which is false. + +Validating against the three legal channels would turn all three into one clear +error. + +**Closed (Phase 1).** `parseArgs` accepts both `--flag value` and `--flag=value`, +rejects a flag swallowed as another flag's value, and validates `--channel` +against `CHANNELS`. + +### 3.3 šŸŸ” The `resolution` merge machinery has no effect + +`keyman.config.ts` carries `ResolutionStrategy`, `KeymanResolutionConfig`, +`mergeValue` and `mergeConfigs` ā€” roughly 45 lines, imported from nopy's design. +Every property in `KeymanConfigSchema` is a `z.string()`. For two strings, +`mergeValue` returns `childValue` in the `override` branch (`:121-123`) and +returns `childValue` again from the primitive fallthrough (`:156`). The two +strategies are indistinguishable for every key the schema permits, and the +array-concat and deep-merge branches are unreachable through a valid config ā€” +unknown keys pass through the merge but are then stripped by +`KeymanConfigSchema.parse` (Ā§3.5). + +So the documented knob does nothing. The doc comment at `:186-194` advertises it: + +```json +{ "vaultRoot": "../vault", "resolution": { "vaultRoot": "override" } } +``` + +and `config.test.ts:212` ā€” "honours an explicit override strategy" ā€” passes for +a case where plain merge gives the same answer, so the test does not distinguish +them either. + +This is a choice to make, not a bug to fix. Either drop the machinery and the +comment, or keep it deliberately as the shape a future object-valued or +array-valued option would need ā€” and say so in a comment, since right now it +reads as functional. + +### 3.4 šŸŸ” `getConfigPaths()` is exported, tested, and called by nothing + +`keyman.config.ts:265` is used only by `config.test.ts:122,131`. It is not +re-exported from `src/index.ts` and not called by the CLI. nopy's equivalent +feeds `nopy.main.ts:64`. + +The absence is felt: `--print-config` prints the *resolved paths* only, so there +is no way to ask which config files were consulted. That information exists only +as a stderr side effect of `loadConfig` ("āœ… Loaded configuration from ā€¦"), which +is not machine-readable and is interleaved with warnings. Folding +`getConfigPaths()` into the `--print-config` JSON makes the function earn its +keep and makes the escape hatch answer the question it is for. + +### 3.5 šŸŸ” A typo in `.keymanrc.json` is silent + +`KeymanConfigSchema` is a plain `z.object`, which strips unknown keys. + +**Verified.** With `{"vaultRoot":"./v","vaultroot":"typo", ā€¦}`, the lowercase key +is dropped without a word and `--print-config` reports the vault from the +correct key. Had only the typo been present, the user would get the `vault` +default and no clue. + +`.strict()` ā€” or keeping the strip and logging the leftover keys as a warning ā€” +turns a silently wrong vault into one line of output. Since `loadConfig` already +degrades to defaults rather than throwing, a warning fits the module's existing +posture better than a hard failure. + +### 3.6 šŸŸ  "Support for key rotation" does not exist + +`README.md:11`. `grep -rn "rotat" packages/keyman/src/` returns nothing. Already +tracked as `DOCS-AUDIT.md` Ā§2.10, still open. Rotation is a genuinely useful +operation for this tool ā€” generate a replacement, encrypt it, keep the old one +until the new one is deployed ā€” so this is worth building rather than deleting. + +### 3.7 šŸŸ” "Copy public key and create README" + +`encrypt.ts:44` says it; no README is written. Suggestively, +`decrypt.test.ts:75` places a `README.md` inside the keys directory as a +fixture, so a per-key README appears to have been the intent once. Either build +it or drop the half of the comment that lies. + +--- + +## 4. Public API and packaging + +### 4.1 šŸŸ” A shebang on the library entry point + +`src/index.ts:1` is `#!/usr/bin/env node`. The bin is `dist/keyman.cli.js` +(`package.json:28`); `index.ts` is the `exports["."]` target and is only ever +imported. nopy's `src/index.ts` has no shebang. Harmless, and a copy-paste +artefact. + +### 4.2 šŸŸ” The exported functions' types are not exported + +`src/index.ts:2` exports `loadConfig` and `resolveConfigPaths`. It does not +export `KeymanConfig`, `KeymanConfigFile`, `ResolutionStrategy` or +`KeymanResolutionConfig`, so a TypeScript consumer cannot name what `loadConfig` +returns or what `resolveConfigPaths` takes. This is the same one-line omission +`CLAUDE.md` already records for nopy's `CubePackageRef`. + +### 4.3 šŸŸ” `export * from './keyman.main.js'` exports only `keyman()` + +The five operation modules and `extractAgePublicKey` are not on the public +surface, so the package is consumable as a library only as "run the entire +interactive menu". That may well be intended ā€” but then `loadConfig` and +`resolveConfigPaths` being exported is the odd part, since a consumer can obtain +the paths and do nothing with them. + +### 4.4 šŸŸ” Update-module constants are half re-exported + +`keyman.update.ts` exports `SCOPE`, `UPDATE_CACHE_DIR`, `UPDATE_CACHE_FILE`, +`DEFAULT_FETCH_TIMEOUT_MS` and `DEFAULT_CONFIG_TIMEOUT_MS`; `src/index.ts:12-31` +re-exports neither, while re-exporting `DEFAULT_CHECK_INTERVAL_MS` and +`NPMJS_REGISTRY`. Pick one rule. + +--- + +## 5. Documentation drift + +### 5.1 šŸŸ  `DOCS-AUDIT.md` lists Ā§1.1 under *checked and accurate* + +`DOCS-AUDIT.md:826-827`: + +> **keyman config** ā€” priority (`VAULT_ROOT` > file > defaults), the four default +> values, and the vault layout match `keyman.config.ts` and `keyman.encrypt.ts`. + +The first two clauses are correct. The third holds only because +`keyman.encrypt.ts` hardcodes `keys` ā€” checking the documented layout against +the file that ignores the config is what made Ā§1.1 invisible. The entry should +move out of section 7 and point at Ā§1.1. + +### 5.2 šŸŸ  `README.md` operations list ā€” still open + +`DOCS-AUDIT.md` Ā§2.10, re-verified: `README.md:90-96` lists four menu entries; +`main.ts:54-61` has six. `Copy public key` and `Generate key` are undocumented ā€” +the latter being the only in-tool way to create a key, which is why the Quick +Start at `README.md:33` tells the user to run `ssh-keygen` by hand. +`README.md:93` says encrypt takes keys "from `vault/tmp/`"; `encrypt.ts:12-20` +unions `~/.ssh` and tmp and offers both. + +### 5.3 šŸŸ  The README documents none of the CLI surface + +`README.md` covers the interactive menu and the config file. It does not mention: + +- `self-update` / `upgrade`, `--dry-run`, `--force`, `--channel`, `--registry` +- `--version` / `-V`, `--print-config` +- `KEYMAN_REGISTRY`, `KEYMAN_REGISTRY_TOKEN`, `KEYMAN_NO_UPDATE_CHECK`, + `KEYMAN_PACKAGE_MANAGER` +- the once-a-day update check, or that it is disabled when `CI` is set + +`README.PUBLISH.md:552-578` documents all of it, but `package.json:37-41` ships +only `dist`, `README.md` and `LICENSE` ā€” so a reader on the registry sees none of +it. This is the same shape as the nopy README problem closed as +`DOCS-AUDIT.md` Ā§2.9, and keyman is now the worse of the two. + +### 5.4 šŸŸ  The README presents a configurable layout that is half-real + +`README.md:46-70` documents `keysDir` and `tmpDir` as configuration, and +`:72-86` draws the default tree. Per Ā§1.1 the first is only half true. Whichever +way Ā§1.1 is resolved, this section needs an edit. + +### 5.5 šŸŸ” `CLAUDE.md` names a binary keyman never runs + +> Encryption shells out to `age` / `age-keygen` / `ssh-keygen`, which must be on +> `PATH`. + +`age-keygen` appears nowhere in `packages/keyman/src`. It appears in +`README.md:22` as a manual setup step, which is presumably where the claim came +from. Either note it as a prerequisite the user runs rather than something +keyman invokes, or make Ā§2.3 true and turn the claim into fact. + +`CLAUDE.md` also does not mention that `decryptKeys` shells out to `cp` and +`chmod` (`decrypt.ts:49-50`) ā€” see Ā§2.1, where the recommendation is to stop. + +### 5.6 āœ… The update module has not drifted from nopy's + +`keyman.update.ts` and `nopy.update.ts` are described in `CLAUDE.md` as "two +near-identical copies of one module", the duplication deliberate. Diffed with +package names normalised: **every difference is a doc comment.** No behavioural +drift at all. The stated risk of the duplication has not materialised; nopy's +copy simply carries fuller comments, and porting the better ones over would cost +nothing. + +--- + +## 6. Checked and accurate + +- **Config precedence.** `VAULT_ROOT` > config file > defaults + (`config.ts:249-259`), matching `README.md:59-70`. Verified via + `--print-config`. +- **Upward traversal and the home-directory config.** `findConfigFiles` + (`config.ts:85-110`) collects root-first and de-duplicates the home config + when it is also an ancestor (`:105`). +- **`loadConfig` never throws.** Invalid JSON is skipped per file (`:220-226`) + and a failed final validation degrades to defaults (`:232-241`) ā€” which is the + documented difference from nopy's behaviour, and it holds. +- **`extractAgePublicKey` is honest about failure.** It returns `null` in every + failure mode and prints why; the defect in Ā§1.3 is entirely in the caller's + `!`. +- **The menu loop.** Returns to the menu after every operation + (`main.ts:45-91`), as `README.md:97` says. +- **The four default values** and the `id_.age` / `id_.pub` layout + inside a per-key folder, as drawn at `README.md:72-86`. +- **`listKeys` status logic** (`list.ts:127-128`) matches its legend and the + README's, including the šŸ”“ state. +- **The update module**, in full ā€” see Ā§5.6. + +--- + +## Suggested order of attack + +**1 ā€” the crashes, together.** Ā§1.2, Ā§1.3, Ā§1.5 and Ā§1.10's `statSync` are all +the same shape: an unguarded call in a function with no error boundary, reaching +a `keyman()` that is never awaited. One `catch` in `keyman.cli.ts` that +distinguishes `ExitPromptError` from a real failure, plus `existsSync` guards and +`try/catch` in encrypt and decrypt, closes all of them and most of Ā§3.1's second +half. This is the smallest change with the largest effect on what a first run +feels like. + +**2 ā€” Ā§1.4 and Ā§2.1.** Both are in `decryptKeys`, both are about writing outside +the vault, and one of them destroys data. Replacing `cp`/`chmod` with the `fs` +equivalents is part of the same edit. + +**3 ā€” Ā§1.1.** Mechanical, but it changes four test assertions, so it wants to be +its own commit. Fix `DOCS-AUDIT.md` Ā§5.1 in the same one. + +**4 ā€” decide on Ā§3.3 and Ā§3.6.** Both are features the documentation claims and +the code does not have; both are decisions rather than fixes. Rotation is worth +building. The `resolution` machinery probably is not, and deleting it would take +`keyman.config.ts` from 267 lines to around 220. + +**5 ā€” Ā§5.2, Ā§5.3 and Ā§5.4** are one rewrite of `README.md`. It is the only +document that ships, and it currently describes two thirds of the menu and none +of the command line. + +**6 ā€” the rest.** Ā§2.2 (drop the passphrase prompt, let `ssh-keygen` ask), Ā§2.3 +(`age-keygen -y`), Ā§2.4 (a shred operation), Ā§1.6 through Ā§1.9, Ā§3.2, Ā§3.4, +Ā§3.5, and the Ā§4 one-liners. diff --git a/packages/keyman/docs/PLAN.md b/packages/keyman/docs/PLAN.md new file mode 100644 index 0000000..73f7800 --- /dev/null +++ b/packages/keyman/docs/PLAN.md @@ -0,0 +1,433 @@ +# keyman remediation plan + +Turns [`AUDIT.md`](./AUDIT.md) into sequenced work. Each phase is one commit, +independently landable, gate-green on its own. Section references (Ā§) are to +`AUDIT.md`. + +Ordering is by *blast radius per unit of risk*, not by severity: the error +boundary comes first because it makes every later phase's failure mode legible, +and the config threading comes late because it is the only phase that rewrites +existing test assertions. + +## Verified before planning + +Four things the fixes depend on, checked by running them rather than assumed ā€” +two of them changed the prescription: + +| Check | Result | Consequence | +| --- | --- | --- | +| `ssh-keygen` with `-N` omitted | Prompts `Enter passphrase ā€¦ (empty for no passphrase)` **and** confirms | Ā§2.2 fix works: omit `-N`, inherit stdio, keyman never holds the passphrase | +| `ssh-keygen -y -f ` | **Prompts for the passphrase** | Ā§1.6 fix cannot be a silent spawn ā€” needs `stdio: 'inherit'` and a skip path | +| `@inquirer/core` from keyman | `ERR_MODULE_NOT_FOUND` ā€” transitive via `inquirer`, not a direct dep | Detect `ExitPromptError` by `error.name`, never by import | +| `z.strictObject` in zod 4.4.3 | Available; reports `unrecognized_keys` with a `keys` array | Ā§3.5 has a hard-failure option, though the plan prefers a warning | + +## What is not a breaking change + +Per Ā§4.3, `src/index.ts` exports only `keyman`, `loadConfig`, +`resolveConfigPaths` and the update module. `encryptKeys`, `decryptKeys`, +`generateKey`, `listKeys`, `copyKey` and `extractAgePublicKey` are **not** on the +public surface, so every signature change below is internal. Phases 2ā€“6 are not +semver-breaking. + +The one user-visible behaviour change is Phase 5 ā€” see [Migration](#migration). + +## Gate discipline + +`lint:ci` ā†’ `typecheck` ā†’ `test:coverage` runs on `pre-push` and in CI. Two +standing constraints: + +- **Every phase lands its tests with its fix.** No phase may leave a red gate, + so there is no "write the failing tests first" commit. +- **`keyman.cli.ts` is excluded from coverage** (`vitest.config.ts:18`). Per + `CLAUDE.md`, *adding logic to those files means moving it somewhere covered* ā€” + which is why Phase 1 extracts argument parsing into a new module rather than + growing `cli.ts`. + +Per-phase verification is `pnpm --filter @bitsquare/keyman run test`; the full +gate (`pnpm run lint:ci && pnpm run typecheck && pnpm run test:coverage`) before +each push. + +--- + +## Phase 1 ā€” Error boundary, `--help`, argument validation + +Closes Ā§3.1, Ā§3.2, and the second half of Ā§1.2 (the crash dump). + +First because it is pure addition, touches no operation module, and converts +every latent throw in phases 2ā€“6 from a stack dump into a line of text. The +`ExitPromptError` half is independently worth shipping: today **Ctrl-C at any +prompt** produces a crash dump. + +**New file `src/keyman.args.ts`** (covered by the gate, unlike `cli.ts`): + +- `parseArgs(argv: string[]): ParsedArgs` ā€” supports `--flag value` *and* + `--flag=value`, rejects a flag consumed as another flag's value, rejects + unknown flags, and validates `--channel` against `'latest' | 'next' | 'main'` + so Ā§3.2's false "Could not reach " cannot happen. +- `helpText(): string` ā€” flags, both subcommands, and the four `KEYMAN_*` + variables. This is the text Phase 9 keeps in step with the README. + +**`src/keyman.cli.ts`** stays wiring: dispatch on the parse result, and + +```ts +try { + await keyman(); +} catch (error) { + if ((error as { name?: string }).name === 'ExitPromptError') { + console.log('\nšŸ‘‹ Goodbye!\n'); + process.exit(0); + } + console.error(`āŒ ${error instanceof Error ? error.message : error}`); + process.exit(1); +} +``` + +`error.name`, not `instanceof` ā€” `@inquirer/core` is not a direct dependency and +does not resolve from this package. + +**Tests** ā€” new `tests/args.test.ts`: each rejection, both flag forms, the +channel whitelist, and that `helpText()` names every flag `parseArgs` accepts +(so the two cannot drift). + +**Done when** `keyman --help` prints usage and exits 0 without loading config or +prompting; `keyman --bogus` errors; Ctrl-C prints Goodbye and exits 0. + +--- + +## Phase 2 ā€” Guards and error handling in encrypt/decrypt + +Closes Ā§1.2 (first half), Ā§1.5, and Ā§1.10's `statSync`. + +- `encrypt.ts:13,16` and `decrypt.ts:8` ā€” `existsSync` guard, falling through to + the "āš ļø No ā€¦" message each function already has but cannot currently reach. +- `main.ts:40-41` ā€” create `keysDir` alongside `vaultRoot` and `tmpDir`. Use + `{recursive: true, mode: 0o700}` now, so Phase 4 does not have to revisit it. +- Wrap the `age` spawns in both functions. An `ENOENT` on the binary gets its own + message ("`age` was not found on PATH") ā€” it is the one hard external + requirement and currently the least legible failure. +- `list.ts:80` ā€” `readdirSync(dir, {withFileTypes: true})` instead of + `statSync` per entry, which also drops N stat calls and fixes the broken-symlink + throw. +- Delete the debug logging while in these files: `encrypt.ts:18-19` and + `decrypt.ts:10` (Ā§1.10). That also closes `DOCS-AUDIT.md` Ā§6.4's open bullet. + +**Tests** ā€” the cases the current suite structurally cannot have, because every +`beforeEach` pre-creates the directories: encrypt with no `~/.ssh`, encrypt with +no tmp, decrypt with no `/keys`, each asserting the warning and no throw. +Plus `list` with a dangling symlink in the keys directory. + +**Note on coverage.** `encrypt.ts` and `decrypt.ts` are at 100 % lines and +branches *today*. The number will not move; the tests are the point. + +**Done when** a first run against an empty vault can reach every menu entry and +return to the menu. + +--- + +## Phase 3 ā€” Resolve the age recipient once, and derive it properly + +Closes Ā§1.3 and Ā§2.3, and makes `CLAUDE.md`'s `age-keygen` claim true (Ā§5.5). + +Two changes that belong together because both are about the recipient: + +1. **`utils.ts` ā€” derive, don't scrape.** `extractAgePublicKey` currently regexes + `# public key:` out of a comment (`utils.ts:16`) and trusts it. Replace with + `age-keygen -y `, which derives the public key *from the private key* + and cannot disagree with it. Keep the comment parse as a fallback for when + `age-keygen` is absent, behind a warning that the recipient is unverified. + The function becomes `async`. +2. **`main.ts:73,80` ā€” delete both `!`.** Resolve the recipient once before the + `switch`, and treat `null` as recoverable: print the remedy + (`age-keygen -o `) and `break` back to the menu. This is the whole of + Ā§1.3 ā€” the type already said null was possible. + +Sequencing matters inside the phase: fix the call site first. Without it, a +missing key file still reaches `age -r null`, and the generate path still leaves +a **plaintext private key in `tmpDir`** after telling the user the operation +failed. + +**Tests** ā€” `utils.test.ts` gains the `age-keygen -y` path with `execa` mocked, +the fallback-with-warning path, and the both-unavailable path. `main.test.ts` +gains: missing recipient ā†’ neither `generateKey` nor `encryptKeys` is called, a +remedy is printed, and the menu loop continues. + +**Done when** `keyman` against a vault with no `age.key` reaches the menu, +refuses generate and encrypt with a remedy, and still offers list and decrypt. + +--- + +## Phase 4 ā€” Decrypt: stop overwriting, stop the 0644 window + +Closes Ā§1.4 and Ā§2.1. The highest-value phase ā€” Ā§1.4 is the only finding that +destroys data the user did not ask to touch. + +- **Collision check before any decryption.** Both output paths, both modes. + Prompt per collision, defaulting to skip; `~/.ssh` deserves the friction more + than `vault/tmp` does, but the check is the same code. +- **Replace the shell-outs** (`decrypt.ts:49-50`) with `fs.copyFileSync` and + `fs.chmodSync`. Three spawns per key become one, it works on Windows, and it + removes a `cp` that overwrites unconditionally. +- **Close the permission window.** Verified: `age -o` creates the file `0644` + and `mkdirSync` creates `vault/tmp` as `0755`, so a plaintext key is + world-readable for the duration of two process spawns ā€” and stays `0644` if the + `chmod` fails. `fs.chmodSync` immediately after `age` resolves; `mode: 0o700` + on the directory (already done in Phase 2); create `~/.ssh` `0700` if absent. + +**Test rework ā€” the fiddliest in the plan.** `decrypt.test.ts` asserts on the +mocked spawns: `argsOf('cp')` (`:98,111`) and `argsOf('chmod')` (`:99,112`) both +disappear, and `execa` is mocked with a bare `mockResolvedValue` (`:57`) that +writes no output file. Once `copyFileSync` is real it needs a real file, so the +mock must write to its `-o` argument the way `encrypt.test.ts:51-54` already +does. Assert the on-disk result and mode instead of the argv ā€” a better test +than the one it replaces, since it checks the outcome rather than the mechanism. +`execa` call counts also change (`:123`: six spawns ā†’ two). + +**Done when** decrypting onto an existing key requires a confirmation, and the +decrypted key is never observable at anything but `0600`. + +--- + +## Phase 5 ā€” Thread `keysDir` and `tmpDir` through encrypt and decrypt + +Closes Ā§1.1 and the `DOCS-AUDIT.md` entry in Ā§5.1. + +Mechanical, but it is the one phase that rewrites assertions that pass today, so +it stays its own commit with nothing else in it. + +- `encryptKeys(sshDir, keysDir, tmpDir, pubkey)` ā€” drop `vaultDir`, delete the + hardcoded `path.join(vaultDir, 'keys')` (`encrypt.ts:38`). +- `decryptKeys(sshDir, keysDir, tmpDir, ageKey)` ā€” drop `vaultDir`, delete the + hardcoded joins at `decrypt.ts:7,39,43`. +- `main.ts:76,84` ā€” pass `paths.keysDir` and `paths.tmpDir`. Neither function has + any remaining use for `vaultRoot`, so the parameter goes rather than becoming a + second source of truth. + +**Assertions to change** ā€” all four, named so the diff is reviewable: + +| Location | Today | After | +| --- | --- | --- | +| `main.test.ts:164-175` | "encrypts keys into the vault root", asserts `paths.vaultRoot` | asserts `paths.keysDir`, `paths.tmpDir` | +| `main.test.ts:177-187` | asserts `paths.vaultRoot` | asserts `paths.keysDir`, `paths.tmpDir` | +| `encrypt.test.ts:95,115,128-129` | `path.join(vaultDir, 'keys', ā€¦)` | `path.join(keysDir, ā€¦)` | +| `decrypt.test.ts:53,89` | `keyDir = path.join(vaultDir, 'keys')` | `keysDir` passed in directly | + +**New test, the one that would have caught this:** a config with +`keysDir: 'encrypted'` and `tmpDir: 'plain'`, encrypt a key, then list it, and +assert the listing shows it in `[Vault]`. That round trip fails today and is the +regression worth owning. + +**Also in this commit:** move the `DOCS-AUDIT.md:826-827` bullet out of *checked +and accurate* and point it at this finding. It was verified against +`keyman.encrypt.ts` ā€” the file that ignores the config ā€” which is precisely how +Ā§1.1 stayed invisible. + +--- + +## Phase 6 ā€” Generate: passphrase off argv, and `.pub` recovery + +Closes Ā§2.2, Ā§1.6, and Ā§1.10's leftover-directory bullet. + +**Passphrase (Ā§2.2).** Verified: omitting `-N` makes `ssh-keygen` prompt *and* +confirm. So delete keyman's own password prompt (`generate.ts:26-33`), omit `-N`, +and spawn with `stdio: 'inherit'`. The passphrase never enters keyman's memory +and never reaches argv ā€” strictly better than routing it more carefully, and it +deletes code. `generate.test.ts:78-87` loses `-N`/`'pw'` from the expected argv +and the password-prompt case goes away. + +**Missing `.pub` (Ā§1.6).** The selection list is built from private keys only, so +an orphan private key is offered and `copyFileSync` throws *after* `age` has +written the `.age` file ā€” leaving a vault entry with no public key and killing +the rest of the batch. Fix in two parts: + +- Derive it with `ssh-keygen -y -f ` when the sibling is absent. **Verified + that this prompts for a passphrase on an encrypted key**, so it needs + `stdio: 'inherit'` and a clean skip when the user cannot or will not supply it + ā€” not a silent spawn whose stdout is captured. +- Wrap the loop body in `encrypt.ts:36-48` per key, so one bad key costs one key. + Report the failures at the end rather than dying at the first. + +**Leftover directory.** `generate.ts:65` creates `//` before +`age` runs at `:68`. Move the `mkdirSync` after `age` succeeds. + +--- + +## Phase 7 ā€” Config: warn on typos, decide on the dead machinery + +Closes Ā§3.5, Ā§3.4, and asks for a decision on Ā§3.3. + +**Typos (Ā§3.5).** Verified: `{"vaultroot": "ā€¦"}` is silently stripped by +`z.object`. Warn per file rather than failing ā€” diff `Object.keys(rawConfig)` +against the schema keys plus `resolution` inside the existing per-file loop +(`config.ts:210-227`), where the filename is in hand. That names the offending +file, which `z.strictObject` cannot do from the merged result, and it preserves +the module's documented posture of degrading to defaults rather than throwing. +(`z.strictObject` is available in zod 4.4.3 and reports `unrecognized_keys` with +a `keys` array, if a hard failure is preferred later.) + +**`getConfigPaths` (Ā§3.4).** Add it to the `--print-config` JSON as +`configFiles`. The function is currently exercised only by its own test, and +`--print-config` currently cannot answer *which files were read* ā€” that exists +only as unstructured stderr from `loadConfig`. One change fixes both. + +**Decision needed ā€” the `resolution` machinery (Ā§3.3).** Roughly 45 lines +(`config.ts:23-30,115-157`) that cannot affect a valid config, because every +schema property is a `string` and both strategies return `childValue` for +primitives. `config.test.ts:212` "honours an explicit override strategy" passes +either way. + +- **Recommended: delete it**, along with the doc comment at `:186-194` that + advertises it. `keyman.config.ts` goes from 267 lines to roughly 220, and the + config file stops documenting a knob that does nothing. +- **Alternative: keep it** as the shape a future array- or object-valued option + would need ā€” but then say so in a comment, because today it reads as + functional, and make `config.test.ts:212` assert something that distinguishes + the two strategies (which requires a non-string property to exist first). + +Deleting is the smaller lie. It also diverges from nopy, where the machinery +*is* load-bearing ā€” worth a line in `CLAUDE.md` so the divergence reads as +deliberate. + +--- + +## Phase 8 ā€” Portability and the gaps that make keys invisible + +Closes Ā§1.7, Ā§1.8, Ā§1.9, Ā§2.4. Independent of each other; split if any grows. + +- **Clipboard (Ā§1.9).** `pbcopy` / `wl-copy` / `xclip` / `clip.exe` by platform, + falling back to printing the key to stdout so the operation is never a dead + end. Delete the comment at `copy.ts:46-48` that admits the shortcut. +- **Home directory (Ā§1.7).** `os.userInfo()` for the current user; for a named + user, look the home directory up rather than assuming `/home/` ā€” wrong on + the one platform the tool currently supports. Check `existsSync` and say so, + instead of feeding a nonexistent path into a `readdir`. + `main.test.ts:198-204` changes. +- **Non-`id_*` keys (Ā§1.8).** Relax the filters (`copy.ts:9`, `encrypt.ts:14,17`, + `list.ts:23,51`) to *any* private key with a recognisable header, or at minimum + print a count of the keys that were skipped and why. Today a key named + `deploy_ed25519` is simply absent from the menu ā€” and pre-existing keys are the + population a key manager is adopted to take over. `decrypt.ts:9` reconstructs + `id_${dir}` from the folder name, so the vault layout has the assumption baked + in; relaxing discovery means storing the real filename per key, which is the + largest single item in this plan. **Size it before committing to it** ā€” a + skipped-key count is a tenth of the work and closes most of the surprise. +- **Plaintext hygiene (Ā§2.4).** A "šŸ§¹ Clear decrypted keys" menu entry, and write + a `.gitignore` next to the vault on first run covering `age.key` and `tmp/` ā€” + which `README.md:25-26` currently tells the user to do by hand. This is the + cheapest guard against the exact failure the tool exists to prevent. + +--- + +## Phase 9 ā€” Documentation + +Closes Ā§5.2, Ā§5.3, Ā§5.4, Ā§5.5. Last, so it documents what the code now does. + +- **`README.md` ā€” the only shipped document** (`package.json:37-41` ships `dist`, + `README.md`, `LICENSE`). Currently describes four of six menu entries, invents + key rotation, and mentions none of `self-update`, `--print-config`, + `--version`, `--help`, or the four `KEYMAN_*` variables. `README.PUBLISH.md` + has all of it and never reaches a reader on the registry. Reuse Phase 1's + `helpText()` as the source for the CLI section so the two cannot drift. +- **`README.md:46-86`** ā€” the configuration and vault-layout sections, now that + Phase 5 makes `keysDir`/`tmpDir` real, plus the migration note below. +- **`README.md:11`** ā€” drop "Support for key rotation" unless Phase 10 lands + first. +- **`README.md:33`** ā€” stop telling the user to shell out to `ssh-keygen`; the + Generate operation exists. +- **`CLAUDE.md`** ā€” `age-keygen` becomes true in Phase 3; note that `cp`/`chmod` + are gone (Phase 4) and record the `resolution` divergence from nopy (Phase 7). +- **`AUDIT.md`** ā€” mark findings closed, keeping their text as the record, the way + `DOCS-AUDIT.md` does. + +--- + +## Phase 10 ā€” Key rotation (decision required) + +Ā§3.6. `README.md:11` has advertised it since before this audit; +`grep -rn "rotat" packages/keyman/src/` returns nothing. + +Unlike the rest of this plan it is a feature, not a repair, and it is the one +item that could reasonably be dropped instead. **Recommendation: build it** ā€” +rotation is the operation that makes a key vault worth having, and the pieces all +exist by Phase 6 (generate under a new name, encrypt, keep the old key until the +replacement is deployed, then shred). Sketch: + +1. Pick an existing vault key. +2. Generate a replacement into `tmpDir` under a versioned name. +3. Encrypt it alongside the current one ā€” never replacing it. +4. Report both public keys, so the new one can be deployed before the old one + goes. +5. A separate "retire" step that removes the superseded key once the user + confirms. + +Steps 3 and 4 are the whole value: a rotation that atomically replaces the key is +a rotation that locks you out of the host you were rotating for. If this is +deferred, delete the README claim in Phase 9 instead. + +--- + +## Migration + +Phase 5 is the only user-visible change. Anyone with a custom `keysDir` or +`tmpDir` currently has a **split vault** ā€” `generate` and `list` on the +configured directory, `encrypt` and `decrypt` on `/keys` and +`/tmp`. After Phase 5 all five agree on the configured directory, so +anything written by `encrypt` before the upgrade needs moving: + +```sh +mv /keys/* // +``` + +Nobody on the defaults is affected, since the two halves coincide there. The +README gets this as a note, and it is worth a line in the release notes for +whichever version carries Phase 5. + +## Rollout + +Per `CLAUDE.md`: bump `packages/keyman/package.json`, land on `main`, then tag +`keyman-v`. + +- **Snapshots come free.** Every push to `main` publishes + `-main..g` to Gitea under the `main` dist-tag, so each phase + is installable for testing without a release. `pnpm run try:snapshot` installs + one into a throwaway project. +- **Suggested cut points.** After Phase 4 as `0.6.0` ā€” error boundary, guards, + recipient handling and the data-loss fix, which is the set worth getting to + users first. After Phase 9 as `0.7.0`, carrying the Phase 5 migration note. +- **keyman can reach npmjs.** It has no `workspace:*` dependencies (`execa`, + `inquirer`, `semver`, `zod` only), so `scripts/linked-deps.mjs` has nothing to + block on ā€” unlike `nopy`, which `CLAUDE.md` records as gated behind + `nopy-cubes` shipping. keyman has never been published to npmjs; `0.6.0` could + be the first, and versions being `0.x.y` rather than `1.0.0-alphaN` means the + `latest` dist-tag will now actually move. +- **`pnpm publish`, never `npm publish`** ā€” no `workspace:` ranges here, but the + rule is repo-wide and `scripts/verify-pack.mjs` enforces it in both workflows. + +## Sequencing at a glance + +``` +1 cli boundary + --help + args Ā§3.1 Ā§3.2 Ā§1.2(half) isolated, pure addition +2 guards in encrypt/decrypt/list Ā§1.2 Ā§1.5 Ā§1.10 new tests only +3 age recipient, once and derived Ā§1.3 Ā§2.3 Ā§5.5 signature ā†’ async +4 decrypt: no clobber, no 0644 Ā§1.4 Ā§2.1 reworks decrypt.test.ts + ā”€ā”€ cut 0.6.0 ā”€ā”€ +5 thread keysDir/tmpDir Ā§1.1 Ā§5.1 rewrites 4 assertions +6 generate: -N gone, .pub recovery Ā§2.2 Ā§1.6 Ā§1.10 reworks generate.test.ts +7 config: warn, prune, print Ā§3.5 Ā§3.4 Ā§3.3* *decision +8 portability + hygiene Ā§1.7 Ā§1.8 Ā§1.9 Ā§2.4 Ā§1.8 needs sizing +9 documentation Ā§5.2 Ā§5.3 Ā§5.4 Ā§5.5 README is the shipped one + ā”€ā”€ cut 0.7.0 ā”€ā”€ +10 rotation Ā§3.6* *decision: build or delete +``` + +Phases 1ā€“6 are repairs and want to land in order. 7 and 8 are independent of each +other and of 5ā€“6. 9 depends on everything before it. 10 is optional and gates +one line of Phase 9. + +## Open decisions + +Neither blocks Phase 1. Both change scope where they land: + +1. **Ā§3.3, at Phase 7** ā€” delete the inert `resolution` machinery (recommended, + āˆ’45 lines) or keep it as future shape with a comment saying so. +2. **Ā§3.6, at Phase 10** ā€” build rotation (recommended) or delete the README + claim in Phase 9. diff --git a/packages/keyman/src/keyman.args.ts b/packages/keyman/src/keyman.args.ts new file mode 100644 index 0000000..5595a81 --- /dev/null +++ b/packages/keyman/src/keyman.args.ts @@ -0,0 +1,203 @@ +/** + * Argv parsing for the keyman CLI. + * + * Separate from `keyman.cli.ts` because that file is excluded from coverage: it + * is meant to be wiring, and *which flag takes a value* and *which channel names + * are legal* are behaviour. The old inline `indexOf` reader accepted + * `--channel --force`, which reached the registry as a dist-tag that cannot + * exist and reported an unreachable registry instead of a bad flag. + */ + +import type { Channel } from './keyman.update.js'; + +/** The channels `--channel` accepts, in the order the error message lists them */ +export const CHANNELS: readonly Channel[] = ['latest', 'next', 'main']; + +/** Flags that consume the next token, or the suffix of a `--flag=value` */ +const VALUE_FLAGS: readonly string[] = ['--channel', '--registry']; + +/** Flags that stand alone, short aliases included */ +const BOOLEAN_FLAGS: readonly string[] = [ + '--help', + '-h', + '--version', + '-V', + '--print-config', + '--self-update', + '--dry-run', + '-n', + '--force', + '-f', +]; + +/** + * Flags that only mean anything to `self-update`. Named so that using one on its + * own is an error rather than a silent no-op. + */ +const SELF_UPDATE_ONLY: readonly string[] = [ + '--channel', + '--registry', + '--dry-run', + '-n', + '--force', + '-f', +]; + +/** Every flag the parser accepts ā€” the list `helpText()` is checked against */ +export const KNOWN_FLAGS: readonly string[] = [...BOOLEAN_FLAGS, ...VALUE_FLAGS]; + +const SUBCOMMANDS: readonly string[] = ['self-update', 'upgrade']; + +export type ParsedArgs = + | { command: 'help' } + | { command: 'version' } + | { command: 'print-config' } + | { command: 'interactive' } + | { + command: 'self-update'; + dryRun: boolean; + force: boolean; + channel?: Channel; + registry?: string; + }; + +/** + * A mistake in the invocation. Carries a message meant for the user, so the CLI + * can print one line instead of a stack trace. + */ +export class UsageError extends Error { + constructor(message: string) { + super(message); + this.name = 'UsageError'; + } +} + +/** + * Turns argv (already sliced past `node` and the script) into one command. + * + * @throws {UsageError} on an unknown flag or command, a value flag with no + * value, a boolean flag given one, or a channel that is not a real channel + */ +export function parseArgs(argv: string[]): ParsedArgs { + // Before tokenising, so that help answers a line it could not otherwise parse. + // Exact tokens only: `--registry=--help` is a (bad) registry, not a request. + if (argv.some((token) => token === '--help' || token === '-h')) { + return { command: 'help' }; + } + + const flags = new Set(); + const values = new Map(); + let subcommand: string | undefined; + + for (let index = 0; index < argv.length; index++) { + const token = argv[index]; + + if (!token.startsWith('-')) { + if (!SUBCOMMANDS.includes(token)) { + throw new UsageError(`Unknown command: ${token}`); + } + if (subcommand) { + throw new UsageError(`Unexpected argument: ${token}`); + } + subcommand = token; + continue; + } + + const equals = token.indexOf('='); + const name = equals === -1 ? token : token.slice(0, equals); + + if (VALUE_FLAGS.includes(name)) { + // A value that looks like a flag is a forgotten value, not a value ā€” + // unless it was written as --flag=-value and therefore meant. + const inline = equals === -1 ? undefined : token.slice(equals + 1); + const value = inline ?? argv[++index]; + if (!value || (inline === undefined && value.startsWith('-'))) { + throw new UsageError(`${name} expects a value`); + } + values.set(name, value); + continue; + } + + if (!BOOLEAN_FLAGS.includes(name)) { + throw new UsageError(`Unknown flag: ${name}`); + } + if (equals !== -1) { + throw new UsageError(`${name} does not take a value`); + } + flags.add(name); + } + + const given = (...names: string[]) => names.some((name) => flags.has(name)); + + const isSelfUpdate = subcommand !== undefined || flags.has('--self-update'); + + if (!isSelfUpdate) { + const stray = [...values.keys(), ...flags].find((name) => SELF_UPDATE_ONLY.includes(name)); + if (stray) { + throw new UsageError(`${stray} is only valid with \`keyman self-update\``); + } + } + + if (flags.has('--print-config')) { + return { command: 'print-config' }; + } + + if (given('--version', '-V')) { + return { command: 'version' }; + } + + if (isSelfUpdate) { + const channel = values.get('--channel'); + if (channel !== undefined && !CHANNELS.includes(channel as Channel)) { + throw new UsageError(`Unknown channel: ${channel} (expected ${CHANNELS.join(', ')})`); + } + return { + command: 'self-update', + dryRun: given('--dry-run', '-n'), + force: given('--force', '-f'), + channel: channel as Channel | undefined, + registry: values.get('--registry'), + }; + } + + return { command: 'interactive' }; +} + +/** + * What `--help` prints. + * + * Hand-written rather than generated from the flag tables, so that adding a flag + * to the parser without documenting it fails a test instead of shipping. + */ +export function helpText(): string { + return `keyman ā€” SSH key management and an age-encrypted key vault + +Usage + keyman start the interactive menu + keyman self-update update keyman itself (alias: upgrade) + +Flags + -h, --help print this help and exit + -V, --version print the version and exit + --print-config print the resolved vault paths as JSON and exit + --self-update same as the self-update subcommand + +Flags for self-update + --channel <${CHANNELS.join('|')}> channel to update from + (default: derived from the running version) + --registry registry to query instead of the configured one + -n, --dry-run print the install command without running it + -f, --force reinstall even when already up to date + +Environment + VAULT_ROOT overrides vaultRoot from .keymanrc.json + KEYMAN_REGISTRY registry for the update check and self-update + KEYMAN_REGISTRY_TOKEN bearer token for a private registry + KEYMAN_NO_UPDATE_CHECK set to 1 to skip the once-a-day update check + (also skipped whenever CI is set) + KEYMAN_PACKAGE_MANAGER npm | pnpm | yarn | bun for the install command + +Configuration is read from .keymanrc.json, merged from the current directory +upwards and then from ~/.keymanrc.json. +`; +} diff --git a/packages/keyman/src/keyman.cli.ts b/packages/keyman/src/keyman.cli.ts index 84113a3..f86a975 100644 --- a/packages/keyman/src/keyman.cli.ts +++ b/packages/keyman/src/keyman.cli.ts @@ -1,9 +1,9 @@ #!/usr/bin/env node import { createRequire } from 'node:module'; +import { helpText, type ParsedArgs, parseArgs, UsageError } from './keyman.args.js'; import { loadConfig, resolveConfigPaths } from './keyman.config.js'; import { keyman } from './keyman.main.js'; -import type { Channel } from './keyman.update.js'; import { formatCommand, selfUpdate, updateNotice } from './keyman.update.js'; const { version, buildInfo } = createRequire(import.meta.url)('../package.json') as { @@ -18,35 +18,42 @@ const { version, buildInfo } = createRequire(import.meta.url)('../package.json') */ const versionLabel = buildInfo?.commit ? `${version} (${buildInfo.commit})` : version; -const args = process.argv.slice(2); - -/** Reads `--flag value` out of argv, or undefined when the flag is absent */ -function flagValue(name: string): string | undefined { - const index = args.indexOf(name); - return index === -1 ? undefined : args[index + 1]; +let parsed: ParsedArgs; +try { + parsed = parseArgs(process.argv.slice(2)); +} catch (error) { + if (!(error instanceof UsageError)) throw error; + console.error(`āŒ ${error.message}`); + console.error('Run `keyman --help` for usage.'); + process.exit(2); } -if (args.includes('--print-config')) { +if (parsed.command === 'help') { + console.log(helpText()); + process.exit(0); +} + +if (parsed.command === 'print-config') { const config = loadConfig(); const paths = resolveConfigPaths(config); console.log(JSON.stringify(paths)); process.exit(0); } -if (args.includes('--version') || args.includes('-V')) { +if (parsed.command === 'version') { console.log(versionLabel); process.exit(0); } -if (args[0] === 'self-update' || args[0] === 'upgrade' || args.includes('--self-update')) { - const dryRun = args.includes('--dry-run') || args.includes('-n'); +if (parsed.command === 'self-update') { + const { dryRun } = parsed; try { const result = await selfUpdate({ currentVersion: version, - channel: flagValue('--channel') as Channel | undefined, - registry: flagValue('--registry'), + channel: parsed.channel, + registry: parsed.registry, dryRun, - force: args.includes('--force') || args.includes('-f'), + force: parsed.force, }); const { status } = result; @@ -79,4 +86,15 @@ if (notice) { console.error(`\n${notice}\n`); } -keyman(); +try { + await keyman(); +} catch (error) { + // Ctrl-C at any inquirer prompt lands here. `name`, not `instanceof`: + // @inquirer/core is transitive and does not resolve from this package. + if ((error as { name?: string }).name === 'ExitPromptError') { + console.log('\nšŸ‘‹ Goodbye!\n'); + process.exit(0); + } + console.error(`āŒ ${error instanceof Error ? error.message : error}`); + process.exit(1); +} diff --git a/packages/keyman/tests/args.test.ts b/packages/keyman/tests/args.test.ts new file mode 100644 index 0000000..dfbb50c --- /dev/null +++ b/packages/keyman/tests/args.test.ts @@ -0,0 +1,155 @@ +/** + * Tests for keyman's argv parsing. + * + * The old inline reader in keyman.cli.ts turned three different mistakes into + * silence or into a wrong diagnosis, so the interesting cases here are the + * rejections rather than the happy paths. + */ + +import { describe, expect, it } from 'vitest'; +import { CHANNELS, helpText, KNOWN_FLAGS, parseArgs, UsageError } from '../src/keyman.args.js'; + +describe('parseArgs', () => { + it('defaults to the interactive session', () => { + expect(parseArgs([])).toEqual({ command: 'interactive' }); + }); + + it.each([ + [['--help'], 'help'], + [['-h'], 'help'], + [['--version'], 'version'], + [['-V'], 'version'], + [['--print-config'], 'print-config'], + ] as const)('%s selects %s', (argv, command) => { + expect(parseArgs([...argv])).toEqual({ command }); + }); + + it('answers --help even when the rest of the line is wrong', () => { + expect(parseArgs(['--bogus', '--help'])).toEqual({ command: 'help' }); + expect(parseArgs(['--help', '--channel'])).toEqual({ command: 'help' }); + }); + + describe('self-update', () => { + it.each(['self-update', 'upgrade'])('is selected by the %s subcommand', (subcommand) => { + expect(parseArgs([subcommand])).toEqual({ + command: 'self-update', + dryRun: false, + force: false, + channel: undefined, + registry: undefined, + }); + }); + + it('is selected by --self-update', () => { + expect(parseArgs(['--self-update'])).toMatchObject({ command: 'self-update' }); + }); + + it('collects its flags, long and short', () => { + expect(parseArgs(['self-update', '--dry-run', '--force'])).toMatchObject({ + dryRun: true, + force: true, + }); + expect(parseArgs(['self-update', '-n', '-f'])).toMatchObject({ + dryRun: true, + force: true, + }); + }); + + it.each(['--channel main', '--channel=main'])('accepts %s', (form) => { + expect(parseArgs(['self-update', ...form.split(' ')])).toMatchObject({ channel: 'main' }); + }); + + it('accepts every real channel', () => { + for (const channel of CHANNELS) { + expect(parseArgs(['self-update', '--channel', channel])).toMatchObject({ channel }); + } + }); + + it('reads a registry in either form', () => { + expect(parseArgs(['self-update', '--registry', 'https://r.example'])).toMatchObject({ + registry: 'https://r.example', + }); + expect(parseArgs(['self-update', '--registry=https://r.example'])).toMatchObject({ + registry: 'https://r.example', + }); + }); + + it('keeps a value that starts with a dash when it was written inline', () => { + expect(parseArgs(['self-update', '--registry=-weird'])).toMatchObject({ + registry: '-weird', + }); + }); + }); + + describe('rejections', () => { + const reject = (argv: string[]) => () => parseArgs(argv); + + it('rejects a channel that is not a channel', () => { + expect(reject(['self-update', '--channel', 'stable'])).toThrow(UsageError); + expect(reject(['self-update', '--channel', 'stable'])).toThrow( + 'Unknown channel: stable (expected latest, next, main)' + ); + }); + + it('rejects the next flag being eaten as a value', () => { + // The bug this whole module exists for: --channel --force used to set the + // channel to "--force" and report an unreachable registry. + expect(reject(['self-update', '--channel', '--force'])).toThrow('--channel expects a value'); + }); + + it('rejects a value flag with nothing after it', () => { + expect(reject(['self-update', '--channel'])).toThrow('--channel expects a value'); + expect(reject(['self-update', '--registry='])).toThrow('--registry expects a value'); + }); + + it('rejects a boolean flag given a value', () => { + expect(reject(['--dry-run=yes'])).toThrow('--dry-run does not take a value'); + }); + + it('rejects unknown flags and commands', () => { + expect(reject(['--vault', 'foo'])).toThrow('Unknown flag: --vault'); + expect(reject(['-x'])).toThrow('Unknown flag: -x'); + expect(reject(['encrypt'])).toThrow('Unknown command: encrypt'); + expect(reject(['self-update', 'upgrade'])).toThrow('Unexpected argument: upgrade'); + }); + + it.each(['--channel', '--registry', '--dry-run', '-n', '--force', '-f'])( + 'rejects %s without self-update rather than ignoring it', + (flag) => { + const argv = flag === '--channel' || flag === '--registry' ? [flag, 'main'] : [flag]; + expect(reject(argv)).toThrow('is only valid with `keyman self-update`'); + } + ); + + it('rejects a self-update flag alongside another command', () => { + expect(reject(['--print-config', '--force'])).toThrow('--force is only valid'); + }); + }); +}); + +describe('helpText', () => { + it('documents every flag the parser accepts', () => { + const text = helpText(); + for (const flag of KNOWN_FLAGS) { + expect(text, `${flag} is missing from --help`).toContain(flag); + } + }); + + it('names both subcommands, every channel, and the environment variables', () => { + const text = helpText(); + expect(text).toContain('self-update'); + expect(text).toContain('upgrade'); + for (const channel of CHANNELS) { + expect(text).toContain(channel); + } + for (const variable of [ + 'VAULT_ROOT', + 'KEYMAN_REGISTRY', + 'KEYMAN_REGISTRY_TOKEN', + 'KEYMAN_NO_UPDATE_CHECK', + 'KEYMAN_PACKAGE_MANAGER', + ]) { + expect(text).toContain(variable); + } + }); +});