Upgrades operator key management and enforcement - #125
Conversation
atc964
left a comment
There was a problem hiding this comment.
Strong PR overall: crypto and storage match the seller pattern, the gating inventory checks out exactly as described (42 of 43 MCP tools, health/docs public list identical to the old middleware), the v5 to v6 migration ran clean here against a real v5 database, bootstrap has no unauthenticated mint path, and the full suite is green on current main (3495 passed). Four asks before this anchors v2.5.0, ranked:
-
MCP auth fails open on SDK internals. In _deny_unless_operator, the broad except around mcp.get_context() treats any exception as trusted stdio, so a FastMCP behavior change would silently un-gate all 42 tools, and test_stdio_allows_without_key enshrines that. Please narrow the except or affirmatively detect the HTTP transport, and log whenever the trusted fallback engages.
-
Shim downgrade path, confirmed empirically, and the docs contradict it. The env shim gates on no ACTIVE operator keys, not no keys ever: I revoked all DB keys and the plaintext API_KEY resumed authenticating with a 200. authentication.md and the PR body both say the env key is ignored once any DB key exists, which is false in the revoked-all case. Either gate on any-row-ever (count_active_operator_keys could become count_all, it is currently unused) or keep it as a recovery path but document it and log loudly on every shim authentication. Docs must match whichever you choose.
-
Gated surfaces the PR leaves behind: the streamable/e2e smoke tests call tools anonymously and will fail against a properly gated live server (CI never sees it because they skip without a server), and run-demo.sh never mints a key so every documented follow-on curl 401s. Please plumb an operator key env into the smoke tests and add the create-operator-key step to the demo script. A route-sweep test (iterate app.routes, assert anonymous 401 outside the allowlist) would make every gate load-bearing; right now only two routes have anonymous-401 assertions and I could silently un-gate a route with the suite staying green.
-
Undisclosed second breaking change: the full buyer/seller port swap (8000/8001) plus OPENDIRECT_BASE_URL and IAB_SERVER_URL default changes across settings, scripts, and some twenty docs pages. Defensible change, but it is nowhere in the PR body. Please disclose it in the description and migration notes, or split it out. Related: v2.5.0 needs an upgrade doc covering keyless-mode removal, shim semantics and its removal release, and the port swap.
Noted, non-blocking: label uniqueness is check-then-insert (concurrent mints can duplicate), 401 detail reveals key state for a presented key, new code uses datetime.utcnow() against CODING_STANDARDS, and MediaKitClient still consumes the deprecated env key for outbound auth, which will silently de-authenticate when the shim is removed next release unless addressed.
Happy to re-review same day; this is the anchor for Friday's v2.5.0 so fast turnaround appreciated.
|
@atc964 thanks for the feedback. let me clean things up a bit more and will ping you when it's ready. not sure it'll make the friday cutoff but will try. |
Summary
API_KEYmiddleware with hashed operator credentials stored in SQLite (api_keys, schema v6), matching seller-agent's model.ad-buyer create-operator-key/list-operator-keys/delete-operator-key); no unauthenticated HTTP mint path.health_check) withrequire_operator_key; additional keys viaPOST /auth/api-keys/operatorwith an existing operator credential.API_KEYenv shim when no DB operator keys exist yet; ignore it once any hashed key is present.SELLER_ENDPOINTSexamples at the seller agent's default port (see below)..gitignore, and unit/integration/smoke tests for the new control-plane auth.Seller endpoint port (8000)
SELLER_ENDPOINTSis the setting that points at a seller agent, and the seller agent listens on port 8000 by default. The examples and defaults across.env.example, the quickstart, the configuration guide, andrun-demo.shused:8001, so a developer following the docs pointed the buyer at a port where no seller listens.SELLER_ENDPOINTSexamples (and the seller-side URLs in docs such asseller.example.com) now use:8000.IAB_SERVER_URLis unchanged — it addresses the IAB agentic-direct server, not a seller agent.OPENDIRECT_BASE_URLis unchanged — it addresses a separate OpenDirect endpoint.Anyone setting
SELLER_ENDPOINTSexplicitly is unaffected; only the documented examples and demo defaults moved.Test plan
ad-buyer create-operator-key --label "Primary operator"with the sameDATABASE_URLas the server; key printed onceGET /healthsucceeds without a key;GET /bookingsreturns 401 without a keyGET /bookingsandPOST /auth/api-keys/operatorsucceed withAuthorization: Bearer <key>orX-Api-Key: <key>POST /auth/api-keys/operatorwithout an existing operator key returns 401 (no HTTP bootstrap)API_KEYset and an emptyapi_keystable, the env shim still authenticates; after minting a DB key, the shim is ignored./run-demo.shbrings up the seller on:8000and the buyer on:8001and completes a booking end to enduv run --locked pytest tests/unit/test_operator_auth.py tests/unit/test_api_auth.py tests/unit/test_seller_base_url_resolution.py tests/unit/test_no_hardcoded_urls.py tests/unit/test_docs_inventory_drift.py tests/smoke/test_quickstart_smoke.py tests/integration/test_api_endpoint_integration.pyuv run --locked ruff check src/ tests/ && uv run --locked ruff format --check src/ tests/