feat: add --secrets keychain to install/up + guard-disable safety#9
Merged
Conversation
Phase 2 of encrypted secrets. Opt-in, default unchanged (plaintext), so no existing flow regresses. - install/up `--secrets keychain`: secret-flagged env vars are stored AES-GCM-encrypted in ~/.mcpm and written into client configs as `mcpm:keychain:server/KEY` placeholders instead of plaintext. Non-secret vars stay inline. mcpm guard resolves the placeholders at launch (Phase 1). Both commands print a "run `mcpm guard enable`" note. - `up --secrets keychain` is rejected under `--ci` (never persist secrets to a CI runner's keychain). - keychain.ts: add deriveKeychainId() — maps slash-containing registry ids (io.github.owner/repo) to a placeholder-safe keychain id. - guard disable: read-only post-disable scan warns when an unwrapped config still references `mcpm:keychain:` placeholders (they only resolve under guard). Never silently rewrites plaintext (security-first). Tests: 9 new (deriveKeychainId, install/up keychain placeholder writes, plaintext no-regression, missing-dep + --ci guards). 1082 pass; coverage 80.85% lines / 87.57% branches. typecheck + build clean.
…-disable Applies review findings #1–#5 from the multi-perspective review: - #1/#5: extract the secret→placeholder swap into a single `applyKeychainSecrets()` in keychain.ts (also now the home of `SecretsMode`). install.ts and up.ts both delegate to it, so the "no plaintext secret in config" invariant lives in one place and can't diverge. Drops the duplicated loop + the `storedSecretKeys` array (now a returned count). - #2: guard disable's placeholder warning is now a static import, never throws (advisory scan can't break a successful disable), and surfaces non-ENOENT config-read errors instead of swallowing them. - #3: `up --secrets keychain` only prints the "run mcpm guard enable" notice when a secret was actually stored (threaded via storedCount). - #4: add tests for applyKeychainSecrets, the slash-name keychain-id round-trip, and placeholderEnvKeys (the disable-warning detector). 1088 tests pass; typecheck + build clean.
…essaging
Addresses the dedicated security audit of the secrets feature:
- CRIT-1: deriveKeychainId is now injective — sanitized prefix + a
sha256 suffix — so names differing only in unsafe chars
("owner/repo" vs "owner_repo") can no longer collide into one secret
namespace (cross-server secret confusion/exposure).
- MED-1: applyKeychainSecrets persists all of a server's secrets in a
single atomic batch (new keychain.setSecrets) BEFORE returning the env
written to config — no orphaned half-written secrets if one fails.
install/up deps switch from setSecret to setSecrets accordingly.
- MED-4: install/up notices no longer overclaim — they state secrets are
"encrypted at rest (protects against other-user/offline access, not
same-user processes)", matching keychain.ts's documented posture.
Tests updated to the batch API + an explicit collision-injectivity test.
1088 pass; typecheck + build clean.
Pre-existing guard/keychain findings (full-env forward, event-log
sanitization, store write lock, log rotation) are out of scope here and
tracked separately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2 of encrypted secrets (builds on #8). Lets
installandupstore secret env vars encrypted instead of writing API keys as plaintext into client config files. Fully opt-in — the default is unchanged (plaintext), so no existing flow regresses.With
--secrets keychain, secret-flagged env vars are stored AES-GCM-encrypted in~/.mcpmand written into client configs asmcpm:keychain:server/KEYplaceholders.mcpm guardresolves them to real values at launch (the resolver shipped in #8). Non-secret vars stay inline.Changes
install --secrets <keychain|plaintext>— keychain mode stores eachisSecretvar viasetSecretand writes a placeholder; prints a "runmcpm guard enable" note. (commands/install.ts)up --secrets <keychain|plaintext>— same for stack installs; rejected under--ci(never persist secrets to a CI runner's keychain). (commands/up.ts)deriveKeychainId(name)— registry ids likeio.github.owner/repocontain/, which is invalid for a keychain id and breaks placeholder parsing; this maps them to a deterministic, placeholder-safe id. (store/keychain.ts)guard disablesafety — a read-only post-disable scan warns when an unwrapped config still referencesmcpm:keychain:placeholders (which only resolve under guard, so the server would otherwise receive the literal placeholder and fail). It never silently rewrites plaintext into configs (security-first). (guard/cli.ts)Why opt-in + the guard coupling
Resolution only happens on the guard launch path (#8). So
--secrets keychainis opt-in and always advises runningmcpm guard enable(the newly installed server isn't wrapped until guard re-runs). Default stays plaintext → zero regression. A guard-independent secrets shim remains a possible V2.Security properties
JSON.stringify(entry)must not contain the secret value).guard disablenever auto-materializes plaintext; it warns instead.Test plan
pnpm test— 1082 pass (9 new:deriveKeychainId, install/up keychain placeholder writes, plaintext no-regression, missing-setSecretguard,up --cirejection)pnpm lint(tsc) — cleanpnpm build(tsup) — cleanpnpm test:coverage— 80.85% lines / 87.57% branches (above thresholds)--secretsappears ininstall/uphelp; invalid mode rejected by the parser@inquirerprompt path (not driveable headlessly)Follow-ups
guard disable --materialize-secretsto convert placeholders to plaintext on the way out.