Decrypt SSH deploy keys to a transient file for git operations - #1795
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces secure transient materialization of SSH deploy keys during git-over-SSH operations. Keys are decrypted into a temporary directory with strict permissions only for the lifetime of a single git spawn and are cleaned up immediately after. The review feedback highlights two key improvements: robustly handling Windows path separators and case-insensitivity when repointing SSH config paths, and filtering out subdirectories in the SSH directory to prevent potential read errors.
|
Reviewed; no blockers found. All four review threads (Windows path handling, EISDIR on subdirectories, unguarded per-key |
- IdentityFile config rewrite: replace the naive `sshConfig.split(sshDir).join(tempDir)` substitution with a regex match that accepts either slash direction per path segment and is case-insensitive on win32, so it still repoints paths when sshDir (built via path.join, backslash on Windows) doesn't byte-match a forward-slash config. Extracted as `rewriteSshConfigPaths()` so the Windows-path behavior can be pinned directly from any CI host. - readdir the ssh dir with `withFileTypes` and filter to real files, so a stray subdirectory ending in `.key` is skipped instead of reaching `readFile` and throwing EISDIR, which would have aborted the whole spawn. - Wrap each key's `readFile` in its own try/catch, matching the existing decrypt-failure fail-open policy: a key that can't even be read (permission drift, concurrent rotation, transient EIO) is logged and skipped rather than aborting a spawn that may not need it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SSH deploy keys are being sealed at rest as `enc:v1:` envelopes (harper-pro#581), but git/ssh needs a key *file*. `materializeGitSSH` decrypts each sealed key into a fresh 0700 temp dir as a 0600 file and copies the ssh config with its `IdentityFile` paths repointed at the transient copies; `nonInteractiveSpawn` brackets every spawn with it, so the plaintext exists only for the lifetime of one git invocation and is removed on success, failure, and timeout alike. This is the same decrypt-to-transient-file shape already shipped for the per-deploy `.npmrc` (#1717). A key that cannot be decrypted (no custody on this node, an envelope sealed under a different cluster key, a tampered envelope) is logged and skipped rather than failing the spawn: a deploy that doesn't need that key is unaffected, and one that does fails with the usual SSH auth error. No log or error message carries key material or the envelope. Legacy plaintext keys pass through unchanged. Co-Authored-By: Claude Opus <noreply@anthropic.com>
- IdentityFile config rewrite: replace the naive `sshConfig.split(sshDir).join(tempDir)` substitution with a regex match that accepts either slash direction per path segment and is case-insensitive on win32, so it still repoints paths when sshDir (built via path.join, backslash on Windows) doesn't byte-match a forward-slash config. Extracted as `rewriteSshConfigPaths()` so the Windows-path behavior can be pinned directly from any CI host. - readdir the ssh dir with `withFileTypes` and filter to real files, so a stray subdirectory ending in `.key` is skipped instead of reaching `readFile` and throwing EISDIR, which would have aborted the whole spawn. - Wrap each key's `readFile` in its own try/catch, matching the existing decrypt-failure fail-open policy: a key that can't even be read (permission drift, concurrent rotation, transient EIO) is logged and skipped rather than aborting a spawn that may not need it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…onfig rewrite String.replace treats a string replacement argument as a pattern (`$&`, `$1`, etc.), so a literal `$` in normalizedTempDir could corrupt the rewritten IdentityFile path. A function replacer sidesteps special-token interpretation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
b12eaf5 to
a73ca35
Compare
- IdentityFile config rewrite: replace the naive `sshConfig.split(sshDir).join(tempDir)` substitution with a regex match that accepts either slash direction per path segment and is case-insensitive on win32, so it still repoints paths when sshDir (built via path.join, backslash on Windows) doesn't byte-match a forward-slash config. Extracted as `rewriteSshConfigPaths()` so the Windows-path behavior can be pinned directly from any CI host. - readdir the ssh dir with `withFileTypes` and filter to real files, so a stray subdirectory ending in `.key` is skipped instead of reaching `readFile` and throwing EISDIR, which would have aborted the whole spawn. - Wrap each key's `readFile` in its own try/catch, matching the existing decrypt-failure fail-open policy: a key that can't even be read (permission drift, concurrent rotation, transient EIO) is logged and skipped rather than aborting a spawn that may not need it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Harper Pro is moving SSH deploy keys to sealed-at-rest storage (harper-pro#581 — the key file and the replicated
add_ssh_keyop body becomeenc:v1:envelopes instead of plaintext). git/ssh still needs a key file, so core needs to decrypt one for the duration of a git invocation. This is that half.getGitSSHCommand()(which just pointedGIT_SSH_COMMANDat the durable<rootDir>/sshdir) is replaced bymaterializeGitSSH(), which:enc:v1:key into a fresh 0700 temp dir as a 0600 file, via the already-registeredgetSecretDecryptor()hook;configwith itsIdentityFilepaths repointed at the transient copies (known_hostsholds no secrets and stays put);cleanup()thatnonInteractiveSpawnruns in afinally, so the plaintext is removed on success, non-zero exit, and timeout alike.This is the same decrypt-to-transient-file shape already shipped for the per-deploy
.npmrcin #1717 — the SSH analogue of it.Legacy plaintext keys are copied through unchanged, so nodes with pre-existing keys (or no custody) keep working exactly as before.
Why this shape
Bracketing inside
nonInteractiveSpawnrather than around the whole deploy (as the.npmrcdoes onApplication) means one code path, cleanup guaranteed byfinally, no call-site churn, and theinstall_node_modulesspawn innpmUtilities.tsis covered for free. It also narrows the plaintext's lifetime from a whole deploy to a single spawn. The cost is areaddirper spawn, which is negligible next tonpm install.Where to look
materializeGitSSH's failure policy is the main design call worth a second opinion. A key that can't be decrypted (no custody on this node, an envelope sealed under a different cluster key, a tampered envelope) is logged and skipped, not fatal. Rationale: throwing would break every deploy on such a node — including npm-only ones that never touch that key — whereas skipping means a deploy that doesn't need the key is unaffected, and one that does fails with the normal SSH auth error (isSSHAuthFailurealready produces a good message). Fail-closed on the key, not on the whole node.sshConfig.split(sshDir).join(tempDir)), which relies onIdentityFilelines holding absolute paths into the ssh dir — which is how Pro'sadd_ssh_keywrites them.Tests
unitTests/components/gitSSHMaterialization.test.js— 10 tests: sealed key decrypts to a 0600 file in a 0700 dir withIdentityFilerepointed; legacy plaintext passes through; no-custody / foreign-cluster-key / tampered-envelope keys are skipped with no plaintext and no key material in the logs; and the transient dir does not outlive the spawn on success, on failure, or on timeout.Companion PR
Pairs with harper-pro #582 — Encrypt stored SSH deploy keys at rest, which does the sealing half. Neither is useful alone; the pro PR bumps the
coresubmodule to this branch.🤖 Generated by KrAIs (Claude Opus 4.8) with Claude Code