Skip to content

security: encrypt MCP connector secrets with per-record derived keys - #32

Open
amal66 wants to merge 1 commit into
upstream-mainfrom
upstream-pr/connector-secret-encryption
Open

security: encrypt MCP connector secrets with per-record derived keys#32
amal66 wants to merge 1 commit into
upstream-mainfrom
upstream-pr/connector-secret-encryption

Conversation

@amal66

@amal66 amal66 commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Summary

MCP connector secrets — OAuth access/refresh tokens, client secrets, and auth configuration for the external services a user wires in (email, DMS, etc.) — are stored encrypted, but every row was encrypted under the SAME derived key (one scrypt derivation with a fixed salt). That single key is a master skeleton key: anyone who extracts it (memory dump, side channel, a leaked debug artifact) can decrypt every connector secret for every user in the database. This PR gives each stored secret its own unique 256-bit key, derived per record via HKDF-SHA256 from the master secret plus a random 16-byte salt. Compromising one record's key now reveals exactly one secret; the blast radius of a partial compromise drops from "all connector credentials" to "one".

Changes

  • backend/src/lib/mcp/client.ts:
    • deriveKey(salt) — HKDF-SHA256 (RFC 5869) over the existing MCP_CONNECTORS_ENCRYPTION_SECRET / USER_API_KEYS_ENCRYPTION_SECRET master secret with a fresh random 16-byte salt per encryption.
    • The connector tables have no salt column and span four encrypted fields, so instead of a schema migration the salt is packed into the stored value: v2. + base64(salt ‖ ciphertext). unpackCiphertext resolves the right key from the prefix; a wrong or forged salt derives a wrong key, so AES-GCM authentication fails closed (decrypt returns null/empty, never garbage plaintext).
    • Backward compatible: rows without the v2. prefix decrypt with the previous static scrypt key (legacyEncryptionKey), so existing connectors keep working; every new write is per-record. No migration required.
  • backend/src/lib/mcp/__tests__/crypto.test.ts: round-trip through the versioned scheme, fresh salt per encryption (same plaintext ⇒ different ciphertext), legacy static-salt ciphertext still decrypts, tampered salt/ciphertext fails closed to null.
  • backend/tsconfig.json: exclude *.test.ts / __tests__/** from the tsc build so shipped tests never enter dist/.

Why

Connector secrets are standing credentials into systems that hold privileged client material. Encrypting them all under one static key concentrates that entire surface behind a single secret-derived value; per-record keys de-correlate the rows so no single derived key is worth stealing. Confidentiality of client-adjacent credentials improves with zero operational cost: same env vars, same tables, no migration, old rows readable. No new runtime dependencies — HKDF comes from Node's built-in crypto.

Testing

  • cd backend && npm install && npm run build — tsc clean on the branch as committed.
  • With the vitest harness (upstream-pr/test-harness) merged locally: cd backend && npx vitest run2 test files, 16 tests passed (4 new crypto tests + 12 pre-existing harness tests).

Provenance

All changes are mechanical ports of code in amal66/mike@origin/main (commit b3166dd); exceptions:

  • The fork's version of this change also swaps console.error for its structured logger in the decrypt catch blocks; this repo has no logger module, so those lines are left as upstream's console.error (no change).
  • One comment clause referencing the fork's HKDF-based userApiKeys.ts was dropped, since this repo's userApiKeys.ts does not use that scheme.

Credits & prior art

🤖 Generated with Claude Code

https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC

Connector secrets (auth config, access/refresh tokens, client secrets)
were all encrypted under ONE key: crypto.scryptSync(secret,
"mike-user-mcp-v1"), a static salt shared across every row — extracting
that one derived key exposes every stored connector secret.

Derive a unique 256-bit key per secret via HKDF-SHA256 over a random
16-byte salt instead. The connector tables have no salt column and span
four encrypted fields, so rather than a migration, pack the salt into
the stored value: `v2.` + base64(salt || ciphertext). Decrypt resolves
the key from the prefix; a wrong/forged salt derives a wrong key so GCM
fails closed. Rows without the `v2.` prefix decrypt with the old static
key — existing secrets keep working, new writes are per-row. Covered by
crypto.test.ts (round-trip, per-row salt, legacy fallback, tamper →
null).

tsconfig excludes test files from the build output; the crypto unit
tests run under a vitest harness added separately.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC
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.

1 participant