feat(security): add_ssh_key generate=true — server-side ed25519 keygen

[dawsontoth]

Draft / prototype for #570. Part of the create-harper deploy-by-reference effort. This is the secondary SSH-key path — the default is the client-side-sealed token credential (HarperFast/harper#1778).

add_ssh_key generate=true (with key omitted): the node mints an ed25519 keypair, stores/replicates the private half through the existing write path, and returns the public_key for the caller to register (e.g. as a GitHub deploy key). The private key never has to travel from the client.

• Backward-compatible: supplying key behaves exactly as before.
• The "key or generate" invariant is enforced with a clear error.
• Peers re-running the replicated op already carry key, so they never re-generate.

Where: security/sshKeyOperations.ts for the operation; security/sshKeyGeneration.ts for the keypair itself.

Worth a look — the keypair is encoded by hand. Generation is in-process (node:crypto), with no ssh-keygen subprocess: that binary is absent on a stock Windows host, and shelling out meant the minted private key landed in a temp file before it could be read back, putting plaintext on disk that generate: true exists to avoid. Node can generate ed25519 but cannot serialize it for SSH — its pkcs8/spki PEM exports are formats OpenSSH refuses to load (ssh-keygen -y reports invalid format), and a PEM public key is not what a git host accepts as a deploy key. So sshKeyGeneration.ts encodes the raw key bytes into the openssh-key-v1 container and the one-line ssh-ed25519 <base64> <comment> public key directly. That encoder is the part of this PR that most deserves review.

Because hand-rolled crypto serialization is only worth doing if it's provably right, unitTests/security/sshKeyGeneration.test.mjs verifies it against the real ssh-keygen: load the private key, re-derive the same public key and comment, fingerprint both halves identically, and sign with it — the signature is what proves the encoded private seed, since -y alone would pass on a wrong seed by reading the public blob embedded beside it. Plus a sweep over comment lengths 1–16 to cover every residue of the 8-byte padding boundary. Those assertions skip where ssh-keygen is absent (Windows, the platform that motivated the change) and run everywhere else.

Refs #570. Docs: HarperFast/documentation#599 covers generate and matches the response shape here.

Generated by Claude Opus 5 via Claude Code.

Related — deploy-by-reference effort

• HarperFast/harper#1849 — two-phase stage/activate + revert_component
• HarperFast/harper#1850 — harper deploy by_ref=true (deploy by git reference)
• HarperFast/harper#1851 — harper deploy setup=true (client-side sealed deploy credential)
• HarperFast/harper#1876 — CI token auth + harper login --for-ci
• HarperFast/harper-pro#594 — add_ssh_key generate=true (cluster-side keygen)
• HarperFast/create-harper#118 — scaffolds this flow
• HarperFast/documentation#599 — two-phase deploy, revert, by-reference deploys, sealed credentials, add_ssh_key generate, OIDC (all v5.3.0)
• HarperFast/documentation#630 — CI token credentials in the canonical auth precedence (v5.2.0, shipped; mergeable independently)
• HarperFast/documentation#617 — by_ref ref pinning, credential=true, GitHub Actions behavior
• HarperFast/documentation#616 — merged into Replication observability: link-down-since timestamp per link #599's branch on 2026-07-30; its content now lives in the three PRs above

[gemini-code-assist]

Code Review

This pull request adds support for generating an ed25519 SSH keypair on the server side when generate: true is specified, keeping the private key secure within the cluster. Feedback on these changes focuses on performance, reliability, and type safety: using asynchronous execFile instead of blocking execFileSync to keep the event loop responsive, explicitly disallowing conflicting request parameters while wrapping key generation in a try-catch block to handle errors securely, and using Object.assign to prevent a TypeScript compilation error on the replication response.

[claude]

Reviewed; no blockers found.

[kriszyp]

A few suggestions. I think most importantly keep the plaintext of the disk seems central to the goal here. Otherwise, a great new feature!
_{🤖 Reviewed with GPT 5.6}

[dawsontoth]

@kriszyp — your review summary mentions "a few suggestions," but only the summary line came through; the inline comments aren't on the PR. I checked the REST comments endpoint, the per-review comments endpoint, and the GraphQL reviewThreads connection, and the review has an empty comment set on all three. So I think they were lost rather than that I'm overlooking them — could you re-add them?

The headline point in your summary is handled either way. Keeping the plaintext off disk was exactly right, and it turned out to be the same fix @cb1kenobi's Windows concern needed: c019827 drops the ssh-keygen subprocess for in-process node:crypto generation, so the minted private key no longer transits a temp file at all. Details and the verification approach are in that thread.

🤖 Posted by Claude Code (Opus 5) on Dawson's behalf

[kriszyp]

Looks good. Might be worth using wx file mode for better atomicity, but I wouldn't say atomicity is high priority.

Proposed inline comments (anchors failed):

• security/sshKeyOperations.ts: The preceding existence check and this default writeFile are a TOCTOU pair. Two concurrent generate: true requests for the same name can both observe no file, mint different pairs, and overwrite the same .key; both callers can then receive 200 with different public keys even though only the last private key remains. A retry while the first request is still running is enough to produce a deploy key that can never authenticate. Please atomically reserve/create the name (wx or a per-name lock) before generation, return the duplicate-name error to the loser, and keep the config update in the same serialized section.

_{(security/sshKeyOperations.ts:196 is not part of this PR's diff, so this is a file-level comment)}

• security/sshKeyOperations.ts: public_key exists only in this initial response. If the client disconnects or times out after the local key is committed, a retry gets “Key already exists,” while get_ssh_key exposes only the sealed envelope; automation cannot recover the public half without deleting and regenerating the key. Please persist the non-secret public key and expose it on retrieval/retry, or otherwise make generated adds idempotently return the existing public key.

_{(security/sshKeyOperations.ts:284 is not part of this PR's diff, so this is a file-level comment)}

_{🤖 Reviewed with Codex}

[dawsontoth]

Thanks @kriszyp — and the inline comments came through this time, so we have them now.

Both are real; I checked them against the code rather than taking them on faith. Filed as follow-ups so they don't hold up this PR:

• add_ssh_key: TOCTOU between the duplicate-name check and the key write hands one caller an unusable generated deploy key #693 — the TOCTOU. Confirmed: writeFileEnsureDir calls writeFile with no flag, so it defaults to 'w' (create-or-truncate), and there are two awaits between the exists check and the write. The generate: true case is the damaging one exactly as you describe, since each racer mints different material and the loser walks away with a public key that can never authenticate. The SSH config block has the same shape of problem — L209 appends unconditionally, so a race leaves two #name blocks for one key. Captured your wx-or-per-name-lock suggestion, including keeping the config update in the same serialized section.
• add_ssh_key generate=true: the minted public_key is unrecoverable if the generating response is lost #694 — public_key recoverability. Confirmed there's no way back to the public half once that response is lost: get_ssh_key returns the sealed envelope by design, retry gets Key already exists, and update_ssh_key won't mint. Wrote up both options you implied — persist the public half, or make generated adds idempotent — with a note that they interact with add_ssh_key: TOCTOU between the duplicate-name check and the key write hands one caller an unusable generated deploy key #693 and are probably worth doing together.

One thing worth flagging on #693: any reservation scheme needs to stay compatible with the replicated path, since peers re-run the op with key already present and should be able to write deliberately. That's called out in the issue.

🤖 Posted by Claude Code (Opus 5) on Dawson's behalf

[dawsontoth]

@kriszyp I think this is ready to merge, yeah? Or are we waiting for some more 5.2 stuff to land?

[cb1kenobi]

Re-reviewed 67db98b7 — no issues found. This PR looks good, nice job!

The one Nit left open from the last pass is fixed: generateEd25519SSHKeyPair now rejects a \r/\n in the comment, and it does so before generateKeyPairAsync, so no key material is minted on the rejected path.

The openssh-key-v1 encoder is byte-for-byte unchanged in this push, but I re-ran the cross-check against real ssh-keygen rather than assume it still held — 331 assertions, 0 failures, across comment lengths 0-40 and the newly-allowed spaced comments: the private key loads, re-derives the identical public key, both halves fingerprint the same, and -Y sign / -Y verify round-trips (the signing leg being what proves the encoded private seed rather than the public blob sitting beside it).

Both unit suites run and pass, 22/22 — 7 in sshKeyGeneration.test.mjs with the real-ssh-keygen block executing rather than skipping, and 15 in sshKeyOperations.test.mjs including the whole generate: true group. For anyone else hitting the empty-core-submodule wall in a worktree: git submodule update --init --depth 1 core populates it, sshKeyGeneration.test.mjs then needs --conditions=typestrip, and sshKeyOperations.test.mjs needs a tsc build first.

—
Generated by Barber AI

[kriszyp]

Looks good to me (leaving comments in place for the record, and I am investigating the failed anchors).

---

Proposed inline comments (anchors failed):

• security/sshKeyOperations.ts: The preceding existence check and this default writeFile are a TOCTOU pair. Two concurrent generate: true requests for the same name can both observe no file, mint different pairs, and overwrite the same .key; both callers can then receive 200 with different public keys even though only the last private key remains. A retry while the first request is still running is enough to produce a deploy key that can never authenticate. Please atomically reserve/create the name (wx or a per-name lock) before generation, return the duplicate-name error to the loser, and keep the config update in the same serialized section.

_{(security/sshKeyOperations.ts:196 is not part of this PR's diff, so this is a file-level comment)}

• security/sshKeyOperations.ts: public_key exists only in this initial response. If the client disconnects or times out after the local key is committed, a retry gets “Key already exists,” while get_ssh_key exposes only the sealed envelope; automation cannot recover the public half without deleting and regenerating the key. Please persist the non-secret public key and expose it on retrieval/retry, or otherwise make generated adds idempotently return the existing public key.

_{(security/sshKeyOperations.ts:284 is not part of this PR's diff, so this is a file-level comment)}

_{🤖 Reviewed with Codex}

[dawsontoth]

Ran a deep-review pass over this branch (auth / replication / concurrency / api). One finding was a defect in this PR's own code and is fixed in 45f3cd9; the rest are pre-existing and filed separately.

Fixed here (45f3cd9): generate was read for truthiness, and validateBySchema discards Joi's coerced value — so a caller that stringifies booleans reached addSSHKey with req.generate still a string, and 'false' is truthy. add_ssh_key generate='false' with no key therefore minted a keypair the caller explicitly declined, where before this feature the same body was a clean key is required error; and generate='false' alongside a real key was rejected as "not both", so such a client could not add a key at all. The schema is now .strict() (Joi accepts those strings by coercion rather than rejecting them) and the branch tests req.generate === true. delete req.generate also moved out of the mint branch so no variant of the flag reaches a peer. Regression test added.

Two smaller ones in the same commit: the generate unit tests passed hostname: 'github.com', which is the branch that really fetches api.github.com — they were making live network calls every run, unlike every pre-existing test in that file. And my comment claiming the minted plaintext "only ever exists in this process" was false on a node with no custody, where sealSSHKey passes the key through and it replicates in the clear.

Filed, not fixed here — all pre-existing, none introduced by this PR:

• ssh/config is written without a lock by parallel threads: a corrupt config disables every deploy key on the node #722 — ssh/config is written without a lock by genuinely parallel threads (ops API is main-thread, inbound replicated ops run on workers). A truncating full-file writeFile overlays a concurrent writer and produces an invalid token, at which point ssh refuses the entire config and every deploy key on the node stops working. Also covers the unreconciled stale #name block (ssh is first-match-wins, so a re-added key gets offered to the previous host), the unanchored #name regex (delete_ssh_key('deploy') also strips #deploy_backup), and the truncated-read cascade through cloneSSHKeys.
• add_ssh_key generate=true: a minted key is unusable on peers with different secret custody, and same-name generates diverge — both reported as success #723 — a generate-minted key is sealed against the origin's custody, but custody is only ever shared by cert-auth cloneNode; add_node never syncs it. Peers reject the envelope, the origin still returns Added ssh key, and since a generated key has no plaintext copy anywhere the peer can never obtain it. Also covers same-name generate on two nodes diverging into two different private keys, both reported as success.
• add_ssh_key: durable writes precede an untimed api.github.com fetch, so a concurrent delete_ssh_key leaves a revoked key live on every peer #724 — all durable writes land before an untimed api.github.com fetch and replication happens after, so a delete_ssh_key in that window deletes locally while the add still reaches every peer: a revoked deploy key surviving cluster-wide.
• add_ssh_key / update_ssh_key private keys reach the operations log: key is missing from the processLocalTransaction strip list harper#2199 — key is missing from processLocalTransaction's log strip list, so SSH private keys reach the operations log at info/debug/trace. harper-pro's own logRedaction.ts already strips this exact field for the replication paths; the operations log never got the same treatment. Fix is in core, so it needs a core commit plus a submodule bump rather than landing here.

What held up under the review: the hand-rolled openssh-key-v1 encoder, checked field-by-field against a real ssh-keygen container, 200 random keys re-derived, 21 comment shapes (empty, tabs, multi-byte UTF-8, 65535-char), and the returned public key independently re-derived from the extracted seed to prove it is the true counterpart. Also cleared: peers cannot receive generate or, where custody exists, plaintext; no replication loop; cloneNode's forwarding path still validates.

Note this needs a fresh approval on 45f3cd9 — require_last_push_approval is on, and it was already unsatisfied before this push.

🤖 Posted by Claude Code (Opus 5) on Dawson's behalf

[cb1kenobi]

Re-reviewed 45f3cd9e — no issues found. This PR looks good, nice job!

Both halves of the commit hold up under checking, and the ✅ from 67db98b7 still stands.

Strict generate parse. I ran the full input matrix against joi 17.13.4 through a faithful replica of validateBySchema (which does discard result.value, as your comment says), old schema vs new:

input	before	after
true	generate	generate
true / TRUE / FALSE	generate	reject
false	generate	reject
false, absent, undefined	no-gen	no-gen
1, 0, 1, 0, yes, ``, null, `{}`, `[]`	reject	reject

Only the four strings joi coerces change, and every one of them moves toward rejection — nothing that previously declined to mint now mints, so the narrowing carries no regression in the dangerous direction. Validation runs ahead of every side effect (validateBySchema → strict read → mutual exclusion → duplicate-name exists() → mint), and the silent-success case is unreachable: the only non-true values that survive the schema are false and absent, both of which fall through to the explicit requires `key`, or `generate: true` error rather than a 200 with no key.

Worth noting the idiom is not bespoke — core already reaches for it for exactly this reason in dataLayer/schemaDescribe.ts (exact_count, skip_record_count, include_computed are all Joi.boolean().strict()).

Tests off the network. I did not take this on trust: I re-ran the suite under a preload that hard-throws on dns.lookup/dns.promises.*, net.Socket.prototype.connect, net.connect, tls.connect, http(s).request, and fetch. The new file passes 16/16 with all of that blocked, so it is genuinely socket-free rather than merely fast. Running the pre-fix file under the same block gives 14 passing / 1 failing — "mints the keypair…" fails because the github.com branch really did reach api.github.com and the swallowed failure appended "Unable to get known hosts…" to the asserted message. The dependency was real.

Mutation test, rebuilding and re-running each time:

• revert both .strict() and === true → fails (Missing expected rejection: expected "false" to be rejected outright) — the original bug, reproduced
• revert only .strict() → fails
• revert only req.generate === true → passes 16/16

So the suite does discriminate the fix, with the schema carrying it; the === true read is untested defence-in-depth, which is exactly what the comment claims it to be. Fine as-is — just don't let a future refactor read that green suite as coverage of the comparison.

Counts: 16 passing in sshKeyOperations.test.mjs, 7 in sshKeyGeneration.test.mjs (including the three real ssh-keygen cross-checks), 23/0 combined with the network blocked. CI is clean at head — unit v22/v24/v26, integration 3/3 and cluster 6/6, build, lint, Socket all green.

On the private key, checked but not re-litigated: only public_key leaves the node; logRedaction.ts strips key on the replication path; no error string in either module interpolates key material. On the operations log I can add a detail to #2199 in your favour — the strip happens at the top of processLocalTransaction on a fresh rest-spread of req.body, so on the generate path there is no key field yet and the minted private key never reaches that log at all. The gap is the supplied-key path only, and it is pre-existing rather than anything this feature widened.

One thought for later, no action here: .strict() is the right local call, but the reason it is needed is that validateBySchema drops joi's coerced value entirely — so every non-strict Joi.boolean() in Harper carries the same latent truthy-string bug (~13 in core, 3 in harper-pro). Might be worth an issue against the wrapper so the next one is caught by construction.

—
Generated by Barber AI

[dawsontoth]

Thanks — the mutation test is the useful part here, and I'm taking the conclusion as-is: reverting only req.generate === true still passes 16/16, so the schema's .strict() is what the suite actually pins and the comparison is untested defence-in-depth. That's inherent rather than a gap I can close — with .strict() in place there is no input that reaches the comparison in a non-true, non-false state, so a test for it would have to bypass validation, and a test that bypasses the thing protecting it isn't testing the real path. Leaving it, with the comment saying why it's there.

One correction, on #2199 — I'd rather not add the detail you offered, because it would narrow the issue past what's true. You're right about the origin: on the generate path req.key is still undefined when the log line runs, and the issue already calls that exemption out explicitly. But the supplied-key path on the origin isn't the only gap. A peer receiving a replicated add_ssh_key is the second one, and it's the one that matters for a minted key:

replicationConnection.ts:2952 redacts for its own debug line, then hands the unredacted object to server.operation(...) on the very next line → serverUtilities.ts:311 operation() → processLocalTransaction({ body: operation }, …) → the operationLog.info(cleanBody) at :111. On a peer the received body already carries key, so it's in that log — the enc:v1: envelope where custody exists, and the plaintext private key on a node with no custody, since sealSSHKey passes it through in that mode. So a generate-minted key does reach an operations log, just not the origin's. The issue states both paths; narrowing it to supplied-key-only would send someone to fix the wrong half.

Your last point was the most valuable thing in the review, and it's now filed: HarperFast/harper#2200. .strict() is a per-site workaround for a wrapper-level problem — validateBySchema discards result.value entirely, so every non-strict Joi.boolean() carries the same latent truthy-string bug. Counts came out at 15 in core and 4 in harper-pro (6 in core already carry .strict(), so the idiom is established, as you noted).

Checking those four surfaced one that isn't latent. security/certificate.ts:254 has is_authority: Joi.boolean().required(), read for truthiness at :292 and :325 — so add_certificate with is_authority: "false" takes the CA branches even though the caller said it isn't a CA: the "non-CA certs must have a private key" guard at :292 is skipped entirely. super_user-gated, and adjacent to the already-ticketed CORE-3069 sub-CA work, but it's a live instance rather than a hypothetical, so it's written up in #2200 as the concrete case.

CI is green at 45f3cd9 (26 success, 6 skipped). Note @kriszyp's approval is on 67db98b and require_last_push_approval is enabled, so it doesn't carry to this head — a fresh approval on 45f3cd9 is what's needed to unblock.

🤖 Posted by Claude Code (Opus 5) on Dawson's behalf

[kriszyp]

Some codex suggestions, but concurrent requests isn't exactly a "major" IMO.
_{🤖 Reviewed with Codex}