-
Notifications
You must be signed in to change notification settings - Fork 75
fix(keys): scope /v1/keys list and revoke queries to the caller's workspace #75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f6ca9b1
76f098f
9240492
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
| from app.deps import get_db, get_key_context | ||
| from app.quality_scores import resolve_model_metrics | ||
| from app.schemas import ChatCompletionRequest | ||
| from packages.auth.spend import budget_exceeded, get_lifetime_spend_microcents | ||
| from packages.auth.types import KeyContext | ||
| from packages.db.models.request_log import RequestLog | ||
| from packages.litellm_adapter.catalog import CATALOG, CATALOG_BY_ID | ||
|
|
@@ -267,6 +268,21 @@ async def chat_completions( | |
| detail=f"Model '{body.model}' is not allowed for this API key", | ||
| ) | ||
|
|
||
| # Budget enforcement: `budget_limit_cents` is a lifetime cap on this | ||
| # key's billable (status < 400) spend. Checked before any routing, | ||
| # 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)) | ||
| if budget_exceeded(spend, kc.budget_limit_cents): | ||
| raise HTTPException( | ||
| status_code=429, | ||
| detail=( | ||
| "API key budget exhausted " | ||
| f"({spend} of {kc.budget_limit_cents * 10_000} microcents spent)." | ||
| ), | ||
| ) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 P2 Budget enforcement fails open when the request-log spend write fails The new lifetime-cap enforcement's only accounting source is |
||
| client = await router_cache.get_router(db) | ||
| raw_strategy = getattr(client, "strategy", None) | ||
| strategy = raw_strategy if isinstance(raw_strategy, str) and raw_strategy else "balanced" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
| from datetime import datetime, timezone | ||
|
|
||
| from fastapi import APIRouter, Depends, HTTPException, Response | ||
| from pydantic import BaseModel | ||
| from pydantic import BaseModel, Field | ||
| from sqlalchemy import select | ||
| from sqlalchemy.ext.asyncio import AsyncSession | ||
|
|
||
|
|
@@ -20,16 +20,46 @@ | |
|
|
||
| class CreateKey(BaseModel): | ||
| name: str | ||
| # Optional restrictions for child keys. Only reachable by unrestricted | ||
| # callers (require_unrestricted above), so a restricted key can never | ||
| # mint a sibling with looser limits than its own — it can't mint at all. | ||
| model_allowlist: list[str] | None = None | ||
| budget_limit_cents: int | None = Field(default=None, gt=0) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 P2 Bound budget_limit_cents to the Integer column range, or a large value 500s on Postgres Pydantic accepts any int > 0 ( |
||
|
|
||
|
|
||
| def require_unrestricted(kc: KeyContext) -> None: | ||
| """Key management is reserved for unrestricted keys. | ||
|
|
||
| A key that carries any restriction (`model_allowlist` or | ||
| `budget_limit_cents`) must not be able to mint, list, or revoke other | ||
| keys — otherwise it could create a sibling with no restrictions and | ||
| trivially bypass its own allowlist/budget. Unrestricted keys already | ||
| hold the maximum privilege this single-workspace edition exposes | ||
| (same trust level as PUT /v1/providers/*), so denying restricted keys | ||
| here grants nothing to anyone; it only closes the escalation path. | ||
| """ | ||
| if kc.model_allowlist is not None or kc.budget_limit_cents is not None: | ||
| raise HTTPException( | ||
| status_code=403, | ||
| detail=( | ||
| "Restricted API keys cannot manage keys. " | ||
| "Use an unrestricted key." | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| @router.get("") | ||
| async def list_keys( | ||
| _kc: KeyContext = Depends(get_key_context), | ||
| kc: KeyContext = Depends(get_key_context), | ||
| db: AsyncSession = Depends(get_db), | ||
| ) -> dict: | ||
| require_unrestricted(kc) | ||
| rows = ( | ||
| await db.execute( | ||
| select(ApiKey).where(ApiKey.is_deleted == 0).order_by(ApiKey.created_at) | ||
| select(ApiKey).where( | ||
| ApiKey.workspace_id == kc.workspace_id, | ||
| ApiKey.is_deleted == 0, | ||
| ).order_by(ApiKey.created_at) | ||
| ) | ||
| ).scalars().all() | ||
| return { | ||
|
|
@@ -54,12 +84,15 @@ async def create_key( | |
| kc: KeyContext = Depends(get_key_context), | ||
| db: AsyncSession = Depends(get_db), | ||
| ) -> dict: | ||
| require_unrestricted(kc) | ||
| full_key, key_hash, key_prefix = generate_api_key() | ||
| row = ApiKey( | ||
| workspace_id=kc.workspace_id, | ||
| name=body.name, | ||
| key_hash=key_hash, | ||
| key_prefix=key_prefix, | ||
| model_allowlist=body.model_allowlist, | ||
| budget_limit_cents=body.budget_limit_cents, | ||
| ) | ||
| db.add(row) | ||
| await db.commit() | ||
|
|
@@ -70,18 +103,28 @@ async def create_key( | |
| "name": row.name, | ||
| "key_prefix": row.key_prefix, | ||
| "api_key": full_key, # plaintext shown ONCE | ||
| "model_allowlist": row.model_allowlist, | ||
| "budget_limit_cents": row.budget_limit_cents, | ||
| } | ||
|
|
||
|
|
||
| @router.delete("/{key_id}", status_code=204) | ||
| async def revoke_key( | ||
| key_id: str, | ||
| _kc: KeyContext = Depends(get_key_context), | ||
| kc: KeyContext = Depends(get_key_context), | ||
| db: AsyncSession = Depends(get_db), | ||
| ) -> Response: | ||
| require_unrestricted(kc) | ||
| row = ( | ||
| await db.execute( | ||
| select(ApiKey).where(ApiKey.id == key_id, ApiKey.is_deleted == 0) | ||
| select(ApiKey).where( | ||
| ApiKey.id == key_id, | ||
| # Workspace scoping: without this, any key could revoke any | ||
| # other workspace's keys (the write path has always been | ||
| # scoped; the read/delete paths were not). | ||
| ApiKey.workspace_id == kc.workspace_id, | ||
| ApiKey.is_deleted == 0, | ||
| ) | ||
| ) | ||
| ).scalar_one_or_none() | ||
| if row is None: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| """Per-key spend lookup used to enforce `ApiKey.budget_limit_cents`. | ||
|
|
||
| Semantics: `budget_limit_cents` is a lifetime cap on the key's billable | ||
| spend — request-log rows with `status_code < 400`. 1 cent = 10,000 | ||
| microcents (1 USD = 1,000,000 microcents, matching chat.py's cost math). | ||
|
|
||
| Kept free of FastAPI imports so it stays unit-testable and reusable from | ||
| non-HTTP contexts (background jobs, CLI minting tools). | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from sqlalchemy import func, select | ||
| from sqlalchemy.ext.asyncio import AsyncSession | ||
|
|
||
| from packages.db.models.request_log import RequestLog | ||
|
|
||
| MICROCENTS_PER_CENT = 10_000 | ||
|
|
||
|
|
||
| async def get_lifetime_spend_microcents( | ||
| session: AsyncSession, api_key_id: str | ||
| ) -> int: | ||
| """Sum of billable (status < 400) spend ever recorded for this key.""" | ||
| stmt = select(func.coalesce(func.sum(RequestLog.cost_microcents), 0)).where( | ||
| RequestLog.api_key_id == api_key_id, | ||
| RequestLog.is_deleted == 0, | ||
| RequestLog.status_code < 400, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 P2 Count 499/503 stream rows (which carry real upstream cost) toward the budget The budget excludes every row with status_code >= 400. But on the streaming path, a client disconnect (status 499) or a mid-stream upstream failure (status 503) still records the aggregated usage from the chunks already delivered (agg_usage populated from the stream), so those rows carry real, provider-billed cost — tokens were consumed upstream before the stream ended. Those rows are systematically excluded from get_lifetime_spend_microcents, so actual operator spend is undercounted: a leaked key can burn real money through streams that never complete (disconnect right after the last chunk, or repeated failing streams) without ever consuming its budget. The "billable = status < 400" assumption does not match what the provider bills. |
||
| ) | ||
| return int((await session.execute(stmt)).scalar_one()) | ||
|
|
||
|
|
||
| def budget_exceeded(spend_microcents: int, budget_limit_cents: int) -> bool: | ||
| return spend_microcents >= budget_limit_cents * MICROCENTS_PER_CENT | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 P1 Make budget enforcement atomic per key — the read-check-write is a TOCTOU race that lets concurrent requests overshoot the cap
The new budget enforcement is a plain check-then-act:
get_lifetime_spend_microcentsrunsSELECT SUM(cost_microcents)(packages/auth/spend.py) and rejects only if the already-committed total is over the cap. The request's own billable cost row is inserted and committed only AFTER the upstream call completes — blocking path:db.add(log); await db.commit()in thefinallyat line 733-735; streaming path:_finalize()at stream end via a separate session (line 560-562). There is no serialization anywhere: noSELECT ... FOR UPDATEon the key row, no conditional UPDATE with rowcount check, no per-key lock, and the engine sets no isolation level (Postgres default READ COMMITTED, SQLite default). So N concurrent requests for the same key can all read spend below the cap, all be served, and all commit their cost rows — the key's realized spend exceedsbudget_limit_centsby up to N × per-request cost. Because the check-to-record window spans the entire upstream call/stream (seconds to minutes), any budgeted key under concurrent load near its cap overshoots; the commit's own claim ("lifetime cap … so an exhausted key costs the operator nothing") is false under concurrency and the operator is billed past the configured cap. Fix: make the accounting atomic — e.g. add a per-keyspent_microcentscolumn and commit spend with a conditional UPDATE (UPDATE api_keys SET spent_microcents = spent_microcents + :cost WHERE id = :key_id AND spent_microcents + :cost <= budget_limit_microcents), treating rowcount == 0 as exhausted and writing the request-log row in the same transaction; at minimum, re-check the sum after the upstream call and before committing the billable row under a key-row lock.