HarperFast / HarperFast/harper-pro

ssh/config is written without a lock by parallel threads: a corrupt config disables every deploy key on the node

Open
#722 0 comments 0 reactions 0 assignees View on GitHub
area:clustering bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.