Skip to content

Encrypt stored SSH deploy keys at rest - #582

Merged
kriszyp merged 3 commits into
mainfrom
fix/encrypt-ssh-keys-at-rest
Jul 20, 2026
Merged

Encrypt stored SSH deploy keys at rest#582
kriszyp merged 3 commits into
mainfrom
fix/encrypt-ssh-keys-at-rest

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 14, 2026

Copy link
Copy Markdown
Member

Summary

add_ssh_key wrote the private key plaintext to <rootDir>/ssh/<name>.key and replicated the raw key in the operation body — so the key material transited replication and landed on every peer's disk. Log redaction only ever covered the log surface, not storage or the wire.

The key is now sealed into an enc:v1: envelope (the same envelope encryption backing the hdb_secret store) before it touches disk or the replicated op body, closing both exposures at once. Core decrypts it to a transient 0600 file only for the lifetime of a git invocation.

  • Ingest (sealSSHKey): plaintext in → envelope on disk, envelope in the replicated op body.
  • Already-sealed keys (replicated from a peer, cloned from the leader) are stored verbatim — never decrypted just to forward them. Their kid is checked against this node's custody the way set_secret vets an ingested envelope.
  • get_ssh_key now returns the key as stored (the envelope). This is a deliberate behavior change, called out below.
  • update_ssh_key rotation and list_ssh_keys (names only) are externally unchanged.

Fixes the storage half of #581. Requires the companion core PR (see below).

⚠️ This needed a core change — please sanity-check the scope call

The issue and the dispatch both scoped this as harper-pro-only, on the reasonable assumption that the encryption primitives were the only core dependency. They are there (utility/secretEnvelope.ts, resources/secretDecryptor.ts) — but the at-use site is also in core: getGitSSHCommand() in core/components/Application.ts is what builds GIT_SSH_COMMAND, and it has no pro-side hook. There is no way to satisfy the issue's step 2 (decrypt-to-transient-file for the git operation) from pro alone, so this is a coordinated two-PR change:

I went ahead and built both rather than stopping to ask, so it's ready if you want it — but if you'd rather this land differently (e.g. a generic core hook that pro registers into), the core PR is cheap to abandon.

Degraded mode: plaintext + loud WARN (option (a))

The issue left this open. I found a clear precedent and followed it rather than inventing behavior:

  • Core's setSecret throws without custody — but that's a new op whose entire purpose is custody.
  • Core's ingestRegistryAuth passes the literal credential through without custody, rather than failing the deploy.

add_ssh_key is the ingestRegistryAuth case: an existing feature being retrofitted, which must keep working on a node with no custody. So with no custody the key is stored plaintext (today's behavior) with a WARN naming the key and telling you to configure secretCustody. Custody is present by default (the file tier generates a cluster keypair on first boot), so this is the exception, not the path. At use time the posture is the opposite and fail-closed: an encrypted key on a node that can't decrypt it is skipped and logged loudly, never treated as plaintext.

Where to look

  • get_ssh_key returning the envelope instead of the plaintext key is the one observable API change, and the bit I'd most like a second opinion on. It's what makes cloneSSHKeys carry ciphertext (it does get_ssh_key on the leader → add_ssh_key locally, which is another plaintext-over-the-wire path today), and it stops get_ssh_key from being a plaintext-key read for superusers. But if anything external depends on get_ssh_key returning usable key material, this breaks it. The issue only pinned update_ssh_key/list_ssh_keys as must-not-change, so I took the more secure reading — say the word if you'd rather it decrypt-on-read.
  • req.key is mutated to the envelope before replicateOperation(req) in both addSSHKey and updateSSHKey — that mutation is what keeps the plaintext off the wire, so it's load-bearing rather than incidental.
  • addSSHKey/updateSSHKey now also pass mode: 0o600 to the initial write, closing the brief window where a freshly-created key file sat at the umask default before chmod.

Tests

unitTests/security/sshKeyOperations.test.mjs — 10 tests: sealing on add (disk and the replicated op body), config block, verbatim storage of an already-sealed key, rejection of a foreign-cluster kid and of a malformed envelope, rotation, names-only listing, get_ssh_key returning ciphertext, and both degraded-mode paths. Full pro unit suite: 364 passing.


🤖 Generated by KrAIs (Claude Opus 4.8) with Claude Code

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces SSH key sealing at rest to prevent plaintext private keys from being stored on disk or transmitted over replication. It adds a sealSSHKey helper to encrypt keys using the cluster's secret custody, falling back to plaintext with a warning if custody is unavailable. Additionally, comprehensive unit tests are added to verify the sealing, replication, and degraded mode behaviors. The reviewer suggested using defensive optional chaining when retrieving the custody public key fingerprint to prevent potential runtime errors.

Comment thread security/sshKeyOperations.ts Outdated
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

kriszyp added a commit that referenced this pull request Jul 14, 2026
Chain optional access through getPublicKey() so a custody
implementation returning undefined fails gracefully instead of
throwing a TypeError, matching the existing optional-chaining intent
on custody itself.

Addresses gemini-code-assist review comment on PR #582.
@kriszyp
kriszyp marked this pull request as ready for review July 14, 2026 17:58
@kriszyp
kriszyp requested a review from a team as a code owner July 14, 2026 17:58
@kriszyp
kriszyp requested review from kylebernhardy and ldt1996 July 14, 2026 17:58
@dawsontoth

Copy link
Copy Markdown
Contributor

Could Studio encrypt it before sending it, too? Like we do for secrets?

kriszyp and others added 3 commits July 16, 2026 19:34
`add_ssh_key` wrote the private key plaintext to `<rootDir>/ssh/<name>.key` and
replicated the raw key in the operation body, so the key material transited replication
and landed on every peer's disk. Log redaction only ever covered the log surface.

The key is now sealed into an `enc:v1:` envelope (the same envelope encryption backing
the hdb_secret store) before it touches disk or the replicated op body, closing both the
at-rest and in-op-body exposures. A key that arrives already sealed — replicated from a
peer, or cloned from the leader — is stored verbatim and never decrypted to forward it,
so `get_ssh_key`/`cloneSSHKeys` carry ciphertext too. Core decrypts to a transient 0600
file only for the lifetime of a git invocation (`materializeGitSSH`).

Degraded mode: with no secret custody registered there is no key to seal against, so the
key is stored as plaintext — today's behavior — with a loud WARN. This follows core's
`ingestRegistryAuth`, which likewise passes a literal credential through rather than
failing the operation when custody is absent; SSH keys predate custody and must keep
working on a node that has none.

`update_ssh_key` rotation and `list_ssh_keys` (names only) are externally unchanged.

Refs #581

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Chain optional access through getPublicKey() so a custody
implementation returning undefined fails gracefully instead of
throwing a TypeError, matching the existing optional-chaining intent
on custody itself.

Addresses gemini-code-assist review comment on PR #582.
get_ssh_key now returns the sealed enc:v1: envelope by design (#581) —
update the test's expectation to match instead of the stale plaintext
assertion. The delete_ssh_key failure was a side effect: the prior
assertion failure skipped that test's cleanup, leaving a stray key for
the next test to trip over.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kriszyp
kriszyp force-pushed the fix/encrypt-ssh-keys-at-rest branch from ca1693a to 868407a Compare July 17, 2026 01:37

@dawsontoth dawsontoth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. Crazy idea... could we support generating SSH keys within the cluster purely? And then we can hand out the public key to super users, who could go stick it in GitHub or wherever. Then the private key never leaves the cluster. Not proposing we do that here, just kicking the idea out of my head and into the ether.

@kriszyp

kriszyp commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

Looks reasonable to me. Crazy idea... could we support generating SSH keys within the cluster purely? And then we can hand out the public key to super users, who could go stick it in GitHub or wherever. Then the private key never leaves the cluster. Not proposing we do that here, just kicking the idea out of my head and into the ether.

Looks like you have addressed this with #594. Thank you!

@kriszyp
kriszyp merged commit 948ead0 into main Jul 20, 2026
32 checks passed
kriszyp added a commit that referenced this pull request Jul 20, 2026
Chain optional access through getPublicKey() so a custody
implementation returning undefined fails gracefully instead of
throwing a TypeError, matching the existing optional-chaining intent
on custody itself.

Addresses gemini-code-assist review comment on PR #582.
@kriszyp
kriszyp deleted the fix/encrypt-ssh-keys-at-rest branch July 20, 2026 16:34
dawsontoth added a commit that referenced this pull request Jul 20, 2026
`add_ssh_key generate=true` (with key omitted) mints an ed25519 keypair on the node (async ssh-keygen); the minted private key flows through the same seal-at-rest + replicate path (sealSSHKey) as a client-supplied key, returning public_key for the caller to register (e.g. a GitHub deploy key). The private key never travels from the client. generate and key are mutually exclusive; the origin strips generate before replicating so peers receive a plain (sealed) key add. ssh-keygen failure (incl. not on PATH) is wrapped in a ClientError (no key material).

integrationTests cover generate=true (mints, returns public_key, sealed on disk), generate+key (rejected), and neither (client error). Rebased onto main to integrate with encrypt-at-rest (#582). Relates to HarperFast/harper#1778.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dawsontoth added a commit that referenced this pull request Jul 29, 2026
`add_ssh_key generate=true` (with key omitted) mints an ed25519 keypair on the node (async ssh-keygen); the minted private key flows through the same seal-at-rest + replicate path (sealSSHKey) as a client-supplied key, returning public_key for the caller to register (e.g. a GitHub deploy key). The private key never travels from the client. generate and key are mutually exclusive; the origin strips generate before replicating so peers receive a plain (sealed) key add. ssh-keygen failure (incl. not on PATH) is wrapped in a ClientError (no key material).

integrationTests cover generate=true (mints, returns public_key, sealed on disk), generate+key (rejected), and neither (client error). Rebased onto main to integrate with encrypt-at-rest (#582). Relates to HarperFast/harper#1778.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dawsontoth added a commit that referenced this pull request Aug 12, 2026
`add_ssh_key generate=true` (with key omitted) mints an ed25519 keypair on the node (async ssh-keygen); the minted private key flows through the same seal-at-rest + replicate path (sealSSHKey) as a client-supplied key, returning public_key for the caller to register (e.g. a GitHub deploy key). The private key never travels from the client. generate and key are mutually exclusive; the origin strips generate before replicating so peers receive a plain (sealed) key add. ssh-keygen failure (incl. not on PATH) is wrapped in a ClientError (no key material).

integrationTests cover generate=true (mints, returns public_key, sealed on disk), generate+key (rejected), and neither (client error). Rebased onto main to integrate with encrypt-at-rest (#582). Relates to HarperFast/harper#1778.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dawsontoth added a commit that referenced this pull request Aug 18, 2026
`add_ssh_key generate=true` (with key omitted) mints an ed25519 keypair on the node (async ssh-keygen); the minted private key flows through the same seal-at-rest + replicate path (sealSSHKey) as a client-supplied key, returning public_key for the caller to register (e.g. a GitHub deploy key). The private key never travels from the client. generate and key are mutually exclusive; the origin strips generate before replicating so peers receive a plain (sealed) key add. ssh-keygen failure (incl. not on PATH) is wrapped in a ClientError (no key material).

integrationTests cover generate=true (mints, returns public_key, sealed on disk), generate+key (rejected), and neither (client error). Rebased onto main to integrate with encrypt-at-rest (#582). Relates to HarperFast/harper#1778.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kriszyp pushed a commit that referenced this pull request Aug 20, 2026
…gen (#594)

* feat(security): add server-side keygen to add_ssh_key via `generate`

`add_ssh_key generate=true` (with key omitted) mints an ed25519 keypair on the node (async ssh-keygen); the minted private key flows through the same seal-at-rest + replicate path (sealSSHKey) as a client-supplied key, returning public_key for the caller to register (e.g. a GitHub deploy key). The private key never travels from the client. generate and key are mutually exclusive; the origin strips generate before replicating so peers receive a plain (sealed) key add. ssh-keygen failure (incl. not on PATH) is wrapped in a ClientError (no key material).

integrationTests cover generate=true (mints, returns public_key, sealed on disk), generate+key (rejected), and neither (client error). Rebased onto main to integrate with encrypt-at-rest (#582). Relates to HarperFast/harper#1778.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(security): reject a duplicate ssh key name before minting a keypair

With `generate: true` the duplicate-name guard ran after generation, so an add
against a taken name spawned ssh-keygen and wrote throwaway private-key material
to a temp file for a request that was guaranteed to throw "Key already exists".
Resolve the paths and run the exists() check right after the key/generate xor
validation instead, so a doomed request never mints anything.

Addresses cb1kenobi's review on #594.

* fix(security): mint ssh keypairs in process instead of shelling out to ssh-keygen

`ssh-keygen` is not present on a stock Windows host, and the subprocess had to
land the minted private key in a temp file before it could be read back — putting
plaintext key material on disk, which is most of what `generate: true` exists to
avoid.

Node's crypto generates ed25519 but cannot serialize it for SSH: its `pkcs8` and
`spki` PEM exports are formats OpenSSH refuses to load for ed25519 (`ssh-keygen -y`
reports `invalid format`), and a PEM public key is not what a host accepts as a
deploy key. So `sshKeyGeneration.ts` encodes the raw key bytes into the two formats
SSH actually reads: the `openssh-key-v1` private container and the one-line
`ssh-ed25519 <base64> <comment>` public key.

Verified against the real ssh-keygen, which loads the private key, re-derives the
same public key and comment, fingerprints both halves identically, and signs with
it — the signature is what proves the encoded private seed rather than just the
public blob embedded beside it.

The ClientError wrapping a failed `ssh-keygen` spawn goes away with the subprocess;
there is no longer an environmental failure mode to translate into a 4xx.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(security): enforce the no-line-break invariant on a generated key's comment

The comment lands verbatim on the one-line public key, so a line break would
split it and leave the tail parseable as a separate entry by whatever consumes
the key — an `authorized_keys` file, a deploy-key field. The invariant was
documented but only enforced indirectly, by `SSH_KEY_NAME_REGEX` in another
module; `generateEd25519SSHKeyPair` is exported from a security module, so it
now keeps the invariant itself.

Unreachable from today's only caller, which derives the comment from a
validated key name. A plain Error rather than a ClientError: reaching it means
a caller bug, not bad client input. Spaces stay legal — ssh treats the rest of
the line as the comment — and a test pins that so the guard isn't tightened
into rejecting them later.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(security): read `generate` as a strict boolean, and keep the unit tests off the network

`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. Conversely `generate='false'` alongside a
real `key` was rejected as "not both", so such a client could not add a key at
all.

Joi accepts those strings by coercion rather than rejecting them, so the schema
now marks the field `.strict()`, and the branch tests `req.generate === true`
rather than truthiness. `delete req.generate` also moves out of the mint branch
so no variant of the flag — including a literal `false` — reaches a peer.

Two unrelated fixes found in the same pass:

- The `generate` unit tests passed `hostname: 'github.com'`, which is the branch
  that really fetches api.github.com. They were making live network calls on
  every run, subject to GitHub's unauthenticated rate limit and to undici's full
  timeout on an offline machine (`.mocharc` sets no cap). Switched to
  `example.com`, as the pre-existing tests in the file already do.
- The comment claiming the minted plaintext "only ever exists in this process"
  was false on a node with no secret custody, where `sealSSHKey` passes the key
  through and it replicates in the clear. Reworded to name that exception rather
  than assert the opposite of what happens.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(security): use plain node:assert in the new keygen test, per the house style

`core/AGENTS.md` is explicit that new unit tests use the bare `node:assert`
module rather than `node:assert/strict`, calling `assert.strictEqual` /
`assert.deepStrictEqual` where strict semantics are actually wanted. It also
names `unitTests/security/` as one of the legacy directories that still imports
`/strict` but is "not the target shape" — which is exactly where this new file
landed, so the sibling file doing it is not a precedent to follow.

All 11 `assert.equal` become `assert.strictEqual` and both `assert.notEqual`
become `assert.notStrictEqual`, so no assertion loosens to `==`. `match`, `ok`,
and `rejects` are identical on plain assert.

Worth noting why lint did not catch this: the `no-restricted-imports` rule that
rejects these imports is configured in `core/.oxlintrc.json`, and harper-pro's
own oxlint config never inherited it, so `npm run lint:required` here is clean
either way.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants