feat(settings): encrypt MCP env / headers with cipher at rest#3193
Conversation
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Solid security improvement that properly encrypts MCP credentials at rest. Clean implementation with comprehensive test coverage and backward compatibility.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
MCP credentials are properly encrypted at rest using Fernet encryption, with successful round-trip decryption and legacy plaintext migration.
Does this PR achieve its stated goal?
Yes. The PR successfully encrypts MCP env and headers values when persisting OpenHandsAgentSettings to disk. Before this change, MCP credentials were stored in plaintext despite a cipher being configured. After this change, credentials are encrypted with Fernet (ciphertext starting with gAAAA), decrypted on load, and legacy plaintext values migrate cleanly on the next save.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Dependencies installed, project builds successfully |
| CI Status | |
| Functional Verification | ✅ MCP encryption, decryption, and migration all work correctly |
Functional Verification
Test 1: MCP credentials encrypted on disk
Step 1 — Create settings with MCP secrets:
Created OpenHandsAgentSettings with MCP config containing:
- GitHub token:
ghp-super-secret-token-abc123 - Authorization header:
Bearer tok-very-secret-xyz789 - API key:
sk-another-secret-def456
Step 2 — Serialize with cipher context (simulating FileSettingsStore.save):
cipher = Cipher(secret_key="agent-server-test-key")
dumped = settings.model_dump(mode="json", context={"cipher": cipher})Step 3 — Verify on-disk format:
Wrote serialized settings to settings.json and analyzed file content:
File size: 3069 bytes
Fernet token prefix 'gAAAA' count: 3
Plaintext secrets found: {'ghp-super-secret-token': False, 'tok-very-secret': False, 'sk-another-secret': False, '<redacted>': False}
MCP config in on-disk file:
- GITHUB_TOKEN: gAAAAABqANfconIneHDnY8sZwAEIGQ2hYR7hhZoYt-5kYQT830lbRAZzILYY...
- Authorization: gAAAAABqANfcuzaxlBMaMrfoYEs4RIx6i1HEs7TiChRvb2_4EkdRj7qoxKB4...
- X-API-Key: gAAAAABqANfcMrzJ02ba0KX01cyEUo7_zLfctaYkQutMiFef_-RGRiIstMIs...
All values encrypted: True (start with 'gAAAA')
Non-secret structure (command, URL) remains plaintext: True
This confirms secrets are encrypted on disk, not stored as plaintext or <redacted>.
Step 4 — Load from disk and verify decryption:
loaded_settings = OpenHandsAgentSettings.model_validate(
parsed, context={"cipher": cipher}
)Decrypted values:
GITHUB_TOKEN: ghp-super-secret-token-abc123 ✓
Authorization: Bearer tok-very-secret-xyz789 ✓
X-API-Key: sk-another-secret-def456 ✓
Round-trip integrity: All secrets match original
This confirms the decryption works correctly and recovers the original plaintext values.
Test 2: Legacy plaintext migration
Step 1 — Simulate legacy settings file (pre-encryption build):
Created payload with plaintext MCP env:
{
"mcp_config": {
"mcpServers": {
"github": {
"command": "uvx",
"env": {"GITHUB_TOKEN": "ghp-legacy-plaintext-token"}
}
}
}
}Step 2 — Load with cipher context:
restored = OpenHandsAgentSettings.model_validate(
legacy_payload, context={"cipher": cipher}
)Result: Token loaded successfully as ghp-legacy-plaintext-token (plaintext preserved).
Step 3 — Re-serialize (next save will encrypt):
re_dumped = restored.model_dump(mode="json", context={"cipher": cipher})Result: Token now encrypted (starts with gAAAA).
This confirms legacy plaintext values are not dropped — they pass through on load and get encrypted on the next save, enabling clean migration.
Test 3: REST API exposure modes
Tested all serialization contexts:
- Default (no context): Value is
<redacted>✓ expose_secrets="plaintext": Raw valuemy-secret-value✓- Cipher context: Value encrypted (starts with
gAAAA) ✓ expose_secrets="plaintext"+ cipher: Plaintext wins (not encrypted) ✓expose_secrets="encrypted"without cipher: RaisesPydanticSerializationErrorwrappingMissingCipherError✓
All exposure modes work as documented.
Issues Found
None. The PR delivers on its stated goal with correct encryption, decryption, and migration behavior.
CI Note: Pre-commit has a minor formatting issue (multi-line function signature formatting). Functional tests (sdk-tests, agent-server-tests) are all passing.
|
@OpenHands fix the conflicts and the pre-commit |
|
I'm on it! VascoSch92 can track my progress at all-hands.dev |
Per-value encrypt every string in MCP server env and headers
when persisting OpenHandsAgentSettings — the same way other
secret fields are encrypted via the cipher context. Other parts of
mcp_config (server names, urls, commands, args) remain plaintext.
Until now the on-disk persistence path passed only
``{"cipher": cipher}`` in the serialization context, and the
``mcp_config`` field serializer simply trusted that signal and
wrote env / headers verbatim — leaving user MCP credentials in
plaintext on disk despite a cipher being configured. This change
brings them under the same at-rest protection as
``SecretStr`` fields persisted via ``serialize_secret``.
- _serialize_mcp_config now:
- ``expose_secrets=plaintext|True``: raw values (backend trusted)
- ``expose_secrets=encrypted`` or ``cipher`` present:
Fernet-encrypt every env / headers value (raises
``MissingCipherError`` when encrypted-mode is requested
without a cipher, mirroring ``serialize_secret``)
- default: redact via ``sanitize_dict``
- New ``_decrypt_mcp_secret_values`` field validator decrypts
values on load when a cipher is in context. Values that don't
start with the Fernet "gAAAAA" prefix pass through as plaintext
so settings written by the previous build (which stored env /
headers unencrypted) migrate cleanly on the next save.
Co-authored-by: openhands <openhands@all-hands.dev>
VascoSch92
force-pushed
the
feat/encrypt-mcp-headers-env
branch
from
8d4ecd6 to
76677e7
Compare
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
|
OpenHands encountered an error: Request timeout after 30 seconds to https://cdcpiqdxfigjxigx.prod-runtime.all-hands.dev/api/conversations/f13ca76a-5702-4685-9154-a04538e62d18/ask_agent See the conversation for more information. |
- Add Cipher.try_decrypt_str so callers don't reach into _get_fernet(). - Promote FERNET_TOKEN_PREFIX to a public Final[str] in cipher.py and drop the duplicate from _secrets_exposure.py. - Extract resolve_expose_mode in pydantic_secrets.py and route both serialize_secret and _serialize_mcp_config through it so the plaintext/encrypted/redact decision lives in one place. - Replace cipher.encrypt(...) or "" with cast(str, ...); the empty-string fallback would have silently destroyed a secret if ever reached. - Deep-copy in _walk_mcp_secret_values so the validator no longer mutates the user-provided payload passed to model_validate. - Rename the over-long encrypt test and drop its # noqa: E501.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste
Clean security fix that properly encrypts MCP credentials at rest. The implementation follows established patterns (serialize_secret, cipher usage), maintains backward compatibility for legacy plaintext values, and has comprehensive test coverage.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Security improvement that fixes plaintext credential storage. Well-tested with backward-compatible migration path for existing installations. Follows existing encryption patterns and includes comprehensive tests covering encryption round-trip, legacy migration, error cases, and integration scenarios.
VERDICT:
✅ Worth merging: Solid security improvement with clean implementation
KEY INSIGHT:
Per-value Fernet encryption brings MCP credentials under the same at-rest protection as other SecretStr fields while transparently migrating legacy plaintext on first load.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
MCP env/headers are successfully encrypted at rest with Fernet cipher, decrypt correctly on load, and migrate legacy plaintext values seamlessly.
Does this PR achieve its stated goal?
Yes. The PR's goal was to encrypt MCP server env and headers values when persisting settings at rest using the configured cipher (the same way other SecretStr fields are encrypted). Functional verification confirms:
- Encryption works: When settings with MCP config are persisted with a cipher,
envandheadersvalues are encrypted with Fernet (start withgAAAAAprefix), not stored as plaintext or<redacted>. - Decryption works: Loading encrypted settings with the same cipher correctly decrypts the values back to their original plaintext.
- Migration works: Legacy settings files with plaintext
env/headers(written by pre-encryption builds) load without data loss and are re-encrypted on the next save. - Non-secret structure remains readable: Server names, commands, args, and URLs remain plaintext for debuggability.
- Agent-server integration works: The REST API (
PATCH /api/settings→ on-disk encryption →GET /api/settingswithX-Expose-Secrets: plaintext→ decryption) round-trips correctly.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv sync --dev completed successfully |
| CI Status | |
| Functional Verification | ✅ All encryption/decryption behavior verified end-to-end |
Functional Verification
Test 1: SDK-Level Encryption Round-Trip
Step 1 — Create settings with MCP secrets and persist with cipher:
Ran custom QA script that creates OpenHandsAgentSettings with MCP config containing:
"env": {"GITHUB_TOKEN": "ghp-test-secret-token"},
"headers": {"Authorization": "Bearer secret-api-key-12345"}Then called settings.model_dump(mode="json", context={"cipher": cipher}) to persist.
Step 2 — Verify on-disk encryption:
Inspected the generated JSON file:
On-disk content verification:
- GitHub GITHUB_TOKEN starts with 'gAAAAA': True
- API Authorization starts with 'gAAAAA': True
- Plaintext 'ghp-test-secret-token' in file: False
- Plaintext 'secret-api-key-12345' in file: False
- Redaction marker '<redacted>' in file: False
Encrypted GitHub token (first 50 chars): gAAAAABqAfJHVFaMH9fDm8z8S8MYLzRhJ8HTMDwswrXRAZ5DTO...
Encrypted API auth (first 50 chars): gAAAAABqAfJHAj50dzgPpyzAXGUKHuR-rj6bPEXQzuEbQuILLC...
This confirms:
- Secrets are encrypted with Fernet cipher (not plaintext)
- No redaction occurred (values are encrypted, not replaced with
<redacted>) - Non-secret fields (command, args, URL) remain plaintext for readability
Step 3 — Load and decrypt:
Loaded the same JSON with OpenHandsAgentSettings.model_validate(on_disk_json, context={"cipher": cipher}):
Decrypted values:
- GitHub GITHUB_TOKEN: ghp-test-secret-token
- API Authorization: Bearer secret-api-key-12345
This confirms round-trip encryption/decryption works correctly.
Test 2: Legacy Plaintext Migration
Step 1 — Simulate legacy plaintext on disk:
Created a settings payload with plaintext env (as old builds would write):
"env": {"API_KEY": "legacy-plaintext-key"}Step 2 — Load with cipher:
Loaded with OpenHandsAgentSettings.model_validate(legacy_payload, context={"cipher": cipher}):
Decrypted API_KEY: legacy-plaintext-key
This confirms the validator passes through plaintext values (no data loss).
Step 3 — Re-save to migrate:
Called model_dump(mode="json", context={"cipher": cipher}) on the loaded settings:
Migrated API_KEY (first 50 chars): gAAAAABqAfJHAv-MFVGRVXCgHWZEj5UPLXYphhjyQ18X6SlLt_...
Starts with Fernet prefix: True
This confirms legacy plaintext values are encrypted on the next save.
Test 3: Agent-Server REST API Integration
Ran existing pytest test test_patch_settings_encrypts_mcp_env_and_headers_on_disk:
tests/agent_server/test_settings_router.py::test_patch_settings_encrypts_mcp_env_and_headers_on_disk PASSED
This test:
- PATCHes
/api/settingswith MCPenv/headerscontaining secrets - Reads the on-disk
settings.jsonand asserts values start withgAAAA(Fernet ciphertext) - Asserts plaintext secrets do NOT appear in the file
- GETs
/api/settingswithX-Expose-Secrets: plaintextand asserts decrypted values match originals
Result: Passed. The full REST API persistence flow works correctly.
Test 4: SDK Unit Tests
Ran all new/modified SDK tests:
uv run pytest tests/sdk/test_settings.py::test_mcp_config_encrypts_env_and_headers_with_cipher \
tests/sdk/test_settings.py::test_openhands_agent_settings_mcp_config_decrypt_legacy_plaintext_on_disk \
tests/sdk/test_settings.py::test_openhands_agent_settings_mcp_config_expose_encrypted_requires_cipher \
tests/sdk/test_settings.py::test_openhands_agent_settings_mcp_config_expose_plaintext_passes_through -xvsResult: All 4 tests passed.
These tests cover:
- Encryption produces Fernet ciphertext and decrypts correctly
- Legacy plaintext values migrate without data loss
expose_secrets="encrypted"without cipher raisesMissingCipherErrorexpose_secrets="plaintext"overrides cipher and returns raw values
Issues Found
None. All verification passed cleanly.
Conclusion: This PR successfully implements MCP env/headers encryption at rest. Secrets are protected with Fernet cipher when persisted to disk, decrypt correctly on load, and legacy plaintext values migrate seamlessly. The implementation matches the design described in the PR and handles all edge cases (legacy migration, explicit plaintext exposure, missing cipher errors). No regressions detected.
4 participants
Stacks on top of #PARENT (
fix/mcp-settings-redacted-on-disk).Per-value encrypts every string in MCP server
env/headerswhen persistingOpenHandsAgentSettings— the same way other secret fields are encrypted via the cipher context. Other parts ofmcp_config(server names, urls, commands, args) remain plaintext.Until now the on-disk persistence path passed only
{"cipher": cipher}in the serialization context, and themcp_configfield serializer simply trusted that signal and wrote env / headers verbatim — leaving user MCP credentials in plaintext on disk despite a cipher being configured. This change brings them under the same at-rest protection asSecretStrfields persisted viaserialize_secret.Behavior
_serialize_mcp_confignow branches on context:expose_secrets=plaintext/True: raw values (backend trusted path).expose_secrets=encryptedorcipherpresent: Fernet-encrypt everyenv/headersvalue. RaisesMissingCipherErrorwhen encrypted-mode is requested without a cipher, mirroringserialize_secret.sanitize_dict(REST default unchanged).A new
_decrypt_mcp_secret_valuesfield validator decrypts values on load when a cipher is in context. Values that don't start with the Fernet"gAAAAA"prefix are treated as legacy plaintext and passed through unchanged, so settings written by the previous build (which storedenv/headersunencrypted) migrate cleanly on the next save.Tests
tests/sdk/test_settings.py:gAAAA…), no plaintext /<redacted>, and that round-tripping throughmodel_validate(..., context={"cipher": cipher})recovers the originals.expose_secrets="encrypted"without cipher test assertingMissingCipherErrorin the cause chain (Pydantic wraps it inPydanticSerializationError).expose_secrets="plaintext"test (overrides cipher even when both are present).tests/agent_server/test_settings_router.py: updated to assert on-disksettings.jsoncontains Fernet ciphertext (not plaintext) and thatGET /api/settingswithX-Expose-Secrets: plaintextround-trips to the original values.Local verification:
pytest tests/sdk/test_settings.py tests/agent_server/test_settings_router.py→ 87 passed.pytest tests/sdk/conversation/test_mcp_secrets_serialization_leak.py tests/sdk/conversation/local/test_state_serialization.py tests/workspace/test_cloud_workspace_sdk_settings.py→ 57 passed.ruff checkandpyrightclean on touched files.This PR was opened by an AI agent (OpenHands) on behalf of the repo owner.
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:5142d1e-pythonRun
All tags pushed for this build
About Multi-Architecture Support
5142d1e-python) is a multi-arch manifest supporting both amd64 and arm645142d1e-python-amd64) are also available if needed