fix(auth): fail closed on the public dev encryption key, add versioned ciphertext - #77
fix(auth): fail closed on the public dev encryption key, add versioned ciphertext#77hasitpbhatt wants to merge 5 commits into
Conversation
…rovisioning restricted keys
![orcacode-review[bot]](https://avatars.githubusercontent.com/in/4254738?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 8 issues in this PR: 🟠 1 P1 · 🟡 5 P2 · ⚪ 2 P3.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
.env.example (line 19): ⚪ P3 Update .env.example Encryption section for the new fail-closed behavior
The same commit that made the dev-key behavior fail-closed updated the comment in app/config.py ("credential_encryption_key is NOT auto-generated ... guards refuses to boot") and added the allow_insecure_dev_key opt-out, but the repository's config template was not touched. .env.example still says "# ── Encryption (auto-generated on first run if empty) ──" and gives no hint about the new opt-out flag. An operator who copies the example and leaves CREDENTIAL_ENCRYPTION_KEY empty (as "auto-generated" implies is fine) now hits the new startup RuntimeError on any non-SQLite database or as soon as a provider key exists, with the only escape being an undocumented env name (see the other finding about ORCA_ALLOW_INSECURE_DEV_KEY). Update .env.example's Encryption section to match the new fail-closed behavior and list the opt-out flag.
app/routes/keys.py (line 76): ⚪ P3 Expose model_allowlist/budget_limit_cents in GET /v1/keys serialization
create_key now returns model_allowlist and budget_limit_cents in its response, and the whole point of the feature is provisioning restricted keys. But GET /v1/keys (list_keys) was not updated to serialize these fields, so the only read path for existing keys cannot show which keys carry restrictions. An operator auditing keys or building tooling on the list endpoint cannot see budgets/allowlists, while the create response exposes them — the API shape is split. Add "model_allowlist" and "budget_limit_cents" to the list_keys serialization (and the dashboard render, which currently has no way to display or edit restrictions).
Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.
| def _allow_flag_enabled(settings_value: bool, os_environ) -> bool: | ||
| if settings_value: | ||
| return True | ||
| return str(os_environ.get(_ALLOW_FLAG_ENV, "")).lower() in ("1", "true", "yes") |
There was a problem hiding this comment.
🟡 P2 Make the insecure-dev-key opt-out honor the Settings field / repo env naming
The new opt-out exists under two names that never meet. The Settings field added in app/config.py is allow_insecure_dev_key, which pydantic-settings (no env_prefix configured anywhere in this repo; every other setting uses its bare name, e.g. DATABASE_URL, CREDENTIAL_ENCRYPTION_KEY) reads from ALLOW_INSECURE_DEV_KEY / .env. But the guard reads ORCA_ALLOW_INSECURE_DEV_KEY straight from os.environ (_ALLOW_FLAG_ENV), and every user-facing message (guards.py:76 RuntimeError remediation, encryption.py:16,57 warning) instructs operators to "set ORCA_ALLOW_INSECURE_DEV_KEY=1". ORCA_ALLOW_INSECURE_DEV_KEY is not a field alias, so in .env (this repo's documented config file; pydantic-settings loads .env itself and nothing calls load_dotenv, and os.environ is only populated from .env when run under docker-compose's env_file) the flag is invisible to both the Settings field and the guard's os.environ read. A user running uvicorn app.main:app (scripts/start.py path) with a Postgres DB or existing provider rows who follows the printed remediation by adding ORCA_ALLOW_INSECURE_DEV_KEY=1 to .env still gets the RuntimeError and the app refuses to boot. Fix: drop the separate env name and have the guard consult the Settings value (already passed as allow_insecure_dev_key) as the single source of truth, and document that name in .env.example; or add an env_prefix/alias so the documented name maps to the field.
|
|
||
| return int( | ||
| (await session.execute(select(func.count()).select_from(ProviderKey))).scalar_one() | ||
| ) |
There was a problem hiding this comment.
🟡 P2 Count only live (is_deleted == 0) provider keys in the startup guard
_count_provider_keys counts every row in provider_keys with no is_deleted == 0 filter, unlike every other ProviderKey query in the codebase (providers.py list/PUT, hosted.py, analytics.py, and build_deployments/usable_providers_from_db all treat soft-deleted rows as absent). The repo's own providers.py documents that pre-existing dev DBs carry soft-delete tombstones from before the hard-delete change (b6fed33). Upgrade path: an operator's SQLite DB has only tombstoned provider-key rows (keys they deleted via the old soft-delete flow), no CREDENTIAL_ENCRYPTION_KEY, and the new guard runs at boot. The count returns N>0, so is_sqlite and key_rows == 0 is false and boot fails with RuntimeError "N provider key(s) already exist in this database ... re-save your provider keys" — even though no live credential is at risk and there is nothing to re-save. The old version booted and served fine in this state. The guard's own contract ("refuse to boot once real credentials are at stake", "zero stored provider keys -> allowed (fresh dev install)") is contradicted by counting deleted rows; the fix is to add ProviderKey.is_deleted == 0 to the count, matching every sibling query.
| detail=( | ||
| "API key budget exhausted " | ||
| f"({spend} of {kc.budget_limit_cents * 10_000} microcents spent)." | ||
| ), |
There was a problem hiding this comment.
🟠 P1 Make the budget enforcement atomic so concurrent requests cannot all pass the cap
The new budget gate is a check-then-act with no reservation: get_lifetime_spend_microcents sums only already-committed RequestLog rows, and a request's own spend row is committed only after the upstream call completes (blocking path: finally commit; streaming path: _finalize). Two or more concurrent requests for the same key therefore all read the same pre-admission sum, all pass budget_exceeded, and all get billed — the "lifetime cap" is exceeded by the sum of every in-flight request. The window is the entire upstream call (seconds), so a burst of N concurrent requests (exactly the leaked-key flood this feature exists to stop) is fully admitted and billed: the documented contract "an exhausted key costs the operator nothing" / "lifetime cap" is violated and the operator is charged beyond the limit. Fix: reserve atomically at admission instead of reading-then-checking, e.g. a per-key budget row updated with UPDATE ... SET reserved = reserved + est WHERE key_id = ? AND spent + reserved + est <= limit (and reconciled with the actual cost at completion), or hold SELECT ... FOR UPDATE on the key row from admission until the log commit so admissions per key are serialized.
| # resolution, or cache work so an exhausted key costs the operator | ||
| # nothing — no upstream attempt, no cache fill. | ||
| if kc.budget_limit_cents is not None: | ||
| spend = await get_lifetime_spend_microcents(db, str(kc.key_id)) |
There was a problem hiding this comment.
🟡 P2 End the budget-check read transaction before the upstream call so it cannot block log commits on SQLite
For a budgeted key, the new get_lifetime_spend_microcents(db, ...) is the first statement executed on the request-scoped session db, and nothing commits or rolls it back before the handler returns. In the streaming path the request-log row is written by _finalize through a different session (session_mod._session_factory()), while the request session's transaction — and its SQLite SHARED lock, since the engine enables no WAL/busy-timeout settings — stays open across the entire upstream stream (the db dependency is torn down only after the response body completes). A separate connection's COMMIT needs an EXCLUSIVE lock and will hit SQLITE_BUSY ("database is locked", default 5s busy timeout) for as long as any budgeted streaming request is in flight, so the stream's own log write (and concurrent requests' log writes) fail, are swallowed by the except Exception: logger.warning("request_log_commit_failed") handlers, and the spend/analytics row is silently dropped — the budget then undercounts that request's spend. Before this change the streaming path never executed anything on db, so the request session never opened a transaction. Fix: end the read transaction immediately after the budget check (e.g. await db.rollback()), or run the spend lookup on a short-lived session, so the request session does not hold a read lock across the upstream call.
| key_rows = 0 | ||
|
|
||
| if is_sqlite and key_rows == 0: | ||
| return |
There was a problem hiding this comment.
🟡 P2 Guard fails open when the provider-key count cannot be completed
assert_credential_encryption_ready is a fail-closed control: it must refuse to boot whenever the publicly-known dev encryption key would protect real provider credentials. The except Exception: key_rows = 0 branch converts ANY failure of the count query (SQLite lock, I/O error, connection failure, corrupted table) into "zero provider keys", and for SQLite (is_sqlite and key_rows == 0) the guard then returns normally — the app boots and keeps provider keys sealed with the public dev key, exactly the condition the guard exists to refuse. In main.py the guard runs after Base.metadata.create_all, so a missing table cannot occur there; every other exception is a genuine DB failure and should fail closed (re-raise) rather than be treated as "empty database". Only an explicit "no such table" error should map to zero rows. As written, a transient error (e.g. database is locked from a second instance sharing the SQLite file) silently defeats the security control and boot proceeds with insecure sealing.
| RequestLog.is_deleted == 0, | ||
| RequestLog.status_code < 400, | ||
| ) | ||
| return int((await session.execute(stmt)).scalar_one()) |
There was a problem hiding this comment.
🟡 P2 Count client-disconnect (499) spend toward the budget; a stream abort currently escapes the cap while the operator still pays upstream
get_lifetime_spend_microcents counts only rows with status_code < 400, but the streaming path records client aborts as status 499 (chat.py cancel branch sets status_code = 499 and still calls _finalize(), which computes cost from the partially-aggregated usage and persists the row). A client holding a budgeted key can stream a request, receive tokens, and drop the connection: the provider bills the tokens generated so far (real money for the operator), yet the row is recorded as 499 and is excluded from the spend sum — so the request is never counted against budget_limit_cents and can be repeated indefinitely, bypassing the cap entirely. The commit frames the cap as a lifetime limit on the operator's billable spend; a status code that is entirely client-controlled and carries real upstream cost must not silently escape the sum. Fix: include client-disconnect (499) rows in the spend aggregation, or record the disconnect cost under a billable status (e.g. keep the underlying 200/usage cost in a separate column and sum that), so an aborted stream still consumes budget.
…LLOW_INSECURE_DEV_KEY from .env
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 5 issues in this PR: 🟠 2 P1 · 🟡 3 P2.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
app/routes/keys.py (line 76): 🟡 P2 Return model_allowlist and budget_limit_cents from GET /v1/keys to match the create response
This change adds model_allowlist and budget_limit_cents to the POST /v1/keys create response (keys.py:106-107) and to the ApiKey model, and require_unrestricted/budget enforcement make those fields meaningful restrictions. But the read side of the same resource — GET /v1/keys — was left untouched and still serializes only id/name/key_prefix/is_active/last_used_at/revoked_at/created_at. A client that creates a restricted key and then lists keys (the only audit surface for the new restriction feature) cannot see which keys carry an allowlist or budget, so it cannot display or manage them; the restriction data is invisible once the create-time plaintext response is gone. The serialization on the wire was updated for one endpoint of the resource and not the other.
Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.
| detail=( | ||
| "API key budget exhausted " | ||
| f"({spend} of {kc.budget_limit_cents * 10_000} microcents spent)." | ||
| ), |
There was a problem hiding this comment.
🟠 P1 Make the budget check atomic against concurrent requests so the cap cannot be overshot
The new budget enforcement is a pure check-then-act on committed spend: get_lifetime_spend_microcents reads already-committed RequestLog rows, and budget_exceeded decides, with no lock, reservation or in-flight accounting. A key whose spend is just below budget_limit_cents can fire N parallel /v1/chat/completions requests; every one of them sees spend below the cap, all N proceed upstream, and only after they complete do their cost rows land — so total billable spend exceeds the configured cap by up to (N-1)×request cost. budget_limit_cents is described in the code and docs as a hard "lifetime cap", so the cap is not enforced under concurrency — the operator is charged beyond the configured limit. The new tests are strictly sequential and cannot exercise this. Fix by reserving estimated cost (or serializing per-key check+record in a transaction/row lock) before the upstream call, or at minimum documenting the cap as best-effort.
| RequestLog.api_key_id == api_key_id, | ||
| RequestLog.is_deleted == 0, | ||
| RequestLog.status_code < 400, | ||
| ) |
There was a problem hiding this comment.
🟠 P1 Count streamed-but-aborted spend toward the budget, or the cap is trivially bypassable
The new budget enforcement counts only request-log rows with status_code < 400. The streaming paths that actually consume provider-billed tokens record status 499 (client disconnect, chat.py:596) and 503 (mid-stream upstream error, chat.py:646); both are >= 400, so their cost never accrues toward get_lifetime_spend_microcents. In the disconnect case the usage chunk (stream end) usually never arrives, so the 499 row records cost 0 anyway. Either way, a client holding a budgeted key can start a streaming request, receive tokens, and disconnect — the pre-request budget check (chat.py:275-284) keeps seeing the old (never-growing) spend, so the key is never blocked while the operator's provider bill grows unbounded. The existing equivalent spend view (/v1/analytics/spend, app/routes/analytics.py:64-90) has no status filter and therefore counts these rows' cost in its total, so the dashboard's reported spend and the enforced budget disagree on the same rows. The budget's whole purpose (test docstring: "a leaked key meant unbounded spend") is defeated for streaming traffic. Fix: include 499/503 rows (they carry the tokens actually billed by the upstream) in the spend sum, or otherwise cap streaming spend before upstream dispatch.
| ), | ||
| ) | ||
|
|
||
| client = await router_cache.get_router(db) |
There was a problem hiding this comment.
🟡 P2 Make budget enforcement fail closed when request-log spend writes fail
The new budget enforcement sums spend exclusively from requests_log (get_lifetime_spend_microcents). But every request-log write in this handler swallows commit failures (except Exception as commit_err: logger.warning("request_log_commit_failed", ...) at lines 451, 558, 564, 737) and still returns 200/streams to completion. When a log commit fails (SQLite "database is locked" under concurrent writes, disk full, connection drop), the billed spend is never persisted, so every subsequent request's budget check undercounts and the 429 is never raised: an exhausted key keeps being served and billed past its cap indefinitely while the operator believes the limit is enforced. The swallow predates this commit, but the new budget control depends on those rows for its correctness and has no compensating mechanism (e.g., failing the request, or writing spend outside the best-effort log). Consequence: the accounted money limit is silently not applied on a failure path. Suggest: on log-commit failure, still surface the spend (or fail the request closed), or otherwise make the budget read a source that cannot be silently lost.
| select(func.count()) | ||
| .select_from(ProviderKey) | ||
| .where(ProviderKey.is_deleted == 0) | ||
| ) |
There was a problem hiding this comment.
🟡 P2 Count soft-deleted provider rows in the credential-encryption guard
_count_provider_keys counts only rows with is_deleted == 0, so a SQLite DB whose provider_keys rows are all soft-deleted tombstones (pre-hard-delete dev DBs, which the code elsewhere explicitly acknowledges exist: providers.py "pre-PR dev DBs might carry tombstones") reports key_rows=0 and the guard lets the app boot with the publicly-known dev encryption key. Those tombstoned rows still contain live provider credentials sealed with the dev key, so the guard's stated contract — "refuse to boot when provider credentials would be sealed with the publicly-known dev key" — fails open for exactly the credential-at-rest case it exists to prevent. The previous commit's version of this guard counted all rows (fail-closed); the live-row filter is a relaxation introduced in this commit. Low confidence because the tombstone scenario is legacy-DB-only and the filter is a documented, deliberate choice, but the guard's security promise is nonetheless not met for tombstoned secrets.
|
Addressing the OrcaCode review findings on this PR:
All changes are stacked; the tip of ix/budget-enforcement contains every fix above. |
|
Closing as part of PR-hygiene cleanup. This was part of an interdependent stack (#71 -> #75 -> #77 -> #81 -> #83 -> #85) rather than an independent branch from latest main. The unique work will be re-raised as clean, independent PRs branched directly from main, brought to distinguished engineering quality, and passed through a rigorous review gate before re-opening. Keys scoping + restricted-key blocking is already superseded by #89; encryption fail-closed / log-redaction / unhandled-exception work has largely landed via #87/#88; remaining unique parts (cache v2 key space, latency logging, async exception handler) will be re-raised independently. Reopen if you want to keep this branch. |
1 participant
Orca-Code-Review — push 2
❌ 2 findings block merge
Problem
The default deployment sealed every BYOK provider key with
sha256(b"orcarouter-lite-dev-key")— a constant in public source — with no warning and no way to rotate..env.exampleeven claimed the real key was "auto-generated on first run", which it never was.Full analysis in #76.
Fix
packages/auth/encryption.py: ciphertexts now version-prefixed (b"\x01" + nonce + ct+tag) enabling future rotation; legacy unversioned blobs keep decrypting, including the ~0.4% whose first nonce byte collides with the version byte (v1 parse attempted first, legacy onInvalidTag). First use of the dev fallback logs a prominentinsecure_dev_encryption_keywarning.packages/db/guards.py(new):assert_credential_encryption_ready()— fail-closed boot when the dev key would guard existing provider rows or any non-SQLite database; fresh SQLite installs pass; explicitORCA_ALLOW_INSECURE_DEV_KEY=1(env) orallow_insecure_dev_key(Settings) opts out.app/main.py: guard wired into the lifespan aftercreate_all.app/config.py: newallow_insecure_dev_key: bool = False; stale "auto-generated on first run" comment corrected to point atopenssl rand -hex 32.Tests
tests/unit/test_encryption_format.py(5): versioned blob shape + round-trip; legacy blob decrypt; nonce-collision recovery; wrong-keyInvalidTag; truncated blob raises.tests/unit/test_startup_guards.py(6): fresh sqlite allowed; rows ⇒ RuntimeError; non-sqlite ⇒ RuntimeError even empty; both opt-in paths bypass; no-op when a real key is configured.Verification
ruff check app packages tests: cleanStacked on #75 (shares lifespan/test infra).
Closes #76