[feat] Write-only vault secrets (values never readable back by users) - #6164
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds write-only secret storage and redacted API responses. It adds runtime-authenticated plaintext access through ChangesWrite-only secret lifecycle
Runtime credential deployment
Design documentation and fixture scanning
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to The change introduces write-only secret resolution and restores caching, but the current implementation caches decrypted credentials before redaction, creating a direct secret-exposure risk; unresolved tenant-isolation, credential-routing, API-contract, and deployment-startup issues also remain. The PR is not merge-ready until the high-impact security and availability risks are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant AccessRouter
participant sign_secret_token
participant verify_secret_token
participant VaultRouter
AccessRouter->>sign_secret_token: issue run_service token with secret-resolve
sign_secret_token->>verify_secret_token: validate grants claim
verify_secret_token->>VaultRouter: expose verified token_grants
VaultRouter->>VaultRouter: return plaintext only with secret-resolve
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
|
Review requested by @mmabrouk. Generated by OpenAI Codex CLI (gpt-5.6-sol, xhigh reasoning) with full cross-PR context; posted by the orchestrating agent. Findings are being addressed on this branch. Codex review (gpt-5.6-sol, xhigh)Request changes. The primary vault routes redact common value fields, and the grant remains signed, additive, and limited to 15 minutes, but several user-facing paths still expose write-only values. The tightening transition also has database and Redis races that can make or serve a secret as readable again, so this is not safe to enable. Findings
Test gaps
Nits
Good: the non-mutating redaction helper, explicit-create precedence, and separation between additive grants and confining scopes are all sound foundations. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Review round addressed in
Tests: api unit 2,940 passed / 0 failed; real-token route tests added. Known gaps stated in the PR body (no live-Postgres concurrency test in the unit environment; legacy VaultMiddleware still logs display names, never values). (This comment restores a summary that was accidentally overwritten by a review-trigger edit.) |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/oss/src/utils/caching.py (1)
43-59: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
invalidate_cachecannot reachfull_project_idkeys.
_pack,pack,get_cache, andset_cacheacceptfull_project_id, butinvalidate_cachedoes not. Its pattern branch always truncates the project id, socache:p:<last12>:u:...:*never matches a key written ascache:p:<full-uuid>:....The vault router still calls
invalidate_cache(project_id=...)on every secret write. That call is now a no-op for thelist_secretsandlist_secrets_generationnamespaces, and only the generation bump keeps readers correct. Add the parameter so the invalidation API matches the read and write APIs, and so a future caller does not assume invalidation works.🛠️ Proposed parameter addition
async def invalidate_cache( namespace: Optional[str] = None, key: Optional[Union[str, dict]] = None, project_id: Optional[str] = None, user_id: Optional[str] = None, full_project_id: bool = False, ) -> Optional[bool]: ... cache_name = _pack( namespace=namespace, key=key, project_id=project_id, user_id=user_id, pattern=True, full_project_id=full_project_id, )api/oss/src/core/webhooks/service.py (1)
149-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA generated webhook signing secret becomes unrecoverable once the write-only gate is on.
Line 149 generates the signing secret when the caller supplies none. That secret is created without an explicit
write_only, so it inheritsAGENTA_VAULT_WRITE_ONLY_DEFAULT. Line 183 then redacts the create echo. When the gate is enabled, the create response returnssecret=Noneand no later response returns the value, so the subscriber can never obtain the key needed to verify HMAC signatures.A webhook signing secret is a shared verification key, not a write-only credential. Set
write_only=Falseon this create, or return the generated plaintext on the create response only.🛠️ Proposed fix
secret_dto = await self.vault_service.create_secret( project_id=project_id, # create_secret_dto=CreateSecretDTO( + write_only=False, header={ "name": f"webhook-{subscription.name or 'subscription'}", "description": "Webhook signing secret", },Also applies to: 179-184
🧹 Nitpick comments (5)
api/oss/tests/pytest/unit/secrets/test_write_only.py (1)
36-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree copies of the in-memory secrets DAO have already drifted. Each file defines its own
_FakeSecretsDAO, and the copies differ in stored shape and surface.createstoresdataas a plain dict frommodel_dump(exclude_none=True), whileupdatestores the DTO object taken from the request and aliases it. The three suites therefore assert against three different approximations of persistence. Extract one shared double into a test-utils module and normalizedatathe same way on create and update.
api/oss/tests/pytest/unit/secrets/test_write_only.py#L36-L82: move this class into a shared helper module and import it here; storedatawith the same normalization on bothcreateandupdate, and deep-copy the incoming DTO data to avoid aliasing the request object.api/oss/tests/pytest/unit/vault/test_write_only_routes.py#L31-L84: delete the local copy and import the shared double; keep the string-keyed record map as a thin adapter if the route tests need it.api/oss/tests/pytest/unit/webhooks/test_write_only_outward.py#L27-L62: delete the local copy and import the shared double, which also restores theget_by_sluganddeletemethods missing here.sdks/python/oss/tests/pytest/unit/agents/connections/test_credentials_parity.py (1)
34-41: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd API-side parity coverage for
SecretKind. A newSecretKindvalue can bypass write-only redaction becauseredact_secret_responserelies onPRIMARY_CREDENTIAL_FIELDS. Assert that{kind.value for kind in SecretKind}equalsset(PRIMARY_CREDENTIAL_FIELDS).api/oss/src/core/secrets/redaction.py (2)
59-87: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGuard against unmapped secret kinds. All current
SecretKindvalues are mapped. If a new kind is added without a mapping,redact_secret_responseleaves its credential value unchanged while settinghas_key=False; raise for unmapped kinds or add an exhaustive coverage check.
17-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winImport credential metadata from its owner module.
CREDENTIAL_EXTRAS_KEYSandPRIMARY_CREDENTIAL_FIELDSare defined inagenta.sdk.agents.connections.credentials, but the API currently imports them throughredaction.py, making that module an implicit re-export surface. Import the constants directly in bothredaction.pyandservices.py, and confirmagentais declared as an API runtime dependency because this import executes during API startup.api/oss/src/dbs/postgres/secrets/mappings.py (1)
75-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the same
exclude_nonepolicy on create and update.
map_secrets_dto_to_dbeserializes withexclude_none=True, but Line 81 serializes the update payload without it. An update therefore persists explicit nulls for omitted optional fields, such asmodels,harnesses, andmodel_keys, while a create omits those keys. The stored shape then depends on which path wrote the row.♻️ Proposed alignment
if key == "data" and hasattr(secrets_dbe, key): secrets_dbe.data = _data_payload( - update_secret_dto.secret.data.model_dump(), + update_secret_dto.secret.data.model_dump(exclude_none=True), write_only=write_only, )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d2ed5c5b-207b-4b48-875a-bd10c1dac5c7
📒 Files selected for processing (29)
api/ee/src/core/organizations/service.pyapi/ee/tests/pytest/unit/test_write_only_provider_settings.pyapi/oss/src/apis/fastapi/access/router.pyapi/oss/src/apis/fastapi/vault/router.pyapi/oss/src/core/secrets/dtos.pyapi/oss/src/core/secrets/redaction.pyapi/oss/src/core/secrets/services.pyapi/oss/src/core/webhooks/service.pyapi/oss/src/core/workflows/service.pyapi/oss/src/dbs/postgres/secrets/dao.pyapi/oss/src/dbs/postgres/secrets/mappings.pyapi/oss/src/middlewares/auth.pyapi/oss/src/utils/caching.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/access/test_grant_exchange.pyapi/oss/tests/pytest/unit/middlewares/test_auth_grants.pyapi/oss/tests/pytest/unit/secrets/test_write_only.pyapi/oss/tests/pytest/unit/utils/test_cache_key_tenancy.pyapi/oss/tests/pytest/unit/vault/test_write_only_routes.pyapi/oss/tests/pytest/unit/webhooks/test_write_only_outward.pydocs/design/write-only-secrets/README.mdsdks/python/agenta/sdk/agents/connections/__init__.pysdks/python/agenta/sdk/agents/connections/credentials.pysdks/python/agenta/sdk/agents/connections/errors.pysdks/python/agenta/sdk/agents/platform/connections.pysdks/python/agenta/sdk/agents/platform/secrets.pysdks/python/agenta/sdk/middlewares/running/vault.pysdks/python/oss/tests/pytest/unit/agents/connections/test_credentials_parity.pysdks/python/oss/tests/pytest/unit/agents/platform/test_write_only_secrets.py
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
The row is Agenta's from the moment it exists: `managed_by="starter-credits-bridge"` on the create refuses user deletes and re-credentialing, and `write_only=True` keeps the proxy virtual key out of every user-facing read. Setting both on CREATE (rather than tightening afterwards) leaves no window in which the seeded connection is readable or removable. Every write this component makes to its own row now passes `allow_managed=True`, the in-process owner's key past the managed guard: the grant-record upsert and the finalize that writes the minted key. There is no delete path in the bridge, so nothing else needs the flag. Also documents that the mint-policy cache key is deliberately global, unlike the per-org feature-flag cache above it: the policy is one program-wide payload, identical for every organization. Stacked on managed-secrets (#6165) and write-only-secrets-api (#6164), which provide the two flags.
mmabrouk
left a comment
There was a problem hiding this comment.
Reviewer guide for this PR. The change is wide but has one spine: values are stripped at the response boundary, never below it, and exactly one kind of caller gets plaintext. The inline notes follow that spine in order - where redaction happens, what the value fields had to become to allow a value-less shape, when an omitted value means keep-the-stored-one, who is allowed to read plaintext, and the three places that could leak around the boundary (the list cache, the one-way flag under concurrency, and the webhook echo). Note the base: this is stacked on #6135 and must be re-targeted to main once that merges. The default is env-gated off, so merging changes nothing for anyone until the flag flips.
The row is Agenta's from the moment it exists: `managed_by="starter-credits-bridge"` on the create refuses user deletes and re-credentialing, and `write_only=True` keeps the proxy virtual key out of every user-facing read. Setting both on CREATE (rather than tightening afterwards) leaves no window in which the seeded connection is readable or removable. Every write this component makes to its own row now passes `allow_managed=True`, the in-process owner's key past the managed guard: the grant-record upsert and the finalize that writes the minted key. There is no delete path in the bridge, so nothing else needs the flag. Also documents that the mint-policy cache key is deliberately global, unlike the per-org feature-flag cache above it: the policy is one program-wide payload, identical for every organization. Stacked on managed-secrets (#6165) and write-only-secrets-api (#6164), which provide the two flags.
The row is Agenta's from the moment it exists: `managed_by="starter-credits-bridge"` on the create refuses user deletes and re-credentialing, and `write_only=True` keeps the proxy virtual key out of every user-facing read. Setting both on CREATE (rather than tightening afterwards) leaves no window in which the seeded connection is readable or removable. Every write this component makes to its own row now passes `allow_managed=True`, the in-process owner's key past the managed guard: the grant-record upsert and the finalize that writes the minted key. There is no delete path in the bridge, so nothing else needs the flag. Also documents that the mint-policy cache key is deliberately global, unlike the per-org feature-flag cache above it: the policy is one program-wide payload, identical for every organization. Stacked on managed-secrets (#6165) and write-only-secrets-api (#6164), which provide the two flags.
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
8bbd078 to
3ef279c
Compare
The row is Agenta's from the moment it exists: `managed_by="starter-credits-bridge"` on the create refuses user deletes and re-credentialing, and `write_only=True` keeps the proxy virtual key out of every user-facing read. Setting both on CREATE (rather than tightening afterwards) leaves no window in which the seeded connection is readable or removable. Every write this component makes to its own row now passes `allow_managed=True`, the in-process owner's key past the managed guard: the grant-record upsert and the finalize that writes the minted key. There is no delete path in the bridge, so nothing else needs the flag. Also documents that the mint-policy cache key is deliberately global, unlike the per-org feature-flag cache above it: the policy is one program-wide payload, identical for every organization. Stacked on managed-secrets (#6165) and write-only-secrets-api (#6164), which provide the two flags.
The list is small and only the settings page reads it, and the runtime path already went straight to the database. Caching it bought little while costing a whole class of question — what a shared Redis entry holds, and whether a stale reader can repopulate it with plaintext after a tighten — which the generation counter and the full-project-id cache keys existed only to answer. The route now reads the database on every request and redacts at the response boundary, so what a caller sees is what the row says. The per-project sweep each secret write already fired is untouched: it predates write-only secrets and serves the other namespaces.
…ite-only secret The error told the user to provide the provider key in the environment, but nothing read it: an agenta-mode connection resolved from the vault alone, so a standalone run against a write-only secret failed even with OPENAI_API_KEY exported. Resolution now reads the variable the harness itself would use for that connection — the provider family's key, or Bedrock's and Azure's own channels, so one service's credential is never sent to another — and raises only when that variable is empty too. The error text says what the resolver already tried.
…ve grant The exchange attached the grant whenever a caller asked with action=run_service, and any member who may run a service can ask: they could call it with their own session or ApiKey, take the returned credential to the vault routes, and read every write-only value in plaintext. That is the whole guarantee, self-serve. The exchange now only carries forward a grant the caller already holds on a verified Secret token, and never creates one. Creation stays where a run actually starts, in-process at the invoke and inspect hops, so the credential still travels with the run: the workflow service and the runner both re-exchange the granted token they were handed, and refresh keeps working with no new credential to distribute and no deployment change.
Keep-on-omit filled an omitted credential from a snapshot the service read before the DAO took its row lock, so a rotation that committed in between was silently undone: the update wrote the older value back over the newer one, and nothing reported it. The DAO now resolves the carry-over inside the locked transaction, against the row as it actually stands — the shape GitDAO.commit_revision already uses for the same reason. The identity check travels with it, because deciding same-identity against a stale row is how a credential crosses identities. The fake DAOs call the resolver at the same point, so keep-on-omit stays exercised rather than skipped, and a new test rotates the stored row inside that window and pins that the value carried over is the rotated one.
… use The environment fallback for a write-only connection read one variable per candidate, so a Bedrock connection could only be credentialed by AWS_BEARER_TOKEN_BEDROCK and a Vertex one not at all — even though the vault path accepts an AWS key pair or service-account material for exactly those connections. The fallback now offers the same channels the stored credential could have used, and a channel counts only when every variable in it is set: half an AWS key pair authenticates nothing, and passing it on would fail at the provider with a misleading error instead of here with an actionable one.
The SSO branch checked that client_secret was PRESENT, not that it held a value, so a create carrying an explicit null stored an SSO record with no credential — while the webhook and custom-secret branches beside it already checked the value. It now checks the value on the create path only: omission still means "keep the stored one" on update, which is what redacted responses and the edit form depend on.
The fixtures were rewritten to obviously-fake, digit-free strings so the scanner has nothing to find going forward. Four occurrences remain inside commits whose amend could not be replayed, and the scan reads history, not the tree. Each is a unit-test constant handed to a fake DAO; no real credential was ever involved. Fingerprints are anchored to their commit, so they need regenerating if either lane's history is rewritten again.
Redaction was applied inside the one helper every caller used, so the connection test and the edit path read a write-only provider's client secret as empty. A redacted read there does not hide a value from anyone — it tests the provider without its secret, then writes is_valid false and is_active false, taking a working provider out of the login screen. There are now two resolvers, the split the webhooks service already uses for signing secrets: the plain one stays plaintext for the internal callers, and an outward one shapes responses and drops the secret once the record is write-only. Both are documented by what the caller does with the value, not by where it is called from.
Keep-on-omit compared kind and provider family but not the stored format, so a text-to-json update that omitted its content carried the stored string into the json shape. The validators could not catch it: they run when the payload is built, before the carry-over fills the value in, so what they saw was a value-less shape and what reached the row was a json secret holding a string. A format change is now an identity change and requires an explicit value, and the merged payload is re-validated under the lock before it is persisted, so nothing is stored that a create of the same shape would have refused. Also moves the per-kind primary credential field out of the SDK classifier and onto the API side: it covers kinds the SDK never resolves (SSO providers, webhook signing secrets) and no SDK code reads it. The extras vocabulary stays shared, which is where drift would actually hurt.
…e, not to whoever asks Closing the self-serve grant broke every agent run, and the reason is the shape of the product's path: the playground and the gate post straight to the workflow service with the user's own ApiKey, so the service exchanges THAT credential and the API's in-process mint never runs. Carry-forward alone therefore had nothing to carry, and runs got the redacted shape. The exchange now mints the grant when the caller proves it is the platform runtime and otherwise carries forward what the caller already holds. The proof is a secret only the backend has, sent on the internal hop and compared in constant time, because that route is publicly reachable and the user's token cannot say who is asking. It resolves from AGENTA_SERVICES_INTERNAL_KEY, falling back to AGENTA_AUTH_KEY, which the services container already receives through the same env file as the API — so deployments keep working unchanged, and a dedicated value narrows what one leaked secret can do. The runner keeps carry-forward and is given nothing new; the key never reaches it or a sandbox, and is never logged.
…untime The runtime key falls back to AGENTA_AUTH_KEY, whose unconfigured value is the string 'replace-me' committed in every example env file — so a deployment that changed neither would have handed a run credential to anyone who sent it. Both sides now treat the placeholder as no key at all: such a deployment issues no grant, which costs it only the ability to run against write-only secrets (off by default) and never gives that ability to a stranger. The variable is documented in the example env files, and the design note now says who may hold the grant and why the exchange cannot decide it from the requested action.
…er's key The bug that broke every run was invisible to unit tests because they exercised the hop that already had a granted token, while the product uses the hop that never did. This drives the real agent app with the exchange stubbed and asserts the request carries both the caller's own credential and the platform's runtime key — and that a service configured without one sends no header at all rather than an empty value.
…aming the provider key The placeholder this refuses is the shipped default in every example env file, so a deployment that never set AGENTA_AUTH_KEY silently loses runs against write-only connections — and what it sees is the SDK telling it to provide OPENAI_API_KEY, which is true for a standalone run and useless here. The service now says it once, at the point of use, and names the variable to set. Live QA hit exactly this and read it as a regression, which is the cost of a failure that points somewhere else.
The fixtures were rewritten to digit-free names, but these two spellings survive in commits whose amend could not be replayed, and both scans read history rather than the tree. A fingerprint names the commit its finding was seen in, so it goes stale every time a lane below is rebased — which happened twice while landing this stack. Exempting the values is stable, and it follows what this file already says: exempt the VALUE, never the path, so a real key added to a fixture would still be seen.
A deployment that turned write-only on, or enabled a component that seeds write-only rows, cannot read those secrets at all without a platform runtime key — and the failure it gets says to provide a provider key, which is right for a standalone run and useless here. The API now says it once at boot, naming the variable and the consequence, next to the other startup validations. The placeholder counts as unset, since it is what the example env files ship.
The warning read env.starter_credits_bridge directly, but that config is an EE addition that this branch does not carry — so on a build without it the API would have raised AttributeError during startup validation, before serving anything. It now asks for the attribute rather than assuming it, and the case that needs the bridge is tested where the bridge exists. My own CI caught it because the test referenced the same missing attribute; the production path had the same bug.
5f3601b to
52d3ca5
Compare
Context
Vault values must be usable by an authorized workload without being readable back through the user API. The earlier version removed the list cache, exposed key-specific status fields, allowed visibility to change on update, and could fall back to the administrator key for runtime proof. Those choices added latency and blurred the security boundary.
Changes
This PR defines the production write-only contract.
Before:
{"write_only": true, "has_key": true, "key_preview": "sk-****9Qa"}After:
{"write_only": true, "value_status": {"configured": true, "preview": "sk-****9Qa"}}The Vault list cache is restored with its existing TTL, namespace, invalidation, and shortened UUID packing. Redis stores the canonical trusted DTO. Every caller is projected after the cache read, so an ordinary caller receives redacted data while a verified runtime grant receives plaintext from the same entry.
write_onlyis selected at creation and cannot change on update. Omitted values keep the value from the row loaded under the DAO lock. Existing rows without the stored field remain readable. SSO and webhook creation explicitly setwrite_only=False, so their edit, login, test, and signature flows do not change.The short-lived
secret-resolvegrant remains project-scoped and allowlisted.AGENTA_SERVICES_INTERNAL_KEYis the only proof accepted on the API-to-Services exchange. It has noAGENTA_AUTH_KEYfallback, never reaches a runner or sandbox, and missing or placeholder configuration now fails API startup. Standalone SDK runs keep the narrow provider-specific environment fallback.Storage remains in the existing encrypted JSON payload. No database migration or feature flag is introduced.
The Python SDK now reads
value_status.configuredthrough one shared helper in all three redaction consumers. There is no productionhas_keyfallback. Updates use one strict contract: omission keeps the stored credential, while an explicit blank provider credential is invalid. The frontend in #6174 omits untouched credentials, and the backend and frontend ship in the same release, so no transition compatibility path is needed.Tests / notes
docs/design/write-only-secrets/implementation-report.md.docs/design/write-only-secrets/qa.md.What to QA
AGENTA_SERVICES_INTERNAL_KEY, then withreplace-me. Both starts must fail and name the variable.Based on
release/v0.114.0.