HarperFast / HarperFast/harper-pro
ssh/config is written without a lock by parallel threads: a corrupt config disables every deploy key on the node
- Dominant language
- JavaScript
- Stars
- 3
- Forks
- 0
- Avg merge
- 1d 21h
- Merged PRs (30d)
- 80
Description
The four mutating SSH-key operations write two files shared across all keys — `/ssh/config` and `/ssh/known_hosts` — with no lock and, on several paths, a full-file truncating `writeFile`. Concurrent operations corrupt the config, and a corrupt config takes out **every** deploy key on the node rather than just the one being written.
## These are genuinely parallel writers, across threads
Worth establishing first, because it moves this from "operator did two things at once" to normal cluster operation:
- The operations API binds on the **main thread** only (`core/components/componentLoader.ts`, `isMainThread` gate).
- The replication WS server starts only under `isWorker` (`core/components/componentLoader.ts`), and an inbound `OPERATION_REQUEST` calls `server.operation(data, …)` on that **worker** thread (`replication/replicationConnection.ts:2947`).
- `registerOperation` has no thread gating for these ops, so they exist in every thread's map.
- There is no mutex anywhere in `security/` for these files.
So adding key K1 on node A while K2 is added on node B gives node A a local add on the main thread and a replicated add on a worker thread, writing `ssh/config` at the same time.
## a) Full-file `writeFile` on `config` overlays a concurrent writer
`writeFileEnsureDir` (`security/sshKeyOperations.ts:27-30`) is a plain `writeFile` — `O_TRUNC`, writes from offset 0, no temp+rename. Two writers overlay byte-for-byte, and the shorter write leaves the longer one's tail behind, producing an invalid token rather than merely losing a block. `ssh` then refuses to run at all:
- **Fresh node, two adds.** Both see `exists(configFile) === false` at `:221` (only a `mkdir` separates the check from the write) and both take the `writeFileEnsureDir` branch at `:224`. Result: one key's block gone, survivor ends `IdentitiesOnly yesyes` → `ssh -F config -G host` fails with `line 6: unsupported option "yesyes". / terminating, 1 bad configuration options`.
- **Populated config, mixed ops.** `delete_ssh_key`'s rewrite at `:368` truncates the file a concurrent add's `appendFile` is writing into. With overlapping `{delete, add, delete, add}`, **ssh rejected the config in 34 of 60 trials** (e.g. `IdentitiesOnly yesHost echo.example`). The same run resurrected a deleted key's block and dropped two live ones.
- **Two deletes, different names.** Both `readFile` at `:367` before either writes; last writer wins. **39/40 trials** left a `Host` block whose `IdentityFile` points at an unlinked key file.
Note `appendFile` itself is fine — `O_APPEND` is atomic, verified clean at 8-way concurrency. Only the full-file paths race.
**Fix:** drop the `exists(configFile) ? appendFile : writeFile` branch and always `appendFile` (it creates the file when missing). For `deleteSSHKey`'s rewrite, write `config.tmp` then `rename()`. Both still need a lock held across read→write — and because the competing writer can be another *thread*, that means an `O_EXCL` lockfile or routing the four mutating ops through the main thread.
## b) A stale `#name` block is never reconciled, and ssh sends the key to the old host
Once (a) leaves an orphan `#deploy` block, nothing repairs it: `addSSHKey` appends unconditionally (`:222`), so re-adding `deploy` yields **two** `#deploy` blocks, and `extractMatchingHostAndHostname` takes `match[0]` (`:420`).
`ssh_config` is first-obtained-value-wins. Confirmed with `ssh -G`: with a stale `#deploy → HostName github.com` above a fresh `#deploy → HostName gitlab.com`, `ssh -F config -G deploy-alias` reports `hostname github.com`. So a newly minted private key is offered to the **previous** remote, and `get_ssh_key`/`list_ssh_keys` report the stale hostname, which looks self-consistent to the operator.
**Fix:** make `addSSHKey` idempotent on the config — strip any existing `#name` block before appending — and have `deleteSSHKey` verify the block is gone rather than assuming the replace matched.
## c) The `#name` regex is unanchored, so it matches as a prefix
`#${escapedName}[\S\s]*?IdentitiesOnly yes` at `:366` and `:420`. `delete_ssh_key('deploy')` strips both the `#deploy` **and** the `#deploy_backup` blocks while `deploy_backup.key` stays on disk — silently breaking an unrelated key and leaving it in exactly the orphan state (b) exploits.
**Fix:** anchor with `#${escapedName}$` under the `m` flag.
## d) Truncating writes let readers see a short file, and `cloneSSHKeys` turns that into a silent cascade
Same `writeFileEnsureDir`: a reader interleaving between `open()` and the write sees short or empty content. Measured **157 of 3000** concurrent `readFile`s during a ~2 KB key write returned short or empty.
`cloneSSHKeys` (`cloneNode/cloneNode.ts:605-612`) feeds `get_ssh_key` output straight into `addSSHKey`, so a truncated read yields `key: ''` (Joi rejects) or a truncated `enc:v1:` envelope (`parseEnvelopeFields` throws). Either way `addSSHKey` throws inside the loop — and because the `try` at `cloneNode.ts:598` wraps the **entire** `for` loop, **every remaining key is skipped** with only a logged error. The same cascade fires with no race at all whenever a key's config block is missing, since `getSSHKey` omits `host`/`hostname` when no block matches but `addValidationSchema` requires both — exactly the state (a) and (b) produce.
**Fix:** temp+rename in `writeFileEnsureDir`; move `cloneNode`'s try/catch inside the loop and log which keys were skipped.
## Provenance
All four confirmed by code reading and reproduction during a `deep-review` of harper-pro#594; the reproduction counts above come from running the exact code sequences. All pre-existing and outside that PR's diff — `add_ssh_key generate=true` is a new way to reach (a), not its cause, and it raises the stakes because a minted key's public half cannot be recovered (#694) once the config is broken.
Related: #693 (same-node TOCTOU on the key file), #694 (`public_key` unrecoverable).
Contributor guide
Research direction
Start by reading security/sshKeyOperations.ts, especially writeFileEnsureDir, addSSHKey, deleteSSHKey, and extractMatchingHostAndHostname, then inspect cloneNode/cloneNode.ts:598-612. Reproduce the concurrent add/delete and truncated-read cases described in the issue. Done means concurrent mutations cannot corrupt or expose partial ssh files, key blocks are matched and reconciled by exact name, and cloneNode reports and continues past an individual key failure.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- nodejs, typescript
- Domain
- backend, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100