feat(agent-server): add require_secret_key to refuse plaintext secret persistence - #4618
feat(agent-server): add require_secret_key to refuse plaintext secret persistence#4618simonrosenberg wants to merge 4 commits into
Conversation
… persistence Adds an opt-in Config.require_secret_key flag (env OH_REQUIRE_SECRET_KEY). When set, FileSettingsStore/FileSecretsStore raise MissingCipherError instead of silently downgrading to plaintext storage when secrets are present and no OH_SECRET_KEY cipher is configured. Off by default, so the zero-config self-hosted/OSS path is unchanged. Fixes #4609.
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
simonrosenberg
added
the
review-this
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — clean design, correct implementation, a couple of symmetric test gaps worth filling.
The config field → helper → store-constructor wiring is consistent and readable. All write paths (save, set_secret, delete_secret via _save_with_versions) properly funnel through the require_cipher guard. The update() path on FileSettingsStore inherits the check automatically because it delegates to save(). The flag is off by default so existing deployments are unaffected. Error messages are actionable.
Two symmetric tests are missing (details in inline comments), and one docstring is pure noise. Otherwise good.
[TESTING GAPS]
tests/agent_server/test_persistence_require_cipher.py— two symmetricFileSecretsStorecoverage holes (see inline).
[STYLE NOTE]
openhands-agent-server/openhands/agent_server/persistence/store.py, line 854 — redundant docstring (see inline).
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Default isFalse; no behaviour change for any existing deployment. When enabled, the guard raises immediately with a clear, actionable message before writing anything. All secret write paths are covered by the same_save_with_versionsfunnel.
VERDICT:
✅ Worth merging — functionally correct and safe. The two missing tests are worth adding before or just after merge but are not blockers.
KEY INSIGHT:
The security contract (raise before writing, never silently store plaintext when the flag is on) is correctly enforced on every write path; the only gap is symmetric backward-compat test coverage for FileSecretsStore.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
- Add symmetric FileSecretsStore coverage for the two backward-compat cases (no-secrets no-op, default-off plaintext), mirroring the existing FileSettingsStore tests. - Drop the redundant _get_require_cipher docstring — it restated the signature verbatim.
|
Addressed both review points in fad27ba: added the two symmetric FileSecretsStore tests and dropped the redundant docstring on |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — Clean, minimal, well-tested opt-in guard. One improvement opportunity worth a follow-up.
Summary: This PR adds require_secret_key (OH_REQUIRE_SECRET_KEY) as an opt-in boolean flag that makes FileSettingsStore / FileSecretsStore raise MissingCipherError instead of silently falling back to plaintext when OH_SECRET_KEY is absent. The design is correct — default-off preserves the zero-config OSS path, and the guard is wired through both stores symmetrically. 8 tests exercise the real code paths across all combinations.
One gap worth a follow-up: The settings guard checks llm_api_key_is_set (LLM api_key field only). OpenHandsAgentSettings also has critic_api_key: str | SecretStr | None with its own serialize_secret serializer. A user with only critic_api_key set and require_cipher=True will silently get plaintext output — the guard won't fire. See inline comment for details. This matches the existing warning gate, so it's a pre-existing design boundary, but widening it here would make the guarantee complete.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Pure opt-in feature withdefault=False. Default-off means zero behavior change for existing deployments. The only code that executes on the new path is an early raise inside an already-existingif not cipher:branch — no new state, no new I/O. Test coverage is solid and the backward-compat test is explicitly preserved.
VERDICT:
✅ Worth merging — core logic is sound, the critic_api_key gap is a narrow pre-existing edge case that can be addressed in a follow-up once the enterprise opt-in wiring lands.
KEY INSIGHT: The secret-presence gate (llm_api_key_is_set / custom_secrets) is inherited from the existing warning path — broadening it to cover all SecretStr fields would make require_cipher airtight.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
PersistedSettings.has_any_secret covers both llm.api_key and verification.critic_api_key (a separate secret field on OpenHandsAgentSettings). Without this, a user who only set critic_api_key could still get plaintext storage even with require_cipher=True.
|
Addressed the critic_api_key gap in 6ee2092. Ready for another look. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🔴 Taste Rating: Needs improvement — the direction is sound, but the secret-detection predicate still has security-sensitive blind spots.
[CRITICAL ISSUES]
- [openhands-agent-server/openhands/agent_server/persistence/models.py, Line 184] Security/Correctness:
has_any_secretonly checks the LLM API key and critic API key. Other settings fields serialize secrets too (agent_context.secrets, MCPenv/headers, header auth credentials, OAuth token state), sorequire_cipher=Truecan still fall through to plaintext persistence for those settings.
[TESTING GAPS]
- Add regression coverage for
FileSettingsStore(..., require_cipher=True)with no cipher whenagent_context.secretsis populated, and for at least one MCP secret-bearing field. Those tests should fail today because the guard returnsFalseand the store writes plaintext.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This PR touches security-sensitive secret persistence. The default behavior remains unchanged, but the new opt-in guard can give multi-tenant deployments a false guarantee while still allowing some settings secrets to be persisted in plaintext.
VERDICT:
❌ Needs rework: the require_secret_key guard needs to cover every settings secret serializer, not just LLM/critic keys.
KEY INSIGHT:
The store should reuse the existing serialization pipeline to detect secret-bearing settings instead of hand-maintaining a partial list of secret fields.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
…checklist
has_any_secret previously checked only llm.api_key + critic_api_key by
name -- MCP server env/headers, agent_context.secrets, and any future
secret-bearing field would silently fall through require_cipher=True
to plaintext storage.
Replaced with a probe cipher passed as context={"cipher": probe} to
force every secret field's own serializer down its real 'encrypted'
branch (see resolve_expose_mode), so detection reuses the actual
serialization logic instead of hand-walking the model for SecretStr
instances -- which also can't see fields like AgentContext.secrets,
whose bare-string values are plain str at rest and only become
secret-shaped inside their own field serializer at dump time.
Added regression tests for MCP env secrets and agent_context.secrets.
|
Addressed the structural gap in 091ceb5 — has_any_secret now reuses the real serialization pipeline (probe cipher forcing every field's own serializer) instead of a hand-maintained checklist, so it generalizes to MCP secrets, agent_context.secrets, and any future secret-bearing field. Ready for another look. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — Elegant, focused solution that fixes a real gap in secret detection coverage.
What this does
Previously, the plaintext-storage guard only checked llm_api_key_is_set (settings) and custom_secrets non-empty (secrets), meaning MCP server env/headers, critic_api_key, agent_context.secrets, and any future secret-bearing fields were silently persisted in plaintext with no warning. This PR:
- Broadens detection via
has_any_secret, which probes the real Pydantic serialization pipeline instead of field-type inspection — correctly catchingAgentContext.secretswhose values are plainstrat rest and only become secret-shaped inside their field serializer. - Adds
require_secret_keyconfig flag (defaultFalse) that turns the warning into a hardMissingCipherErrorraise, intended for multi-tenant / production deployments. - Tests all the edge cases: critic key, MCP env, agent context secrets, empty-secrets bypass, cipher round-trips, backward compat.
One minor observation
In the no-cipher path, has_any_secret runs model_dump (probe) before save() runs model_dump again for the actual write — two serializations. Harmless given the filesystem I/O that follows, but worth noting if this path ever becomes latency-sensitive.
The PR description is empty, which makes it harder to understand the motivation from the PR UI, but the code and test comments are self-documenting enough.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
require_secret_keydefaults toFalse, so all existing behavior is fully preserved. The only behavioral difference for existing deployments is that the plaintext warning now fires for more secret types (MCP env vars, critic key, agent context secrets) — a strictly safer default. TheMissingCipherErrorpath is entirely opt-in.
VERDICT:
✅ Worth merging — Clean implementation, solid tests, backward-compatible.
KEY INSIGHT:
Using the real serialization pipeline as a probe (a fake cipher that sets a flag on encrypt()) is the right design: it guarantees coverage over all current and future secret-bearing fields without maintaining a fragile field checklist.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
2 participants
HUMAN:
Split this out of OpenHands/OpenHands#15722 after closing it — the multi-repo ACP consolidation it tracked was done, but this one remaining gap (plaintext secret fallback) was worth its own scoped issue and fix. Verified locally: new tests pass, existing test suite (273 tests across settings/profiles/credential-binding) is unaffected, and pre-commit (ruff, pyright, import-dependency rules) is clean.
AGENT:
Why
FileSettingsStore.save/FileSecretsStore._save_with_versions(openhands-agent-server/openhands/agent_server/persistence/store.py) silently fall back to storing secrets in plaintext at rest when noOH_SECRET_KEYcipher is configured — they log a warning and write the file anyway. This is a real gap for multi-tenant/Cloud deployments where a missingOH_SECRET_KEYis a deploy-config bug, not a legitimate zero-config state.The encrypt/decrypt paths elsewhere already treat a missing cipher as fatal (
pydantic_secrets.py'sMissingCipherErrorformode="encrypted",conversation_service.py'sValueErrorforsecrets_encrypted=True) — the gap is narrower than "missing cipher is silently swallowed everywhere." It's specifically that the persistence-layer save path chooses the plaintext serialization mode instead of ever attempting the encrypted one, so it never reaches those raises.Config(the agent-server's deployment config) is deployment-agnostic — the same code path serves self-hosted/OSS installs (whereOH_SECRET_KEYis legitimately often unset) and Cloud/multi-tenant SaaS (where it should always be set). A blanket hard-fail would break the zero-config OSS path, which an existing test (test_save_without_cipher_stores_plaintext_for_backward_compat) explicitly guards. So this is opt-in, not a default-behavior change.Summary
Config.require_secret_key: boolfield (defaultFalse), settable viaOH_REQUIRE_SECRET_KEY.get_settings_store/get_secrets_storeinto a newrequire_cipherconstructor param onFileSettingsStore/FileSecretsStore.require_cipher=True,save()raisesMissingCipherErrorinstead of downgrading to plaintext — but only when secrets are actually present (llm_api_key_is_set/custom_secrets, the same gate the existing warning already uses). No secrets present → no cipher needed, save proceeds normally.require_cipher=False) behavior is byte-for-byte unchanged — the existing backward-compat test still passes untouched.OpenHands/enterpriseopts in explicitly. That wiring is a separate follow-up PR inenterprise.Issue Number
Fixes #4609.
How to Test
New test coverage:
tests/agent_server/test_persistence_require_cipher.py(6 tests — raises without cipher for both stores when required, no-op when no secrets present, succeeds normally with a cipher, and confirms the default/off behavior is unchanged).Ran locally:
pytest tests/agent_server/test_persistence_require_cipher.py— 6/6 passedpytest tests/agent_server/test_settings_router.py tests/agent_server/test_profiles_router.py tests/agent_server/test_agent_profiles_router.py tests/agent_server/test_credential_binding.py tests/agent_server/test_mcp_oauth_store.py— 273/273 passedpre-commit run --files <changed files>— clean.github/scripts/check_persisted_settings_compat.py— passes (no schema fields touched)🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• 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:091ceb5-pythonRun
All tags pushed for this build
About Multi-Architecture Support
091ceb5-python) is a multi-arch manifest supporting both amd64 and arm64091ceb5-python-amd64) are also available if needed