fix(secrets): make the managed-credential write guard structural - #101
fix(secrets): make the managed-credential write guard structural#101simonrosenberg wants to merge 3 commits into
Conversation
The guard this replaces was a baseline remembered in per-request memory: ``store`` preserved CODEX_AUTH_JSON only when the same store instance had called ``load`` first and the submitted value matched what it read. Two live paths defeat that, both proven as regression tests here: - a whole-document save on an instance that never loaded restores the value it carries, undoing a runtime rotation; - ``invalidate_legacy_secrets_store`` runs inside GET /settings, loads, then saves with empty custom_secrets, which deletes the credential outright. ``store`` now drops protected names from the submitted document and excludes their rows from its delete, unconditionally. Whether the caller loaded first no longer matters, so ``_loaded_codex_auth``, ``preserve``/``preserve_codex``, the Codex row lock and its description carry-forward all go away. Writing one is now only possible through ``replace_protected_credential`` / ``delete_protected_credential``, which the three /secrets endpoints dispatch to so a user can still manage their own credential. Renaming into or out of a protected name is refused rather than silently breaking an armed binding. The credential stays a CustomSecret in the same table with the same row encryption and the same SecretStr model, and ``load`` still returns it, so the arming predicate, OSS process sandboxes and flag-off rollback are unaffected. No schema change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Filed the scope note as #102 — the general form of this bug. Reproduced against |
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||
Self-review of the previous commit found it reintroduced the exact failure it
set out to remove. The PUT /secrets/{name} branch for a protected name called
replace_protected_credential with the value it had loaded, so a metadata edit
was a read-modify-write over a rotating credential:
value after description-only edit : {"tokens":{"refresh_token":"r0"}}
rotation preserved? : False
generation changed by metadata edit?: True
Two defects in one: it restored a value the runtime had already rotated away,
and it minted a new generation, invalidating the runtime's compare-and-swap
token so the next flush would 409 on a description change.
Adds set_protected_credential_description, which touches neither the value nor
the generation, and points the endpoint at it.
The router branches had no test coverage, which is why this got through. Adds
six tests over the real FastAPI app and a real store covering create, delete,
description edit, both rename refusals, and an unrelated create leaving a
rotated credential intact; test_managed_description_edit_keeps_value_and_generation
fails against the previous commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review found a bug I introduced here — fixed in
|
simonrosenberg
left a comment
simonrosenberg
left a comment
There was a problem hiding this comment.
🟡 Acceptable — The built-in structural guard and /secrets dispatch are well-shaped and the new real-path tests catch the prior clobber, but the public store extension contract is broken.
[CRITICAL ISSUES]
- One blocking compatibility regression for existing custom
SecretsStoreimplementations is documented inline.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH
This changes user-facing secret writes and a documented secrets-storage extension point. The built-in stores are covered, but existing external implementations now fail at runtime for the protected name. Recommendation: Do not auto-merge; have a human reviewer validate the backward-compatibility/fallback contract for custom stores.
VERDICT:
❌ Needs rework: Preserve the legacy custom-store path or introduce explicit capability handling instead of a runtime 500.
KEY INSIGHT:
A structural guard is the right design for supporting stores, but unsupported extension implementations need an explicit legacy boundary rather than inherited traps.
This review was created by an AI agent (OpenHands) on behalf of the PR author.
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.
| description: str | None = None, | ||
| ) -> None: | ||
| """Write a protected credential, the only user-facing way to change one.""" | ||
| raise NotImplementedError |
There was a problem hiding this comment.
🔴 Critical: SecretsStore is documented above as an application extension point, but existing implementations only had to implement load(), store(), and get_instance(). They inherit this NotImplementedError, while the new /secrets dispatch unconditionally calls these per-key methods for CODEX_AUTH_JSON; I reproduced an old-style custom store returning 201 for a normal POST and 500 for the protected POST. Preserve a legacy fallback or explicit capability dispatch for stores that do not implement protected per-key operations (the built-in stores can retain the structural path). Otherwise this needs to be declared as a breaking extension API and rejected clearly at configuration/startup rather than failing user requests at runtime.
Addresses both review findings, each reproduced first. load() took the update lock, which opens <path>.lock with O_CREAT. A readable secrets.json on a read-only volume raised PermissionError where a plain read succeeded, and the exclusive lock serialised every read: four concurrent loads over a 0.4s critical section took 1.62s. #100 needed that lock to snapshot the preserve baseline; this branch deleted the baseline, so it was dead weight. load() is a plain read again, as it was before #100. The /secrets dispatch called the per-key writers for CODEX_AUTH_JSON on any store, but SecretsStore is a documented extension point and third-party implementations only had to provide load/store/get_instance. They inherit NotImplementedError, so a legacy custom store returned 500 for that one name while ordinary secrets returned 201. Gate the dispatch on a supports_protected_credentials capability, mirroring FileStore.supports_locked_update: False on the base class, True on both first-party stores. Withholding protected names from store() and writing them per-key are two halves of one feature, so a store that lacks the second does not get the first and keeps the whole-document path. Both regressions fail against 979b8d5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both review findings confirmed and fixed in 1.
|
|
Closing, evidence-based, same reasoning as #77 (which this stacks on) — not a judgment on the code, which is sound. Two things this PR did were worth doing, and both are now handled elsewhere:
Turning the write guard from a remembered baseline into a structural one (this PR's actual contribution) is good hardening if the write-back design ships — it isn't shipping on current evidence, so there's nothing for it to harden yet. Re-open alongside #77 if that changes. |
OHE-3025 Codex auth.json: production evidence says re-scope OHE-2794 (config + data loss, not credential sync)
Telemetry-first investigation of the Codex Full writeup: enterprise#120. This issue is the OHE-side summary of what the logs actually show and what to do next. HeadlineOf 73 sandboxes that started Limits on all numbers below
New information from the logsPopulation — small feature
Two distinct failure modesA.
B.
Zero-hit signatures, with a control that makes the zeros meaningfulAll of: Control: False leads ruled out
Live cluster findings (read-only kubectl)
Code findings
Recommendations
Correction issued after further checkingPosted at enterprise#120 comment. Two claims revised, one in each direction:
Unchanged: #101's Codex allowlist and #70's rotation premise. sdk#4171 states its own status — "No production incident has been attributed to this race yet" — which the window confirms. AskRe-scope OHE-2794 from a credential-synchronisation problem to a configuration + data-loss problem, and sequence recommendation 1 ahead of the #70 design work. |
1 participant
HUMAN:
Stacked on #100. Implements the follow-up that PR's notes describe as issue 3 — turning the managed-credential write guard from a remembered baseline into a structural one.
AGENT:
Base is
versioned-credential-store(#100), notmain— it deletes machinery that only exists there. Merge #100 first and this retargets tomaincleanly.Design rationale: #77 (comment) (§ "Plan", change A). Parent design: OpenHands/OpenHands#15393 §1.
Why
#100 protects
CODEX_AUTH_JSONfrom a stale whole-document save using a baseline remembered in per-request memory:storepreserves the credential only when the same store instance calledloadfirst and the submitted value equals what it read. That makes durability depend on a caller's in-process history, and two live paths defeat it. Both are now regression tests, and I verified both fail against #100's head:loadhas no baseline, so the value it carries wins and a runtime rotation is undone.FileSecretsStore:preserveisFalsewheneverbaseline is Noneand the payload names the key.SaasSecretsStore: same viahas_baseline.invalidate_legacy_secrets_store(settings_router.py:390) runs insideGET /settings. It resolvesprovider_tokens— which callsload, establishing a baseline — then savesSecrets(provider_tokens=...)with emptycustom_secrets. BaselineR1≠ submittedNone, sopreserveisFalse, the delete is unfiltered, and the credential is removed with nothing re-inserted. On the SaaS pathload_versionedthen raisesKeyError→ 404 → which SDK 1.37.1 latches as a permanentCredentialNeedsReauthentication.Note the inversion in (b): having a baseline is what makes it not preserve. That is the tell that the mechanism is the wrong shape.
Summary
storenow drops protected names from the submitted document and excludes their rows from its delete — unconditionally, on both stores and on both the locked and unlocked file-store paths. Whether the caller loaded first stops mattering, which lets the following go away entirely:_loaded_codex_authand_LoadedCredential(file store)_loaded_codex_auth(SaaS store)preserve/preserve_codexin bothstoreimplementationswith_for_update()Codex row lock in SaaSstoreand its description carry-forward — including the lock-ordering edit fromd5e880415that feat(app-server): sync managed Codex credentials (Phase 1) #77 flagged as its one change with no local test coverageWriting a protected credential is now possible only through
replace_protected_credential/delete_protected_credential. The three/secretsendpoints dispatch to them so a user can still manage their own credential; renaming into or out of a protected name is refused (400) rather than silently breaking an armed binding.What deliberately does not change. The credential stays a
CustomSecretin the same table, with the sameJwtServicerow encryption and the sameSecretStrmodel, andloadstill returns it. So the arming predicate atlive_status_app_conversation_service.py:2430(which compares the request value againstuser_context.get_secrets()), OSS process sandboxes on the legacy path, and flag-off rollback all keep working. No schema change, no migration.One intentional behaviour change beyond the fix: a whole-document save no longer updates a protected entry's description either. It has no authority over that entry at all. The user-facing description edit still works through the per-key writer.
Issue Number
OpenHands/OpenHands#15393
How to Test
OH_PERSISTENCE_DIR=$(mktemp -d) uv run pytest tests/unit/app_server/test_file_secrets_store_versioning.py -q17 passed. Four are new:
test_store_cannot_create_a_protected_credential,test_store_cannot_overwrite_a_rotation_without_a_prior_load,test_whole_document_save_without_custom_secrets_keeps_the_credential,test_protected_delete_removes_value_and_generation. Existing tests that seeded the credential throughstorenow seed through the per-key writer, which is the contract change.The enterprise suite does run locally — #77 and #100 both say it can't, but the only blockers are missing test-only deps in the root
uvvenv:15 passed, including two new SaaS regressions mirroring (a) and (b).
Full suites, both compared against #100 as the baseline:
tests/unit/app_server/TMPDIR, missingservermodule)enterprise/tests/unit/Seven
enterprise/tests/unitfiles still can't be collected locally — all needgoogle-cloud-recaptcha-enterpriseand none touch secrets.Lint: both pre-commit configs pass — root and enterprise (run from
enterprise/).Type
Notes
Still not fixed here, and still listed on #100 — the other two carried-forward issues, because each needs a decision I shouldn't make unilaterally:
file_secrets_store.py—load_versionedstill writes during a GET to mint a generation for a credential that predates this feature. Removing the write means either deriving the bootstrap version from the value (weakens merge gate 3's ABA guarantee for exactly the first rotation, and #15393 explicitly rejected a plaintext digest) or minting the generation at arming time in feat(app-server): sync managed Codex credentials (Phase 1) #77's layer. Worth deciding in the issue.saas_secrets_store.py::_require_organization_idstill raisesKeyErrorfor a revoked org membership → 404 → permanent SDK latch. The correct status is 403, which the SDK can clear on reactivation, but the mapping lives insandbox_router.pyin feat(app-server): sync managed Codex credentials (Phase 1) #77 — so it needs a distinct exception type here plus a router change there.Scope note. The unconditional delete in SaaS
storestill wipes non-protected custom secrets for the shape in (b), andFileSecretsStore._merge_entriesdrops keys absent from the incoming document. That's pre-existing onmain, independent of the credential work, and I left it alone — butinvalidate_legacy_secrets_storedestroying a user's custom secrets on aGETdeserves its own issue.Enterprise server image for this PR: