diff --git a/CLAUDE.md b/CLAUDE.md index bd7bbb0..fd8dc61 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -214,13 +214,36 @@ out and watching them stay green. ## keyman architecture -Much smaller: `keyman.cli.ts` (argv, plus a `--print-config` escape hatch) → -`keyman.main.ts`, an inquirer menu loop dispatching to one module per operation -(`list`/`copy`/`generate`/`encrypt`/`decrypt`). `keyman.config.ts` mirrors nopy's -upward-traversal + `resolution` merge for `.keymanrc.json`, but validates the -result with Zod and falls back to defaults instead of throwing. `VAULT_ROOT` in -the environment beats the config file. Encryption shells out to `age` / -`age-keygen` / `ssh-keygen`, which must be on `PATH`. +Much smaller: `keyman.cli.ts` (wiring only — argv parsing lives in +`keyman.args.ts`, which is covered, and the CLI is the error boundary that turns a +`UsageError` into one line instead of a stack trace) → `keyman.main.ts`, an +inquirer menu loop dispatching to one module per operation +(`list`/`copy`/`generate`/`encrypt`/`decrypt`/`rotate`/`retire`/`clear`). +`keyman.config.ts` mirrors nopy's upward traversal for `.keymanrc.json` but not +its `resolution` merge: every keyman property is a string, so a child simply wins +and the strategies could not change an outcome — see `docs/AUDIT.md` §3.3. It +validates with Zod, falls back to defaults instead of throwing, and warns about a +key it does not know rather than letting Zod strip it silently. `VAULT_ROOT` in +the environment beats the config file. It shells out to `age`, `age-keygen` +(`-y`, to derive the recipient from the identity rather than trusting the +`# public key:` comment) and `ssh-keygen` (`-y`, to recover a missing `.pub`), +which must be on `PATH`; `runTool` tells a missing binary apart from a refusing +one. Nothing shells out to `cp` or `chmod` any more — `decrypt` copies and +chmods in-process, because the old spawn left a private key at age's 0644 for the +length of two processes. + +The write path is one function, `storeInVault` (`keyman.vault.ts`), shared by +`encrypt`, `generate` and `rotate`; `listVaultKeys` is the one reader of the +`//id_.age` layout. Rotation is deliberately two operations +(`rotate` adds a replacement under the next name in the series, `retire` deletes +the superseded key), because a rotation that replaces the key in place locks you +out of the host it was for. keyman never handles a passphrase: `ssh-keygen` +prompts for it with stdio inherited, since `-N ` put it in argv where `ps` +could read it. + +`docs/AUDIT.md` is a full audit of the package with each finding marked closed as +it landed, and `docs/PLAN.md` the ten phases that closed them. Both are records +now, not plans. ### Updating @@ -365,4 +388,6 @@ unreachable registry). `CubePackageRef` is referenced by the exported `NopyConfig` but is not itself re-exported, so a consumer cannot name the type — one line, not yet fixed. `DOCS-AUDIT.md` tracks the drift in the remaining documents; §2.9 (the nopy README shipping yarn-workspace instructions to npmjs) -is closed, so the keyman README (§2.10) is now the worst of them. +and §2.10 (the keyman README describing four of nine operations and inventing a +tenth) are both closed. The keyman README now quotes `helpText()` verbatim and a +test fails if the two diverge, which is the shape worth copying for nopy. diff --git a/packages/keyman/README.md b/packages/keyman/README.md index a80fb57..488debd 100644 --- a/packages/keyman/README.md +++ b/packages/keyman/README.md @@ -1,104 +1,80 @@ -# Keyman - SSH Key Management with Age Encryption +# keyman — SSH key management with an age-encrypted vault -Keyman is a simple command line tool built around the `age` encryption tool. It allows you to manage SSH keys in public GitHub repositories securely by encrypting the private keys. +keyman keeps SSH private keys in a vault you can commit. Each key is encrypted +with [age](https://github.com/FiloSottile/age) to a single recipient — the vault's +identity file — which is the one thing that has to stay out of the repository. -## Features +It is an interactive menu rather than a set of subcommands: point it at a vault, +pick an operation, repeat until you quit. -- 🔐 Encrypt SSH private keys with age encryption -- 📁 Organized vault structure: `vault/keys/` for encrypted keys, `vault/tmp/` for decrypted keys -- ⚙️ Configurable via `.keymanrc.json` with sensible defaults -- 🔍 Interactive CLI for encrypting, decrypting, and listing keys -- 🔄 Support for key rotation +## Requirements -## Quick Start +`age`, `age-keygen` and `ssh-keygen` on `PATH`. keyman shells out to all three and +names the missing one instead of failing obscurely. -### 1. Generate Age Encryption Key +## Installing -```bash -# Create vault structure -mkdir -p vault/keys vault/tmp +```sh +npm install -g @bitsquare/keyman@main \ + --@bitsquare:registry=https://gitea.bitsquare.dev/api/packages/BitSquare/npm/ +``` -# Generate age encryption key (keep this secret!) +Point the **scope** at that registry rather than setting a bare `--registry`: it +serves `@bitsquare` packages only and does not proxy npmjs, so every other +dependency has to keep resolving from npmjs. Reading needs no token while the +repository is public, and the same line works with `pnpm`. + +`@main` is a snapshot of the default branch, published on every push. Name a tag — +there is no `latest` on that registry yet, so an untagged install resolves to +nothing, and keyman has not been released to npmjs. `keyman self-update` keeps you +on whichever channel you installed from. + +## Quick start + +```sh +# The vault identity. The only secret in the vault, and the only thing here you +# cannot regenerate — back it up somewhere that is not this repository. +mkdir -p vault age-keygen -o vault/age.key -# Add to .gitignore -echo "vault/age.key" >> .gitignore -echo "vault/tmp/" >> .gitignore -``` - -### 2. Generate SSH Keys - -```bash -# Generate SSH key pair -ssh-keygen -t ed25519 -f vault/tmp/id_deploy -N "" -C "deploy@myapp.dev" -``` - -### 3. Run Keyman - -```bash -# Run keyman interactively +# Run keyman against it. VAULT_ROOT=./vault keyman - -# Or if you have .keymanrc.json configured, just run: -keyman ``` -## Configuration +On startup keyman creates `keys/` and `tmp/` under the vault at `0700` and writes +a `.gitignore` beside them covering the identity and `tmp/`, so a fresh vault +cannot be committed by accident. -Keyman uses sensible defaults but can be customized via `.keymanrc.json`: - -```json -{ - "vaultRoot": "./vault", - "keysDir": "keys", - "tmpDir": "tmp", - "ageKeyFile": "age.key" -} -``` - -### Configuration Priority - -1. **VAULT_ROOT** environment variable (highest priority) -2. **.keymanrc.json** file (searched from current directory upward) -3. **Default values** (lowest priority) - -### Default Values - -- `vaultRoot`: `"vault"` -- `keysDir`: `"keys"` -- `tmpDir`: `"tmp"` -- `ageKeyFile`: `"age.key"` - -## Vault Structure - -``` -project/ -├── vault/ -│ ├── age.key # Master encryption key (NEVER commit!) -│ ├── keys/ # Encrypted keys (safe to commit) -│ │ └── deploy/ # Each key has its own folder -│ │ ├── id_deploy.pub # Public key -│ │ └── id_deploy.age # Encrypted private key -│ └── tmp/ # Decrypted keys (NEVER commit!) -│ ├── id_deploy # Decrypted private key -│ └── id_deploy.pub # Public key -└── .keymanrc.json # Configuration (optional) -``` +Then pick **🆕 Generate key**: it makes the key pair and encrypts it into the vault +in one step. `ssh-keygen` collects the passphrase itself — keyman never sees it, so +it can never put it on a command line. ## Operations -Keyman provides an interactive menu-driven interface with the following operations: +Every operation returns to the menu, so a session can run several. -- **📋 List keys** - Compact view showing all keys with checkbox indicators for their locations -- **🔒 Encrypt keys** - Encrypt SSH keys from `vault/tmp/` and store in `vault/keys/` -- **🔓 Decrypt keys** - Decrypt keys from `vault/keys/` to `vault/tmp/` or `~/.ssh/` -- **❌ Quit** - Exit the program +- **📋 List keys** — every key it can see and where it is: encrypted in the vault, + decrypted in `tmp/`, live in `~/.ssh`, or some combination. +- **📝 Copy public key** — the public half of a key in `~/.ssh` or `tmp/`, to the + clipboard via whichever of `pbcopy`, `clip`, `wl-copy`, `xclip` or `xsel` exists. + With none of them, it prints the key instead. +- **🆕 Generate key** — an `ed25519` or 4096-bit `rsa` pair into `tmp/`, encrypted + into the vault straight away. +- **🔒 Encrypt keys** — pick from the private keys in `~/.ssh` *and* `tmp/`; each + goes to `//` with its public half beside it. A key that has no + `.pub` file gets one derived with `ssh-keygen -y`. One key failing costs only + that key. +- **🔓 Decrypt keys** — pick from the vault and decrypt to `tmp/` or `~/.ssh`. + Never overwrites a file without asking first, and the plaintext key is `0600` + from the moment it exists. +- **🔄 Rotate key** — a replacement for a vault key, encrypted *alongside* the + original. See below. +- **🗑️ Retire key** — the other half of a rotation: delete a vault key and its + plaintext copies, after listing every path that goes. +- **🧹 Clear decrypted keys** — remove the plaintext keys from `tmp/`. +- **❌ Quit** -After completing any operation, keyman automatically returns to the main menu, allowing you to perform multiple operations in a single session without restarting the tool. - -### List Keys Output - -The list command shows a compact, unified view of all SSH keys with their locations: +### Listing ``` 🔑 SSH Keys: @@ -117,36 +93,163 @@ The list command shows a compact, unified view of all SSH keys with their locati ⚠️ = Unmanaged (in .ssh or tmp, not encrypted in vault) ``` -**Features:** -- Public keys are indicated with `(.pub)` suffix instead of separate entries -- Status emoji shows management state at a glance -- Checkboxes `[✓]` show presence in three locations: - - **[Vault]** - Encrypted in vault/keys/ - - **[Tmp]** - Decrypted in vault/tmp/ - - **[.ssh]** - Active in ~/.ssh/ -- Alphabetically sorted for easy scanning -- New **🔓** status for keys decrypted to tmp but not yet in .ssh +`(.pub)` means a public key was found next to the private one, in either location. +The rows are sorted by name. -## Example Usage +### Rotating a key -```bash -# Using environment variable -VAULT_ROOT=../../vault keyman +Rotation is deliberately two operations, because both keys have to exist at once: -# Using default configuration -keyman +1. **🔄 Rotate key**, and pick `prod`. keyman generates `id_prod-2` in `tmp/`, + encrypts it to `keys/prod-2/`, and prints both public keys. `prod` is untouched. +2. Add the `prod-2` public key wherever `prod` is authorized. +3. Check that you can log in with `tmp/id_prod-2`. +4. Remove the `prod` public key from those hosts. +5. **🗑️ Retire key**, and pick `prod`. -# Keyman will show: -# 📁 Vault Root: /path/to/vault -# 🔑 Keys Directory: /path/to/vault/keys -# 📂 Temp Directory: /path/to/vault/tmp -# 🔐 Age Key: /path/to/vault/age.key +The name has to change: the vault directory is derived from it, so a replacement +also called `prod` *is* the `prod` entry. Rotating again continues the series +(`prod-2` → `prod-3`), and a version already taken — in the vault, in `tmp/` or in +`~/.ssh` — is skipped rather than overwritten. + +Doing it in one step instead is what this shape avoids: replace the key in the +vault and you have locked yourself out of the host you were rotating for, because +the replacement is not on it yet and the only copy of the key that is has gone. +Retiring warns when nothing in the vault supersedes the key, and then asks you to +type its name. + +### The `id_` prefix + +keyman manages keys named `id_*`; the vault directory for `id_prod` is `prod`. +A private key named anything else is not offered by any operation — but List, Copy +and Encrypt report the ones they found, with a count and the reason, so it is +never silently invisible. Rename it to `id_` to bring it in. + +## Command line + +``` +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 paths and the config files + they came from, as JSON, and exit + --self-update same as the self-update subcommand + +Flags for self-update + --channel 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. ``` -## Best Practices +The update channel is derived from the version you are running — a `-main.` build +checks `main`, any other prerelease checks `next`, a clean version checks `latest` +— so an update cannot quietly move you to a different channel. The check runs at +most once a day and prints its hint to **stderr**, which keeps `--print-config` +machine-readable. -1. **Never commit** `vault/age.key` or `vault/tmp/` to version control -2. **Always backup** your `age.key` securely (password manager, encrypted USB) -3. **Commit** `vault/keys/` - encrypted keys are safe to share -4. **Use environment variables** for CI/CD: `VAULT_ROOT=/path/to/vault keyman` -5. **Keep .keymanrc.json** in your project root for team consistency +## Configuration + +`.keymanrc.json`, with every key optional: + +```json +{ + "vaultRoot": "vault", + "keysDir": "keys", + "tmpDir": "tmp", + "ageKeyFile": "age.key" +} +``` + +| key | default | meaning | +| ------------ | ---------- | ---------------------------------------------------- | +| `vaultRoot` | `vault` | the vault directory; everything else lives inside it | +| `keysDir` | `keys` | the encrypted keys — the part that is safe to commit | +| `tmpDir` | `tmp` | decrypted keys, in plaintext | +| `ageKeyFile` | `age.key` | the age identity the vault encrypts to | + +The last three are resolved against `vaultRoot` unless they are absolute. A +relative `vaultRoot` **in a config file** is resolved against that file's +directory, so a repository config keeps meaning the same vault from any +subdirectory; the built-in default is resolved against the current directory. + +Files are read from `~/.keymanrc.json` first, then from the filesystem root down +to the current directory, so the nearest file wins key by key. `VAULT_ROOT` in the +environment beats all of them. A file that is not valid JSON is skipped with a +warning rather than taken as fatal, and a key keyman does not know is reported +instead of silently dropped — `{"vaultroot": "…"}` used to be indistinguishable +from an empty file. + +`keyman --print-config` answers what all of that resolved to, and which files it +came from: + +```sh +$ keyman --print-config +{"vaultRoot":"/srv/infra/vault","keysDir":"/srv/infra/vault/keys","tmpDir":"/srv/infra/vault/tmp","keyPath":"/srv/infra/vault/age.key","configFiles":["/srv/infra/.keymanrc.json"]} +``` + +## Vault layout + +``` +project/ +├── vault/ +│ ├── .gitignore # written by keyman: the identity and tmp/, not keys/ +│ ├── age.key # the vault identity (NEVER commit) +│ ├── keys/ # encrypted keys (safe to commit) +│ │ └── deploy/ # one directory per key, named without the id_ prefix +│ │ ├── id_deploy.age # the private key, encrypted to the vault recipient +│ │ └── id_deploy.pub # the public key +│ └── tmp/ # decrypted keys (NEVER commit) +│ ├── id_deploy +│ └── id_deploy.pub +└── .keymanrc.json # optional +``` + +With a custom `keysDir` or `tmpDir`, those two names change and nothing else does. + +## Practices this tool assumes + +1. **Back up `age.key`** somewhere outside the repository. It is the only thing + that can decrypt the vault, and nothing in the vault can reconstruct it. +2. **Commit `keys/`.** Encrypted keys are the point; a vault nobody shares is a + directory. +3. **Do not commit the identity or `tmp/`.** keyman writes a `.gitignore` for + this, and never overwrites one you wrote yourself — check it if you brought + your own. +4. **Clear `tmp/` when you are done with it** (🧹), so plaintext keys do not + outlive the reason they were decrypted. +5. **Keep `.keymanrc.json` in the project root** so everyone resolves the same + vault, and use `VAULT_ROOT` for the exceptions. + +## Upgrading from a version before 0.7.0 + +`keysDir` and `tmpDir` used to be honoured by some operations and ignored by +others, which left anyone with custom names holding a **split vault**: `generate` +and `list` used the configured directories while `encrypt` and `decrypt` used +`/keys` and `/tmp`. All of them agree now, so anything +written by the old `encrypt` needs moving once: + +```sh +mv /keys/* // +``` + +Nobody on the default names is affected — for them the two halves were the same +directory all along. diff --git a/packages/keyman/docs/AUDIT.md b/packages/keyman/docs/AUDIT.md index e284600..4a96d3a 100644 --- a/packages/keyman/docs/AUDIT.md +++ b/packages/keyman/docs/AUDIT.md @@ -16,6 +16,17 @@ 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. +## Status + +**All 30 findings are closed except the second half of §1.8**, over the ten phases +of `PLAN.md`. Each one keeps its original text as the record, with what closed it +quoted underneath; the line numbers still point at `75983ab`, so they are history +rather than directions. The one deliberate omission is making keys not named `id_*` +*manageable* — they are now reported rather than silently skipped, and the rest is a +change to the on-disk layout that wanted sizing first. + +Where the audit ends: 336 tests, 99.3 % statements / 95.7 % branches. + --- ## Contents @@ -32,7 +43,13 @@ context for §1.2, not a defence of it. ## 1. Defects -### 1.1 🔴 `keysDir` and `tmpDir` are honoured by half the tool +### 1.1 ✅ `keysDir` and `tmpDir` are honoured by half the tool — **fixed** + +> **Closed in Phase 5.** `main.ts` passes `paths.keysDir` and `paths.tmpDir` to +> `encrypt` and `decrypt`, neither of which joins `vaultRoot` itself any more, so all +> five operations agree on the configured directories. `tests/vault-layout.test.ts` is +> the regression test: non-default names in a real config file, the real loader, one +> encrypt, and a listing that has to show the key in the vault column. `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 @@ -81,7 +98,11 @@ 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 +### 1.2 ✅ Encrypt and decrypt crash with a raw stack trace on a first run — **fixed** + +> **Closed in Phases 1 and 2.** `keyman.cli.ts` is an error boundary — a +> `UsageError` prints one line, anything else prints its message and exits 1, and +> neither prints a stack. The two readdirs that threw are guarded (§1.5). Three `readdirSync` calls have no `existsSync` guard: @@ -131,7 +152,13 @@ rejection is one line rather than an unhandled-rejection stack trace. The missin `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` +### 1.3 ✅ A missing `age.key` becomes `age -r null` — **fixed** + +> **Closed in Phase 3.** The recipient is resolved once per session, before any +> operation that needs one. A null aborts *that operation* with +> `age-keygen -o ` as the remedy and returns to the menu, and is retried on the +> next attempt, so creating the identity mid-session works. `age -r null` is now +> unreachable. `keyman.main.ts:73` and `:80` assert away a null: @@ -169,7 +196,11 @@ 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 +### 1.4 ✅ Decrypting into `~/.ssh` silently overwrites an existing key — **fixed** + +> **Closed in Phase 4.** Every collision is settled before anything is written: a +> confirmation per key defaulting to no, and a skip that says what it kept. The user +> is answering about files that still exist. `decrypt.ts:47-49` writes the decrypted key and copies the public key with no existence check, no confirmation, and no backup. @@ -195,7 +226,12 @@ since `vault/tmp` is scratch space by design. 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 +### 1.5 ✅ `age` or `ssh-keygen` missing is unhandled in encrypt and decrypt — **fixed** + +> **Closed in Phase 2.** `runTool` turns `ENOENT` into a `ToolNotFoundError` +> whose message is an instruction, and keeps it distinct from a tool that ran and +> refused — whose reason is on stderr and nowhere in execa's message. `encrypt` +> re-throws it instead of counting it against one key. 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; @@ -203,7 +239,13 @@ Same missing `try/catch` as §1.3. `generateKey` (`generate.ts:51-76`) and 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 +### 1.6 ✅ Encrypt copies `.pub` unconditionally and aborts the batch midway — **fixed** + +> **Closed in Phase 6.** `storeInVault` reads the `.pub` *before* the vault +> directory exists and derives a missing one with `ssh-keygen -y` — stdout piped, +> stdin and stderr inherited, because the passphrase prompt goes to stderr — storing +> the private key alone if it cannot. And a failing key costs one key: `encrypt` +> collects the failures and names them at the end. `encrypt.ts:45`: @@ -228,7 +270,12 @@ so, and the process dies via §1.2. absent, and wrap the loop body so one bad key costs one key rather than the batch. -### 1.7 🟠 `/home/` is hardcoded +### 1.7 ✅ `/home/` is hardcoded — **fixed** + +> **Closed in Phase 8.** `keyman.home.ts` resolves a named user against the +> sibling of the current home first, then `/home/` and `/Users/`, and +> reports every path it tried. An unset `HOME` falls back to the passwd entry instead +> of resolving `.ssh` against the filesystem root. `keyman.main.ts:33`: @@ -245,7 +292,17 @@ feeds a nonexistent `sshDir` into §1.2 rather than into an error message. 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 +### 1.8 🟡 Keys not named `id_*` are invisible, silently — **half fixed** + +> **Partly closed in Phase 8 — the rest is open.** `scanPrivateKeys` classifies a +> file by reading its first 64 bytes for a private-key header, and List, Copy and +> Encrypt report what they skipped, with the count, the directory and the reason. So +> the keys are no longer *silently* invisible. +> +> They are still not manageable. The vault layout derives `id_` from the +> directory name in four places, so accepting other names changes what is on disk; +> the plan asked for that to be sized before being committed to, and the report is +> the tenth of the work that closes most of the surprise. Left deliberately. 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 @@ -256,7 +313,12 @@ it simply is not in the menu. 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 +### 1.9 ✅ `pbcopy` is hardcoded — **fixed** + +> **Closed in Phase 8.** `keyman.clipboard.ts` picks by platform — `pbcopy`, +> `clip`, or `wl-copy` → `xclip` → `xsel` — falls through only on `ENOENT` (a tool +> that ran and refused is a real error, not an absent tool), and prints the key when +> nothing is installed, since printing it was always the point. `copy.ts:49`, with the comment above it admitting the shortcut: @@ -271,7 +333,13 @@ 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 +### 1.10 ✅ Smaller things — **fixed** + +> **Closed in Phases 2, 6 and 8.** All four: the `statSync` takes +> `throwIfNoEntry: false` and still follows a symlink to a real directory; both +> `replace` calls are anchored to `/^id_/`; a failed `age` now removes the file it +> named and the directory while it is empty, so nothing half-made is left claiming to +> hold a key; and both `console.log` debug lines are gone. - **`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 @@ -296,7 +364,12 @@ dead end. ## 2. Security -### 2.1 🔴 Decrypted private keys are world-readable before the chmod +### 2.1 ✅ Decrypted private keys are world-readable before the chmod — **fixed** + +> **Closed in Phase 4.** `fs.chmodSync(…, 0o600)` in-process, immediately after +> `age` returns — no `cp` or `chmod` spawn, so there is no window and no failure mode +> that leaves the mode behind. Confirmed again while probing Phase 10: age still +> writes 0644, and `ssh-keygen -y` refuses such a file outright. `decrypt.ts:47-50` decrypts, then copies, then chmods — in three separate processes: @@ -326,7 +399,11 @@ 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 +### 2.2 ✅ The passphrase is passed on the `ssh-keygen` command line — **fixed** + +> **Closed in Phase 6.** The prompt is gone and so is `-N`: `ssh-keygen` collects +> and confirms the passphrase itself with stdio inherited. A passphrase keyman never +> learns cannot leak from keyman — and a test asserts it never asks for one. `generate.ts:53`: @@ -344,7 +421,13 @@ 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 +### 2.3 ✅ The age recipient is trusted from a comment, never verified — **fixed** + +> **Closed in Phase 3.** `age-keygen -y` derives the recipient from the secret +> key, so it cannot disagree with it. The comment survives only as a fallback for a +> machine with no `age-keygen`, behind a warning that it is unverified — and +> deliberately *not* as a fallback for `age-keygen` refusing the file, which means +> age cannot read the identity at all. `extractAgePublicKey` (`utils.ts:16`) regexes the recipient out of a comment line in the identity file: @@ -363,7 +446,12 @@ 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` +### 2.4 ✅ Nothing manages the plaintext left in `vault/tmp` — **fixed** + +> **Closed in Phase 8.** A **🧹 Clear decrypted keys** operation that lists what it +> will delete and asks before deleting it, plus a `.gitignore` written beside the +> vault covering the identity and the tmp directory — never overwriting one that is +> already there, and never claiming to cover a path outside the vault. Decrypted keys accumulate in `vault/tmp` indefinitely. There is no shred operation, no warning on exit, and keyman never writes the `.gitignore` its own @@ -378,7 +466,11 @@ reaches a public repository — which is the threat this tool exists to address. ## 3. Unimplemented and dead -### 3.1 🟠 There is no `--help` +### 3.1 ✅ There is no `--help` — **fixed** + +> **Closed in Phase 1.** `helpText()` in `keyman.args.ts`, checked against the +> parser's own flag table by a test so a new flag cannot ship undocumented, and now +> quoted verbatim in the README by a second test (§5.3). `keyman.cli.ts` handles `--print-config`, `--version`/`-V`, and `self-update`/`upgrade`, then falls through to the interactive session. `--help` @@ -411,7 +503,12 @@ need the behaviour. `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 +### 3.2 ✅ `flagValue` accepts things that are not values — **fixed** + +> **Closed in Phase 1.** `parseArgs` rejects a value flag with no value, a boolean +> flag given one, an unknown flag, an unknown command, an unknown channel, and a +> self-update-only flag used without `self-update`. `--channel --force` is now a +> usage error rather than a request for a dist-tag that cannot exist. `keyman.cli.ts:24-27` is `args.indexOf(name)` and `args[index + 1]`: @@ -429,7 +526,11 @@ error. rejects a flag swallowed as another flag's value, and validates `--channel` against `CHANNELS`. -### 3.3 🟡 The `resolution` merge machinery has no effect +### 3.3 ✅ The `resolution` merge machinery has no effect — **fixed** + +> **Closed in Phase 7 — deleted.** Every keyman property is a string, so a child +> simply wins; `mergeConfigs` is one spread with a comment recording why nopy needs +> more and keyman does not. `keyman.config.ts` lost ~45 lines. `keyman.config.ts` carries `ResolutionStrategy`, `KeymanResolutionConfig`, `mergeValue` and `mergeConfigs` — roughly 45 lines, imported from nopy's design. @@ -456,7 +557,11 @@ 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 +### 3.4 ✅ `getConfigPaths()` is exported, tested, and called by nothing — **fixed** + +> **Closed in Phase 7.** `describeConfig()` calls it, so `--print-config` prints +> `configFiles` — the files that were merged, in merge order. That was the one +> question the flag could not answer, and it existed only as unstructured stderr. `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 @@ -469,7 +574,12 @@ 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 +### 3.5 ✅ A typo in `.keymanrc.json` is silent — **fixed** + +> **Closed in Phase 7.** `warnUnknownKeys` names the file, the keys it ignored and +> the keys it knows. Warned rather than fatal, which is this module's posture +> throughout, and warned per file because that is the only place the filename is in +> hand. `KeymanConfigSchema` is a plain `z.object`, which strips unknown keys. @@ -483,14 +593,25 @@ 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 +### 3.6 ✅ "Support for key rotation" does not exist — **fixed** + +> **Closed in Phase 10 — built.** `keyman.rotate.ts`: **🔄 Rotate key** generates a +> replacement under the next name in the series and encrypts it *alongside* the +> original, and **🗑️ Retire key** deletes the superseded key after listing every path +> that goes, asking for the name to be typed out when nothing in the vault supersedes +> it. Two operations rather than one, because a rotation that replaces the key in +> place locks you out of the host you were rotating for. `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" +### 3.7 ✅ "Copy public key and create README" — **fixed** + +> **Closed in Phase 5.** The comment went with the rewrite of `encrypt`. No +> per-key README was ever written and nothing claims one now; the `README.md` fixture +> in `decrypt.test.ts` is a stray-file case, which `listVaultKeys` ignores. `encrypt.ts:44` says it; no README is written. Suggestively, `decrypt.test.ts:75` places a `README.md` inside the keys directory as a @@ -501,14 +622,20 @@ it or drop the half of the comment that lies. ## 4. Public API and packaging -### 4.1 🟡 A shebang on the library entry point +### 4.1 ✅ A shebang on the library entry point — **fixed** + +> **Closed in Phase 9.** The shebang is gone, with a comment saying why the file +> does not want one. Verified against the built `dist/index.js`. `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 +### 4.2 ✅ The exported functions' types are not exported — **fixed** + +> **Closed in Phase 9.** `KeymanConfig` and `KeymanConfigFile` are exported; +> `ResolutionStrategy` and `KeymanResolutionConfig` no longer exist (§3.3). `src/index.ts:2` exports `loadConfig` and `resolveConfigPaths`. It does not export `KeymanConfig`, `KeymanConfigFile`, `ResolutionStrategy` or @@ -516,7 +643,13 @@ export `KeymanConfig`, `KeymanConfigFile`, `ResolutionStrategy` or 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()` +### 4.3 ✅ `export * from './keyman.main.js'` exports only `keyman()` — **fixed** + +> **Closed in Phase 9 — decided, and written down.** The surface is deliberately +> narrow: config resolution, the update machinery, and `keyman()`. The operation +> modules stay internal because every one of them prompts, prints and spawns, so +> there is nothing to do with a single one except rebuild the menu around it. The +> rule is now a comment at the top of `src/index.ts` rather than an accident. 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 @@ -524,7 +657,11 @@ 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 +### 4.4 ✅ Update-module constants are half re-exported — **fixed** + +> **Closed in Phase 9.** `export * from './keyman.update.js'`, so the rule is +> "all of it" and the list cannot drift again. Verified by importing the built +> `dist/index.js` and reading its keys. `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` @@ -535,7 +672,11 @@ re-exports neither, while re-exporting `DEFAULT_CHECK_INTERVAL_MS` and ## 5. Documentation drift -### 5.1 🟠 `DOCS-AUDIT.md` lists §1.1 under *checked and accurate* +### 5.1 ✅ `DOCS-AUDIT.md` lists §1.1 under *checked and accurate* — **fixed** + +> **Closed in Phase 5.** The claim `DOCS-AUDIT.md` makes — that the documented +> vault layout matches the code — is now *true*, which is the substance of it; §1.1 is +> what made it false. The entry has been amended to say what it actually checked. `DOCS-AUDIT.md:826-827`: @@ -547,7 +688,12 @@ The first two clauses are correct. The third holds only because 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 +### 5.2 ✅ `README.md` operations list — **fixed** + +> **Closed in Phase 9.** All nine menu entries are documented, and a test asserts +> the README contains every label `keyman.main.ts` offers, so a tenth cannot arrive +> undocumented. Encrypt is described as it behaves: the union of `~/.ssh` and the tmp +> directory. `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 — @@ -556,7 +702,12 @@ 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 +### 5.3 ✅ The README documents none of the CLI surface — **fixed** + +> **Closed in Phase 9.** The README carries `helpText()` verbatim — every flag, +> both subcommand spellings, and all five environment variables — with a test that +> fails if the two diverge. Installation, the update channels and the once-a-day +> check are documented too. `README.md` covers the interactive menu and the config file. It does not mention: @@ -571,13 +722,24 @@ only `dist`, `README.md` and `LICENSE` — so a reader on the registry sees none 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 +### 5.4 ✅ The README presents a configurable layout that is half-real — **fixed** + +> **Closed in Phase 9.** The section documents what §1.1 made true: the three +> inner names resolve against `vaultRoot`, a relative `vaultRoot` in a config file +> resolves against that file's directory, and the built-in default resolves against +> the current directory. It ends with the migration note for a vault written by the +> old `encrypt`. `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 +### 5.5 ✅ `CLAUDE.md` names a binary keyman never runs — **fixed** + +> **Closed in Phases 3 and 9.** `age-keygen` became true in Phase 3 (`-y`, to +> derive the recipient), and `cp`/`chmod` stopped being spawned in Phase 4. +> `CLAUDE.md` now says all of that, records the deliberate `resolution` divergence +> from nopy, and lists the operations the menu actually has. > Encryption shells out to `age` / `age-keygen` / `ssh-keygen`, which must be on > `PATH`. @@ -627,6 +789,10 @@ nothing. ## Suggested order of attack +> Superseded by `PLAN.md`, which turned this into ten phases and is the record of +> what was actually done in what order. Kept because the reasoning about which +> findings share a shape is still the reason the phases group the way they do. + **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 diff --git a/packages/keyman/docs/PLAN.md b/packages/keyman/docs/PLAN.md index 73f7800..870153b 100644 --- a/packages/keyman/docs/PLAN.md +++ b/packages/keyman/docs/PLAN.md @@ -9,6 +9,25 @@ 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. +## Status + +**All ten phases have landed**, one commit each, on the `keyman-remediation` +branch. Three deviations worth knowing about: + +- **Phase 6's literal instruction was impossible.** "Move the `mkdirSync` after + `age` succeeds" cannot be done — `age -o` will not create its output directory. + The goal (no leftover directory) is met by cleaning up on failure instead, which + also removes a truncated `.age` the plan had not accounted for. +- **§1.8 is half done, deliberately**, exactly as the plan asked: the skipped-key + report is in, the layout change that would make non-`id_*` keys manageable is + not. See `AUDIT.md` §1.8. +- **Rollout has not been done.** No version bump, no tag, nothing published — the + cut points below are still proposals, and pushing this branch to `main` would + publish a snapshot, so that is the user's call to make. + +Both open decisions were resolved the way the plan recommended: the `resolution` +machinery was deleted, and rotation was built. + ## Verified before planning Four things the fixes depend on, checked by running them rather than assumed — diff --git a/packages/keyman/src/index.ts b/packages/keyman/src/index.ts index 9b9bd3f..acd1907 100644 --- a/packages/keyman/src/index.ts +++ b/packages/keyman/src/index.ts @@ -1,31 +1,15 @@ -#!/usr/bin/env node -export { loadConfig, resolveConfigPaths } from './keyman.config.js'; +/** + * The library surface — `exports["."]`, imported and never executed, which is why + * it no longer carries the bin's shebang (AUDIT §4.1). The bin is + * `dist/keyman.cli.js`. + * + * Deliberately narrow: config resolution, the update machinery, and `keyman()` to + * run the menu. The operation modules stay internal — every one of them prompts, + * prints and spawns, so there is nothing to do with a single one except reproduce + * the menu around it (§4.3). The update module is re-exported wholesale rather + * than by name list, because the list had drifted to half of it (§4.4). + */ +export type { KeymanConfig, KeymanConfigFile } from './keyman.config.js'; +export { describeConfig, loadConfig, resolveConfigPaths } from './keyman.config.js'; export * from './keyman.main.js'; -export type { - Channel, - CommandRunner, - PackageManager, - SelfUpdateResult, - UpdateCache, - UpdateStatus, -} from './keyman.update.js'; -export { - buildSelfUpdateCommand, - channelForVersion, - checkForUpdate, - DEFAULT_CHECK_INTERVAL_MS, - detectPackageManager, - fetchChannelVersion, - formatCommand, - formatUpdateNotice, - getUpdateCachePath, - isUpdateCheckDisabled, - NPMJS_REGISTRY, - normalizeRegistry, - PACKAGE_NAME, - readUpdateCache, - resolveRegistry, - selfUpdate, - updateNotice, - writeUpdateCache, -} from './keyman.update.js'; +export * from './keyman.update.js'; diff --git a/packages/keyman/tests/readme.test.ts b/packages/keyman/tests/readme.test.ts new file mode 100644 index 0000000..5e524d2 --- /dev/null +++ b/packages/keyman/tests/readme.test.ts @@ -0,0 +1,47 @@ +/** + * The README is the only document that ships (`package.json` files: dist, + * README.md, LICENSE), so a reader on the registry sees it and nothing else. It + * had drifted to describing four of the menu's entries and none of the command + * line; these assertions are the parts that can drift again silently. + */ + +import fs from 'node:fs'; +import path from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { helpText } from '../src/keyman.args.js'; + +const README = fs.readFileSync(path.join(import.meta.dirname, '..', 'README.md'), 'utf-8'); + +describe('README', () => { + it('quotes --help verbatim', () => { + // Copied rather than described, and checked rather than trusted: a flag added + // to helpText() now fails here instead of shipping undocumented. + expect(README).toContain(helpText().trim()); + }); + + it('documents every menu operation', () => { + const main = fs.readFileSync( + path.join(import.meta.dirname, '..', 'src', 'keyman.main.ts'), + 'utf-8' + ); + const labels = [...main.matchAll(/\{ name: '([^']+)', value: '[a-z]+' \}/g)].map((m) => m[1]); + + // The labels themselves, emoji included, so a renamed entry is caught too — + // but with runs of whitespace collapsed, because some of them carry a second + // space to align a variation-selector emoji in a terminal, and prose should + // not have to reproduce that. + const collapse = (text: string) => text.replace(/\s+/g, ' '); + const readme = collapse(README); + + expect(labels.length).toBe(9); + for (const label of labels) { + expect(readme, label).toContain(collapse(label)); + } + }); + + it('documents every configuration key', () => { + for (const key of ['vaultRoot', 'keysDir', 'tmpDir', 'ageKeyFile']) { + expect(README, key).toContain(key); + } + }); +});