Repository navigation
fix(agent-server): support cipher key rotation and refuse writes under a lost key - #5758
Conversation
… cipher key After a cipher key change, undecryptable secrets loaded as None, so the stores' "different key" guards never fired and any settings PATCH or secrets PUT/DELETE answered 200 and saved null over the stored ciphertext. Read-modify-write paths in FileSettingsStore and FileSecretsStore now load with a strict_decrypt validation-context flag: validate_secret and validate_secret_dict raise SecretDecryptionError for a token the cipher cannot decrypt, the load fails, and the existing guards refuse the write (409 for settings, 500 for secrets), leaving both files untouched. Reads stay lenient. The F18.secret-key-rotation map bullet now passes, so its known-bug marker is removed. Fixes #5622 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Verified
- Scope: this belongs here. The agent-server settings/secrets stores and
openhands.sdk.utils.pydantic_secretsare owned by this repository, and the change is a bug fix matching #5622 (currentlyready-for-dev). - Head
56f4777is unchanged and CI for this head is green (23 success, 7 skipped, none failing). - The
F18.secret-key-rotationmarker removal is justified: the bullet loses itsknown buglabel and now asserts a 500 (secrets PUT), a 409 (settings PATCH), byte-untouched files, and the original key reading both values back — which the code delivers.
Checked against the code
- Exception safety: through real Pydantic model validation I confirmed
SecretDecryptionError(deliberately not aValueError) propagates unwrapped, so Pydantic does not wrap it in aValidationErrorthat would echo the Fernet token. The messages from both the plain and dict validators, and the router 409/500 detail bodies, contain no token. - Nested dict secret path: a settings write under a wrong key with an encrypted MCP
envvalue fails the strict load (returnsNone, soupdateraises and the route answers 409). The dict validator is strict-aware, so this path is not silently left lenient. - Lenient reads preserved:
load(),get_secret, and the GET routes still degrade toNoneinstead of raising. - No missed read-modify-write path: every in-repo writer of these stores (settings PATCH / meta-profiles / profiles / agent-profiles, and secrets PUT / DELETE / versioned credential binding) goes through
update/save/set_secret/delete_secret/load_versioned_secret/replace_versioned_secret, all nowstrict=True.
Test evidence
tests/agent_server/test_store_cipher_mismatch.py+tests/sdk/utils/test_pydantic_secrets.py: 52 passed.- Wider settings/secrets/profile suites pass. The 12
tests/agent_server/test_credential_binding.pyfailures reproduce identically on unmodifiedmain(confirmed in a clean worktree), so they are pre-existing and unrelated to this change.
Not blocking
- The secrets-500 vs settings-409 asymmetry raised in the description is a deliberate maintainer call; the linked acceptance criterion asks only for a non-2xx refusal, so it is not a defect here.
- The open thread from VascoSch92 (
use Final[str]on line 28) is a style nit without correctness impact and does not hold up approval.
✅ APPROVED
…keys Rotating OH_SECRET_KEY (or the session API key it falls back to) used to make every stored secret unreadable; #5622's guard only stopped writes from destroying them until the old key came back. Config gains previous_secret_keys (OH_PREVIOUS_SECRET_KEYS as a JSON list, or OH_PREVIOUS_SECRET_KEYS_0, _1, ...; also accepted by POST /api/init). Cipher encrypts with the current key and decrypts with the current key first, then each previous key (MultiFernet, same sha256 derivation per key), so values saved under an old key stay readable, and any save writes them back under the current key; a stale token carried through as is is rotated too. At startup the settings and secrets stores are re-encrypted under the current key when they hold a stale token, so the previous key can be dropped afterwards. Config.cipher is rebuilt when a model_copy changes the keys. The write refusal now applies only when no configured key can decrypt a value, and its message says to restore the key or add it to OH_PREVIOUS_SECRET_KEYS. The verify-agent-server map gains F18.secret-key-previous-keys and restart --keep-previous-secret-key. Fixes #5622 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
enyst
changed the title
…sal details by prefix The settings refusal detail now carries the OH_PREVIOUS_SECRET_KEYS guidance, so the F18, F20 and F22 corrupt-file bullets check it with contains instead of eq. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Context
Head 8621cf33 is a superset of the earlier commit (56f4777): it adds previous-key rotation (Cipher with MultiFernet, OH_PREVIOUS_SECRET_KEYS, startup re-encrypt, /api/init field) on top of the strict-decrypt refusal. The linked issue #5622 is still open and ready-for-dev; CI for this head is green (all check-runs success/skipped, none failing).
Verified
- The refusal path is unchanged and correct: read-modify-write loads use
strict_decrypt, the stores stay byte-for-byte unchanged, and the new hint (OH_PREVIOUS_SECRET_KEYS) is added without leaking tokens.SecretDecryptionErrorremains a non-ValueErrorso Pydantic does not wrap it. - Rotation mechanics are sound:
Cipher.encryptuses the current key,decrypt/try_decrypt_struseMultiFernet(current first, then previous),is_current/rotatebehave as documented, andserialize_secretre-encrypts a stale token on save.Config.ciphercorrectly rebuilds onmodel_copywith changed keys. FileSettingsStore.reencrypt/FileSecretsStore.reencryptrewrite only when the file holds a stale token and every value decrypts, otherwise leave it untouched;reencrypt_stale_storesswallows failures.- Focused tests pass locally:
tests/sdk/utils/test_cipher.py tests/sdk/utils/test_pydantic_secrets.py tests/agent_server/test_store_cipher_mismatch.py tests/agent_server/test_config.py tests/agent_server/test_init_router.py= 137 passed.
Material finding: previous_secret_keys delivered via /api/init do not reach an already-cached settings store
The /api/init rotation feature only works when the settings-store singleton has not been built yet. On a deferred-init (warm-pool) pod whose boot path reads persisted settings, _settings_store is constructed with the dormant boot cipher before /api/init runs. get_settings_store caches the first instance regardless of the config passed later (the docstring says so explicitly: "The config parameter is only used on first initialization. Subsequent calls return the existing instance regardless of config"), and InitService.initialize never resets the singletons. reencrypt_stale_stores(new_config) therefore calls get_settings_store(new_config), gets the stale-cipher instance, sees cipher.previous_keys == (), and does nothing.
Reproduction (deferred boot with a telemetry exporter configured, which triggers the boot-time consent read _read_misc_settings_sync -> get_settings_store(config).load() in telemetry/service.py), with settings.json holding an LLM key saved under the old key key-a:
- POST
/api/init{"secret_key": "key-b", "previous_secret_keys": ["key-a"]}returns 200. _settings_store.cipheris stilldormant-keywithprevious_keys == ()(notkey-b).GET /api/settingsreportsllm_api_key_is_set = false(the old LLM key is unreadable).PATCH /api/settingsreturns 409Settings file is corrupted or encrypted with a different key: restore that key as OH_SECRET_KEY or add it to OH_PREVIOUS_SECRET_KEYS.
So the warm-pool rotation path advertised in the PR description ("a warm-pool pod that receives a rotated per-user key ... can still read that user's old values") fails for settings: the user's stored settings become unreadable and any settings write is refused, which is the same symptom as the bug this PR fixes, reached through /api/init. _secrets_store happens to be built lazily at init time (it is not read at boot), so secrets are unaffected, which makes the inconsistency easy to miss. Note that test_previous_secret_keys_reach_the_cipher only asserts on _build_initialized_config(...).cipher and never exercises the store, so the suite does not catch this.
Suggested direction: in InitService.initialize, reset the persistence singletons (e.g. reset_stores()) after _build_initialized_config and before reencrypt_stale_stores(new_config), so the stores are rebuilt from the merged config; add an end-to-end /api/init test that seeds settings under the old key, boots a deferred pod with a settings-reading boot path, and asserts the old LLM key reads back after init.
Not blocking
- The secrets-500 vs settings-409 asymmetry remains a deliberate maintainer call.
🔄 CHANGES REQUESTED
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
🧹 PR Artifact Cleanup Queued The |
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Context
Head moved to bdea6cd0 (commits f4dfad4 "refresh persistence stores during deferred init" + evidence/test follow-ups) since the last review at 8621cf33. Linked issue #5622 is open and ready-for-dev. CI for this head is fully green (30/30 check-runs success). My prior blocking finding on 8621cf33 is addressed.
Prior finding resolved and verified
The earlier defect — previous_secret_keys delivered via /api/init never reaching an already-cached settings store on a warm-pool pod — is fixed. InitService.initialize now calls reset_stores() immediately before reencrypt_stale_stores(new_config) (init_router.py), so the stores are rebuilt from the merged config. I reproduced the original failure on 8621cf33 and re-ran the same scenario on this head: after POST /api/init {secret_key: key-b, previous_secret_keys: [key-a]} against a deferred boot with a settings-reading boot path, _settings_store.cipher is now key-b with previous_keys == ('key-a',), GET /api/settings reports llm_api_key_is_set=true, PATCH /api/settings returns 200, and the old secret still reads. The new regression tests/cross/test_remote_conversation_live_server.py::test_deferred_init_refreshes_persistence_ciphers (both rotated-key and current-key cases) passes locally.
Verified on this head
- Refusal path unchanged and correct: read-modify-write loads use
strict_decrypt, files stay byte-for-byte unchanged, and the newOH_PREVIOUS_SECRET_KEYShint leaks no token.SecretDecryptionErrorremains a non-ValueError, so Pydantic does not wrap it into a token-echoingValidationError. - Rotation mechanics sound:
Cipherencrypts with the current key and decrypts current-first then previous (MultiFernet);is_current/rotatebehave as documented;serialize_secretre-encrypts a stale token on save;Config.cipherrebuilds onmodel_copywith changed keys. reencrypt()rewrites only when the file holds a stale token and every value decrypts;reencrypt_stale_storesswallows failures.- Focused suites pass locally:
tests/sdk/utils/test_cipher.py tests/sdk/utils/test_pydantic_secrets.py tests/agent_server/test_store_cipher_mismatch.py tests/agent_server/test_config.py tests/agent_server/test_init_router.py= 137 passed.
Notes (not blocking)
- The
.pr/live-verification.mdartifact is per the repository's documented.pr/policy (temporary, removed on approval), so it is expected here. - One local run of the new cross test failed with
IndexErrorinenv_parser.merge, but that is a pre-existing bug in a file this PR does not touch, triggered only by ambientOH_*variables in this review sandbox; with a clean environment the test passes, and CI'scross-testsis green. - The secrets-500 vs settings-409 asymmetry remains a deliberate maintainer call, unchanged from the description.
No material findings remain.
✅ APPROVED
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review of head 1c42543d (fresh review: the head moved from bdea6cd to the merge commit 1c42543d; the linked issue #5622 is still open with bug / priority:low / ready-for-dev).
Scope — This change belongs here: this repo owns the Agent Server settings/secrets persistence and the cipher. The two decision points the author flagged (500 for refused secrets writes, strict refusal of every write to a store holding an undecryptable value) are reasonable defaults with clear rationale; neither requires a product/architecture call to proceed.
What I verified on the current head
- The core change files (
cipher.py,pydantic_secrets.py,persistence/store.py,config.py,init_router.py,settings_router.py) are byte-identical tobdea6cd, the commit this account approved on 2026-10-15. The only new work since then is the merge ofmain(plus an unrelated main-side edit merged intoprofiles_router.py). - All 53 non-skipped CI checks pass on this head, including
cross-tests,integration-test,sdk-tests,agent-server-tests, andCheck OpenAPI Schema. - Focused suites:
tests/sdk/utils,tests/agent_server/test_store_cipher_mismatch.py,test_config.py,test_init_router.py→ 339 passed, 2 xfailed. - The refusal path is sound:
SecretDecryptionErroris deliberately not aValueError, so Pydantic does not wrap it and echo the Fernet token; read-modify-write paths (update,save,set_secret,delete_secret,load_versioned_secret,replace_versioned_secret) load withstrict=Trueand leave both files byte-for-byte unchanged, while plain reads (load(),get_secret, GET routes) stay lenient. Rotation viaMultiFernet+previous_keys+ startupreencryptis correct and covered by the real-HTTP cross test. - The earlier
init_router.pyconcern (re-encrypting through a stale boot cipher) is addressed:reset_stores()now runs beforereencrypt_stale_stores(new_config)in the deferred-init path.
Non-blocking operational note — The PR is currently mergeable_state: dirty; I reproduced conflicts against the current main in openhands-agent-server/.../api.py (both sides edited the non-deferred lifespan region), tests/agent_server/test_config.py, and .agents/.../F18-settings-and-secrets.md. The branch needs a rebase/conflict resolution before it can merge. This is a merge-state matter, not a defect in the change itself, so it does not change the code verdict.
The one tests/agent_server/test_agent_launch_additions.py::test_launch_additions_apply_after_agent_resolution[False] failure I saw is not in this PR's diff and also fails on the base commit c73ec79 in this environment; it is pre-existing/environmental and unrelated.
No material correctness, security, or compatibility finding on the current head.
✅ APPROVED
…ange-409 Resolve conflicts: run reencrypt_stale_stores before main's export_session_api_key and ConversationService.get_instance(config) in the lifespan, keep both new test groups in test_config.py, and take main's fixed 422 secret-name contract alongside this PR's key-rotation entries in the F18 feature map. Narrow agent_settings to OpenHandsAgentSettings in test_store_cipher_mismatch.py now that ACPAgentSettings.llm is deprecated on main.
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Why
Changing the encryption key could turn unreadable saved secrets into
nullduring a successful write, destroying values that the old key could have recovered.Summary
Add previous-key decryption and rotation to
Cipher, propagate it through server configuration and deferred initialization, and use strict decryption for destructive read-modify-write paths. Rebuild the cached cipher when initialized keys change.Source: cipher.py · store.py · init_router.py.
REST API contract changes
Compared with base OpenAPI
3e208f61aacafor public/api/**paths.Issue Number
Fixes #5622
How to Test
Previously recorded verification
uv run pytest tests/sdk/utils tests/agent_server/test_store_cipher_mismatch.py tests/agent_server/test_config.py tests/agent_server/test_init_router.pygives 339 passed. New coverage:Cipher: decrypts with a previous key and encrypts with the current one;rotatereturns a new-key token, after which the previous key is not needed; without the old key, decryption still fails; previous keys are normalised.pydantic_secrets: strict decrypt accepts a previous key; the error namesOH_PREVIOUS_SECRET_KEYS;serialize_secretrotates a stale token.PATCH/PUT/DELETEanswer 200 and re-encrypt, and a server with only the new key then reads everything.reencrypt()rewrites only stale files, and leaves a file untouched when the listed key is wrong. Startupreencrypt_stale_stores. Without the old key, the refusal still holds (409, withOH_PREVIOUS_SECRET_KEYSin the detail).OH_PREVIOUS_SECRET_KEYSas a JSON list and indexed; a rotated session key with the session-key fallback;model_copyrebuilds the cipher;/api/initpasses the keys through.tests/agent_server tests/sdk/{utils,llm,conversation,profiles}gives 5230 passed. One canvas-extension test,test_exited_leader_descendant_does_not_block_recovery[status], also fails on unmodified main in this environment, so it is unrelated.map run --file F18 --only F18.secret-key-rotation,F18.secret-key-previous-keys --freshon this branch: bothpass. The server log showsRe-encrypted the settings store under the current keyand the same line for the secrets store.F18.secret-key-previous-keysagainst main (--checkout <main>):fail, because main ignores the previous key and the stored secret reads as 404.map run --file F18 --freshgives 29 pass and 5 xfail (the other known bugs), andF22gives 20 pass and 5 xfail.F20gives 30 pass, 5 xfail and 1 xpass:F20.secret-key-rotationis the expected side effect noted below, fixed for real by the stacked fix(agent-server): refuse activating or re-saving a profile under a changed cipher key #5775. The corrupt-file bullets in F18, F20 and F22 now match the settings refusal detail withcontains, because the detail carries the new guidance.map check: ok.F18.secret-key-rotationwasxfailtwice on main andxpasstwice on the first commit of this branch.Video/Screenshots
n/a
Type
Notes
previous_secret_keysaccepts the server’s JSON-list or indexed environment format;/api/initaccepts the optional list too. Only settings and secrets are eagerly rewritten, and only if all their values decrypt. Keep old keys configured while conversations, profiles, provider connections or MCP OAuth data still need them. ExplicitOH_SECRET_KEYavoids coupling encryption rotation to the first session API key. Lost-key secrets errors still use 500 with masked detail; settings use 409 with guidance. The lost-key named-profile case remains tracked in #5317. Named-profile write protection is stacked in #5775; reference-only credentials remain the longer-term direction in #4288.Visual explanation by Codex using HumanLayer show-me.
REST API contract changes
Compared with the merge base on
mainfor public/api/**paths. (The generated summary compared against a stale base,dfd8fe8, so it also listed changes from #5981 and #5983 that came in frommain. They are trimmed here.)🐳 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-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang: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:bd9d30d-pythonRun
All tags pushed for this build
About Multi-Architecture Support
bd9d30d-python) is a multi-arch manifest supporting both amd64 and arm64bd9d30d-python-amd64) are also available if needed