feat(api): provider accounts + encrypted credential vault (#18) - #134
feat(api): provider accounts + encrypted credential vault (#18)#134leon0399 wants to merge 6 commits into
Conversation
v0.3 opens: the foundational identity/org model everything downstream (policy engine #45, config resolver #46, projects) checks against. - org_units: arbitrary nesting with an ID-based materialized path (deviation from the issue's slug example, deliberately: ids are rename-stable, so only moves rewrite paths). The ancestor set is embedded in the path itself — every RLS policy resolves "role on unit or any ancestor" with ONE memberships scan via string_to_array(path), avoiding the self-referential org_units join Postgres rejects as infinite policy recursion. - memberships: full SPEC §7.3 role enum per (user, unit), unique pair. Inherited roles are COMPUTED along the ancestor path (nearest node wins — a subtree can demote as well as promote), not materialized (no inherited_from_id fan-out to maintain on moves). - external_identities: canonical (provider, external_subject) → user map for channels (SPEC §19.2). Research across open-webui / opencode / hermes-agent found no OSS precedent for nested groups with per-membership roles or cross-channel identity — original design, closest in spirit to open-webui's group_member join table. - RLS ENABLE + FORCE on all three (migration 0015; FORCE hand-appended like 0004/0009/0011 — gotcha list updated). Creator-bootstrap policy solves the fresh-root chicken-egg (root creator self-grants owner in the same tx). Fail-closed everywhere: memberships are own-rows-only, member lists arrive with the admin surface. - IdentityService (providers-only module — zero reachable HTTP surface until the admin API slice): root/child creation, move, effective-role resolution. Two landmines found by the integration suite, fixed + documented: - Postgres applies SELECT policies to INSERT…RETURNING rows: a granted membership belongs to the GRANTEE, invisible to the granter under the fail-closed select policy — grants no longer RETURNING. - substring(x from $n) with an untyped bind parameter resolves to the POSIX-REGEX form and silently yields NULL; the RLS WITH CHECK rejected the corrupted row before NOT NULL ever saw it. Moves use substr(text, int). Verified: 16 unit tests (path utils, nearest-wins resolution), full rls-test.sh harness green — RLS integration (FORCE proofs, cross-tenant denial, escalation denial, forged-path denial, subtree visibility after move) + queue + auth e2e on a throwaway non-superuser-owner Postgres. Lint, tsc, build green. Closes #44 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Multi-user governance (nested org units, memberships, roles) was fully built
at the service + FORCE-RLS layer but had ZERO HTTP surface — an operator
couldn't create an org or add a member except by hand-editing rows.
New IdentityController: POST /org-units (create root — creator becomes owner
in one tx), POST /org-units/:id/children (child; RLS owner/admin on an
ancestor), GET /org-units (the caller's visible units), POST
/org-units/:id/memberships (grant). Owner-scoped by the authenticated
identity + the existing FORCE-RLS policies.
Escalation is closed at the DTO: the grant role enum is { admin, member }
only — owner is assigned solely at creation, so the API can never mint or
escalate to owner; a re-grant conflicts → 409, a garbage userId (FK) → 404.
A single-reviewer round verified all five security claims and caught a real
P1 (CreateOrgUnitDto.type needed @IsOptional(), else omitting it 400s).
Member REVOKE + ROSTER are deferred together: the harness proved an admin
can't remove/see ANOTHER member's row (Postgres applies the own-rows SELECT
policy to a DELETE's targets), so both need the same recursion-safe SECURITY
DEFINER member-visibility change — a coherent follow-up.
Verified: 32 identity specs pass (RLS integration: create → creator-is-owner
+ visible, non-admin can't grant, cross-tenant grant denied, 409; DTO
escalation-guard units), tsc/lint/build clean; api-only.
Closes #44
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Layered effective-config resolution (SPEC §6.3-§6.4) with per-leaf provenance, snapshotted onto every run: - `configs` table: one versioned document per scope (org_unit / user / chat), unique per (scope_type, scope_id), version bumped on every write. RLS ENABLE+FORCE (migration 0016): own user scope, owned chat scope, org members read / owner-admin write via the #44 path trick. The instance layer stays env-derived (no row nobody could write); org_units.settings (added in #44, unreleased, no consumer) is dropped in favor of the uniform table — one source of truth (SPEC §7.2/§25.1 updated to match). - Merge engine: inheritance order instance → user → chat (org-unit layers slot in once chats/projects attach to org units, v0.5), later wins, objects deep-merge, arrays REPLACE whole — fail-closed by design (additive arrays are how a lower scope could smuggle a capability past a higher one; reference platforms concat+dedupe, we deliberately do not). Every resolution records per-leaf provenance (scope + version) plus the full consulted-layer chain (SPEC §6.4 config_version_ids). - Run snapshot: runs.config_snapshot is resolved in the SAME transaction that persists the message and run; execution reads only the snapshot, never live config, so a mid-flight config change can never re-configure a live run. Downstream consumers (the run-token budget, the compaction threshold) read the snapshot via snapshotMaxOutputTokens/snapshotCompactionThreshold — wiring the actual enforcement into run-execution.service.ts is those slices' job, not this one's. - Explain endpoint: GET /api/v1/chats/:id/effective-config returns effective + provenance + layers (404 cross-tenant, no existence leak). Reference research: claude-code's getSettingsWithSources() is the same idea; no comp persists a resolved snapshot at all — the durable per-run snapshot is deliberately novel (SPEC §6.4). - `configs_select`'s org-unit read arm ships with the ancestor- governance fix from the start (a config binds you whether it sits on your own unit's ancestor path OR on a unit whose subtree you belong to) — a too-narrow version of this exact policy shape shipped once elsewhere in this scope model and silently failed to bind descendant members to an ancestor-scoped row; proven here by a dedicated RLS integration test before it could recur. - Policy deny-stripping composes when #45 lands (the issue's third design step) — resolver and snapshot shapes already accommodate it. Verified on the isolated throwaway-Postgres harness (rls-test.sh): 5 integration suites incl. 6 new configs-RLS tests (scope isolation, cross-tenant denial, ancestor-governance read, write-denied-to-members, version increments) + 5 e2e suites incl. two new tests — instance-layer snapshot provenance, and a user-scope config overriding the instance env layer with provenance on the run row, the explain endpoint, and its 404 cross-tenant. 159 unit tests, lint, tsc green. Refs #46 — the deny-stripping acceptance step arrives with #45; the run-token-budget and compaction-threshold *consumers* of the snapshot are separate slices, not part of this one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
v0.3's governance spine, completing the milestone's core alongside #44 and #46: - `policies` rows (SPEC §7.4): effect (allow|deny), dotted action with '*' wildcard support, optional resource_type/resource_id, jsonb conditions (equality vs check context — ALL must hold, absent keys fail closed), and a SPEC §7.5 approval level on allows. `version` bumps on every write. `never_allowed` is deliberately not an approval value — that is what a deny IS. - Pure evaluator (policy-eval.ts): DEFAULT DENY ("policy before capability"); DENY OVERRIDES ALLOW regardless of specificity; approval = the STRICTEST level demanded by any matching allow — a deeper auto_allow deliberately cannot soften an org's always_ask (the approval analogue of deny-overrides-allow). Reference research: Claude Code's strict deny>ask>allow type precedence is the model; opencode's last-match-wins is the documented multi-author footgun we avoid; open-webui is allow-only and cannot express exclusions. `requiresHumanApproval` is the single source of truth for the approval-demanded threshold (auto_allow_* never asks; ask_*/ always_ask/admin_only do) — exported for the tool-calling loop's gate to consume, not wired into it here. - PolicyService.check(): collects policies across the org-path → user → chat scope chain and appends a `policy_decisions` audit row IN THE SAME TRANSACTION — an unauditable decision does not happen. The decision records effect, approval, and every matched policy id + version (deterministic and auditable after policies change). - Providers-only module, no HTTP surface. Consumers: the config resolver's deny-stripping step (#46) and the tool/connector/model gates of v0.4+ — neither wired in this slice. `policies_select`'s org-unit read arm ships with the same both-ways ancestor-governance shape as `configs_select` (#46) from the start — a member of a CHILD unit can read a policy set on an ANCESTOR unit, proven by a dedicated RLS regression test — rather than replaying the narrower-then-fixed history the original implementation went through. Verified: 213 api unit tests (23 in policy-eval.spec.ts, incl. new requiresHumanApproval coverage), full isolated rls-test.sh harness green (6 integration suites / 51 tests incl. policies-rls's 8, queue substrate, auth e2e), lint/tsc green. `drizzle-kit check` passes. Closes #45 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BYOK foundation (SPEC §14.1-§14.3), carved from experiment/overnight-loop-3 onto the identity + config-resolver + policy-engine stack: - provider_accounts: owner scope (user now, org_unit enum-ready), provider_type (openai_compatible first; rest of the adapter vocabulary enum-reserved so a new adapter is never an enum migration), auth_mode, base_url, default_model, models_cache (null until catalog sync #84), enabled. RLS FORCE with the #45 read/write split (subtree members read org accounts; owner/admin write). - credentials: AES-256-GCM envelopes under a VERSIONED master-key ring (CREDENTIAL_MASTER_KEYS="1:<b64>,2:<b64>"). New secrets seal under the highest version; decryption selects by the row's key_version — rotation is append-and-re-encrypt-lazily, never a flag day. The GCM AAD binds each ciphertext to (provider_account_id, secret_type): a payload copied onto another row fails authentication even with raw DB write access. Credentials are never more visible than their parent account (RLS via the FK), and secret material never leaves apps/api. - SecretString (lifted from opencode's Redacted<T>): decrypted keys redact through String()/JSON/inspect/template literals; only an explicit .reveal() at the model-client factory exposes the value. - Resolution (SPEC §14.3, wired into ModelsService): user BYOK account first, instance env key second, 402 otherwise. The instance boots with no provider AND with no vault key ring (BYOK then fails closed with a clear 400). - HTTP: GET/POST/DELETE /api/v1/provider-accounts — the API key is write-only (hidden from OpenAPI, absent from every response), global auth guard, cross-tenant 404. Review fix on top of the carved commit: resolveUserCredential's own comment promised "fail closed and logged" for a tampered/corrupted credential, but the decrypt call had no try/catch — a CredentialCryptoError would propagate unhandled (500 today; a permanently wedged worker job retrying forever against the same corrupt row once execution moves fully to the worker). Wrapped it: a decrypt failure now logs a warning and returns null so resolution falls through to the instance env key, with a new integration test proving the fail-closed path end-to-end. Also dropped a dead RUN_EXECUTION_MODE=inline env toggle from the e2e spec — inline execution mode no longer exists on this base. Reference comparison (opencode, open-webui): opencode's Redacted<T> is a type-level redaction wrapper only, no encryption at rest — confirmed no cipher/encrypt call anywhere in packages/llm/src/route/auth.ts. Open-webui encrypts with Fernet only for OAuth sessions and tool "valves" (single static key, no versioning — confirmed in utils/valves.py/models/oauth_sessions.py); actual model-provider keys (OPENAI_API_KEYS) are an instance-wide, unencrypted, semicolon-joined env list, and even per-user API auth keys are stored as plaintext columns (models/users.py). Neither comp does per-user BYOK with envelope encryption or key rotation — this is ahead of both on that axis. Migration 0018 regenerated fresh via drizzle-kit against this base (FORCE hand-appended per the established gotcha pattern; AGENTS.md updated). Closes #18 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request implements the Bring Your Own Key (BYOK) provider accounts feature and an encrypted credential vault (#18). It introduces the provider_accounts and credentials tables with forced Row Level Security (RLS) to ensure strict tenant isolation. Credentials are encrypted at rest using AES-256-GCM under a versioned master-key ring (CREDENTIAL_MASTER_KEYS) and protected by a SecretString wrapper to prevent accidental exposure. The reviewer's feedback focuses on enhancing the security and robustness of this implementation. Key recommendations include hashing secrets with SHA-256 before performing a timing-safe comparison to prevent length-based timing attacks, defensively trimming whitespace during configuration parsing and API key ingestion, and debouncing lastUsedAt database updates to avoid performance bottlenecks under high load.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Addresses gemini-code-assist review findings on the provider-vault PR:
- SecretString.equals(): hash both sides with SHA-256 before
timingSafeEqual instead of an `a.length === b.length` short-circuit —
the length check itself leaked whether two secrets were the same byte
length via comparison timing, before the constant-time compare ever
ran. Both inputs are now fixed-length digests, so no length signal
leaks regardless of the underlying secrets' lengths. (No production
caller exists for .equals() yet, but the vault's own crypto bar is
AEAD + versioned rotation — the comparison primitive should hold to
the same standard before something starts calling it.)
- ProvidersService.resolveUserCredential: debounce the last_used_at
write to once per 60s per credential instead of once per model
resolution — this runs on every chat turn, and last_used_at has no
reader yet (pure observability), so sub-minute precision buys nothing
against a write on every single turn.
- CreateProviderAccountDto consumption: trim the API key before sealing
it into SecretString — a key copy-pasted with a trailing newline or
space would otherwise seal successfully and then fail every real
provider call with an opaque auth error.
Verified and NOT changed: parseMasterKeyRing's whitespace-robustness
suggestion. Buffer.from(str, 'base64') already ignores embedded
whitespace and Number() already trims — confirmed directly (Buffer.from(
' '+key.toString('base64'), 'base64').equals(key) === true; Number(' 2')
=== 2). The suggested entry/version/key .trim() calls would be no-ops
against the exact scenarios cited (spaces around colons/commas); left
unchanged and replied on-thread with this evidence.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5a62269 to
c4cbd76
Compare
What
Opens v0.4 (BYOK, SPEC §14.1-§14.3) with the provider-vault core, stacked on
stack/split-policy-engine(identity + config-resolver + policy engine).provider_accounts: owner scope (usernow,org_unitenum-ready),provider_type(openai_compatiblefirst — the rest of the SPEC §14.1 adapter vocabulary is enum-reserved so a new adapter is never an enum migration),auth_mode,base_url,default_model,models_cache(null until catalog sync feat(api): OpenRouter account and model catalog sync #84),enabled.credentials: AES-256-GCM envelopes under a versioned master-key ring (CREDENTIAL_MASTER_KEYS="1:<b64>,2:<b64>"). New secrets seal under the highest version; decryption selects the key by the row'skey_version— rotation is append-a-version-and-re-encrypt-lazily, never a flag day. The GCM AAD binds each ciphertext to(provider_account_id, secret_type), so a payload copied onto another row fails authentication even with raw DB write access.SecretString(lifted from opencode'sRedacted<T>): a decrypted key redacts throughString()/JSON.stringify/template literals/util.inspect; only an explicit.reveal()at the model-client factory exposes the value.ModelsService.resolveModelCredential: the user's own enabled BYOK account first, then the instance env key, else a 402. The instance boots fine with no provider and no vault key ring — BYOK writes then fail closed with a clear 400.GET/POST/DELETE /api/v1/provider-accounts— the API key is accepted write-only (@ApiHideProperty, absent from every response and fromopenapi.json), global auth guard, cross-tenant access 404s.Closes #18.
Out of scope (sibling PR, stacked after this one): native OpenRouter provider, the web settings page,
GET /api/v1/modelsreal model selection, model-catalog metadata fixes, and the model-visibility allowlist — all of those build on this vault core but are carved separately.Security & tenancy
provider_accountsandcredentials, verified by a 9-test integration suite (providers-rls.integration.spec.ts): RLS enabled+forced check, encrypted-at-rest proof (ciphertext never contains the plaintext key), successful resolution (byok source,last_used_atstamped), cross-tenant invisibility of both tables plus a denied cross-tenant credential attach (INSERT ... VALUES (<other user's account>, ...)rejected by RLS/FK), a disabled account failing closed, a tampered/corrupted credential failing closed (added in this review pass — see below), and a vault-off instance failing closed on both write and read.@ApiHideProperty()-hidden fromopenapi.json(regenerated, checked — zeroapiKeystring leaks into the spec beyond the unrelated"type": "apiKey"auth-scheme literal) and absent from every response DTO (ProviderAccountResponseis an explicit allowlist with no secret-derived field); the e2e suite asserts the literal API key string never appears in any response body or body-as-text.(provider_account_id, secret_type)so ciphertexts can't be replayed across rows, versioned key ring so rotation never bricks existing rows, and a missing/malformedCREDENTIAL_MASTER_KEYSfails closed (parseMasterKeyRingreturnsnull→ the service refuses BYOK writes with a 400, never a plaintext fallback). 17 unit tests cover roundtrip, rotation, a removed key version, tampered-ciphertext detection, and cross-account AAD-replay rejection.resolveUserCredential's own comment claimed failures "fail closed... and are logged", but thedecryptCredentialcall had notry/catch— aCredentialCryptoError(tampered payload, or a rotated-away key version) would propagate unhandled instead of failing closed for that one account. Wrapped it so a decrypt failure now logs a warning and returnsnull, falling through to the instance env key exactly like a disabled account does. New integration test proves this end-to-end (confirmed the fail-closed log line fires and the suite still passes).Review findings & refactors
CredentialCryptoErroron a tampered/corrupted credential. Fixed with a scoped try/catch + a new integration test.process.env.RUN_EXECUTION_MODE = 'inline'toggle from the e2e spec — inline execution mode doesn't exist on this base (worker mode is the only path); this carve artifact predated the master-side removal. Verified via repo-wide sweep that no other stale references, pre-reorg import paths, or leftover conflict markers exist in the slice.AGENTS.md: addedproviders/to the module list + boundary-rule sentence (models/consumes it viaProvidersService.resolveUserCredential), added migration0018to both the "don't hand-edit" exception list and the hand-authored-FORCE gotcha description.drizzle-kit generateagainst this branch's actual cumulative schema (next number is0018on this base, not the original branch's0021/0022) — SQL/snapshot/policies are byte-identical in shape to the original commit's migration, just renumbered;FORCE ROW LEVEL SECURITYhand-appended for both new tables per the established pattern.openapi.jsonregenerated viapnpm --filter api build, not hand-merged (+201 lines, matches the original commit's diff size).Reference comparison
Verified directly against the cached checkouts (not just citing the original branch's research):
packages/llm/src/route/auth.ts,packages/opencode/src/provider/auth.ts): usesRedacted<T>(Effect's redaction wrapper) pervasively across its auth/provider code — confirms theSecretStringidea is a faithful port of something real and good. Butauth.tshas zeroencrypt/cipher/AESreferences —Redacted<T>is a type-level guard against accidental logging, not encryption at rest. No key rotation concept exists because there's nothing to rotate.backend/open_webui/utils/valves.py,models/oauth_sessions.py,models/users.py,config.py): Fernet encryption exists, but scoped to tool "valves" config and OAuth session tokens only, both under a single static key (_fernet(), no versioning — confirmed directly, no key-version column anywhere). The actual model-provider credentials (OPENAI_API_KEYS) are an instance-wide, plaintext, semicolon-joined env list (config.py:325-329) — no per-user scoping, no encryption at all. Even open-webui's own user-facing API auth keys are a plaintext column (models/users.py,ApiKey.key = api_key, no encrypt call). Neither comp does per-user BYOK with envelope encryption or rotation; this vault is ahead of both on that specific axis, as the original commit's research claimed and I've now independently confirmed.Spec / roadmap drift (flagged, not fixed in-slice)
ROADMAP.md's v0.4 bullet ("Provider abstraction + encrypted, scoped credential vault; model router; cost/quota tracking") is not struck out — this PR ships only the vault-core half (user-scope accounts + credentials + resolution + CRUD HTTP). Org-scope provider accounts, the model router beyond instance-env fallback, cost/quota tracking, and OpenRouter/model-selection all remain outstanding and are tracked by the stacked sibling PR and later waves. Removing the roadmap line now would be premature; left as-is intentionally.Verification
pnpm install— clean.pnpm --filter api typecheck— clean (tsgo --noEmit).pnpm --filter api lint— clean (oxlint, type-aware, auto-fix touched nothing).pnpm exec jest --maxWorkers=1 src/providers src/models— 3 suites passed (25 tests), 1 DB-gated integration suite correctly skipped withoutTEST_DATABASE_URL.RLS_TEST_PORT=55461 bash apps/api/scripts/rls-test.sh(full harness, run once, nothing concurrent) — green end to end: 7 integration suites / 60 tests (including the new tampered-credential fail-closed test — confirmed the warning log fires), queue integration 8/8, e2e 44 passed / 3 skipped (includingprovider-accounts.e2e-spec.ts).pnpm --filter api build— clean; regeneratedopenapi.json(+201 lines), confirmed noapiKeyleak.Summary by cubic
Adds per-user BYOK provider accounts and an encrypted credential vault, plus HTTP CRUD, and integrates credential resolution to prefer user BYOK over instance env keys. Secrets are sealed with AES-256-GCM under a versioned key ring and never returned by the API.
New Features
provider_accountsandcredentialstables with RLS FORCE;openai_compatiblesupported first.CREDENTIAL_MASTER_KEYSenables lazy key rotation.SecretStringredacts secrets in logs/JSON; only explicit reveal at the model-client factory.GET|POST|DELETE /api/v1/provider-accounts; API key is write-only and hidden fromopenapi.jsonand responses.ModelsServiceresolution: user BYOK → instance env key → 402; instance boots without a vault (BYOK writes return 400).0018adds schema and policies;.env.exampledocumentsCREDENTIAL_MASTER_KEYS;openapi.jsonregenerated.Bug Fixes
CredentialCryptoError) now fail closed and log a warning, falling back to the instance env key; test added.SecretString.equals()uses constant-time compare on SHA-256 digests (no length leaks), and API keys are trimmed before sealing to prevent whitespace auth failures.last_used_atupdates to once per 60s per credential.Written for commit 6c1e11e. Summary will update on new commits.