chore(mcp): encrypt user-scoped MCP credentials at rest - #26836
yuneng-berri merged 3 commits into
Conversation
LiteLLM_MCPUserCredentials.credential_b64 stored both BYOK API keys and OAuth2 access tokens as plain urlsafe-base64 of the raw value. Any DB read could recover the upstream-provider key. Run all writes through encrypt_value_helper (nacl SecretBox, the same helper used for the server-level credentials column) and read back via a small dual-path helper that tries decryption first, then falls back to plain base64 so existing rows keep working until they get rewritten. Folds the three near-identical "decode -> json.loads -> check type == oauth2" sites into _decode_oauth_payload, which simplifies the BYOK guard inside store_user_oauth_credential.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes a regression where Confidence Score: 5/5Safe to merge; the previously flagged P1 (missing key-rotation support) is resolved, and remaining findings are minor P2 style issues. The core security fix is correctly implemented with a sound backward-compatibility fallback. The key-rotation gap flagged in the prior review is now closed. All remaining findings are P2: a type-annotation widening and a cosmetic error-message improvement for unconfigured deployments. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/db.py | Adds encrypt_value_helper for write path, _decode_user_credential with nacl-first/base64-fallback for reads, _decode_oauth_payload helper consolidating three near-identical decode sites, and rotate_mcp_user_credentials_master_key to fix the previously flagged key-rotation gap. |
| litellm/proxy/management_endpoints/key_management_endpoints.py | Wires rotate_mcp_user_credentials_master_key into _rotate_master_key as step 4b, correctly passing new_master_key and catching exceptions so one failed table never aborts the overall rotation. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_db_credentials.py | New test file with 14 unit tests covering write-path encryption, round-trip reads, legacy-plaintext backward compat, BYOK guard, and master-key rotation for both BYOK and legacy rows; uses mocks only (no real network calls). |
Reviews (3): Last reviewed commit: "fix(mcp): re-encrypt user credentials du..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Three minor fixes from Greptile review: 1. _decode_user_credential now also catches TypeError so a null credential_b64 value returns None instead of propagating, matching the documented "returns None when neither path yields a valid string" contract. 2. The OAuth2 BYOK guard error no longer claims the existing row is a BYOK credential — after a salt-key rotation, an OAuth2 row can fail to decrypt and reach the same guard. Reword to "could not be verified as an OAuth2 token", which is accurate for both cases. 3. Drop the no-op sys.path.insert in the new test file (other tests in the directory don't need it; pytest picks up the package via the installed editable wheel). Adds a regression test for the None-input case.
|
@greptileai please re-review |
Greptile P1: this PR encrypts LiteLLM_MCPUserCredentials rows under the salt key, but the /key/regenerate rotation endpoint had no corresponding step for that table. Rotating the master key would leave every BYOK and OAuth2 user credential permanently unreadable. Adds rotate_mcp_user_credentials_master_key, mirroring the existing rotate_mcp_server_credentials_master_key pattern: read each row with the current key (via _decode_user_credential, which also handles unmigrated legacy plaintext rows), re-encrypt under the new master key, write back. One bad row is logged and skipped instead of aborting the whole rotation. Wired into key_management_endpoints.py as step 4b, alongside the existing server-credentials rotation, with the same try/except shape so a transient DB error on this table doesn't kill the whole regenerate-key flow. Tests cover: round-trip through rotation under a new key, automatic re-encryption of legacy plaintext rows (rotation also acts as a migration trigger), and a corrupt row not aborting the rotation.
|
@greptileai please re-review — addressed the P1 in e0b32eb: Added |
174c770
into
BerriAI:litellm_internal_staging
…ption chore(mcp): encrypt user-scoped MCP credentials at rest
2 participants
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewType
🐛 Bug Fix
Changes
LiteLLM_MCPUserCredentials.credential_b64stored both BYOK API keys and OAuth2 access tokens as plainurlsafe_b64encode(...)of the raw value — no encryption. The server-levelLiteLLM_MCPServerTable.credentialscolumn already usesencrypt_value_helper; only the user-scoped table regressed to plaintext.This wires every write to the user-credentials column through
encrypt_value_helper(nacl SecretBox), so a DB dump or replica read no longer hands over upstream-provider keys.Read path (backward compatible). Adds
_decode_user_credential(stored)which tries nacl decryption first and falls back to plainurlsafe_b64decodefor rows persisted by older code. ReturnsNonewhen neither path yields a valid string. Existing rows continue to work; new writes are encrypted.OAuth2 helper. Folds the three near-identical "decode → json.loads → check
type == 'oauth2'" sites (BYOK guard insidestore_user_oauth_credential,get_user_oauth_credential,list_user_oauth_credentials) into_decode_oauth_payload(stored) -> Optional[Dict]. Net effect is fewer code paths and a flatter BYOK guard.Tests. New file
tests/test_litellm/proxy/_experimental/mcp_server/test_db_credentials.py(14 cases) covers:list_user_oauth_credentialsmixes encrypted, legacy-plaintext, and BYOK rows and returns only the OAuth payloadsAll 150 tests across
test_db_credentials.py,test_byok_oauth_endpoints.py,test_mcp_management_endpoints.py,test_db.py, andtest_per_user_oauth_cache.pypass.