Skip to content

Upgrades operator key management and enforcement - #125

Draft
therevoltingx wants to merge 4 commits into
mainfrom
fix/updates-operator-key-management
Draft

Upgrades operator key management and enforcement#125
therevoltingx wants to merge 4 commits into
mainfrom
fix/updates-operator-key-management

Conversation

@therevoltingx

@therevoltingx therevoltingx commented Aug 12, 2026

Copy link
Copy Markdown

Summary

  • Replace shared plaintext API_KEY middleware with hashed operator credentials stored in SQLite (api_keys, schema v6), matching seller-agent's model.
  • Bootstrap the first operator key only via CLI (ad-buyer create-operator-key / list-operator-keys / delete-operator-key); no unauthenticated HTTP mint path.
  • Gate all non-public REST routes and MCP-over-HTTP tools (except health_check) with require_operator_key; additional keys via POST /auth/api-keys/operator with an existing operator credential.
  • Keep a one-release deprecated API_KEY env shim when no DB operator keys exist yet; ignore it once any hashed key is present.
  • Point SELLER_ENDPOINTS examples at the seller agent's default port (see below).
  • Update auth docs, setup-wizard copy, .gitignore, and unit/integration/smoke tests for the new control-plane auth.

Seller endpoint port (8000)

SELLER_ENDPOINTS is 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, and run-demo.sh used :8001, so a developer following the docs pointed the buyer at a port where no seller listens.

  • All SELLER_ENDPOINTS examples (and the seller-side URLs in docs such as seller.example.com) now use :8000.
  • This matters more now that auth is enforced: a seller URL that refuses connections is easily mistaken for the new 401s, making the auth upgrade look broken.
  • IAB_SERVER_URL is unchanged — it addresses the IAB agentic-direct server, not a seller agent.
  • OPENDIRECT_BASE_URL is unchanged — it addresses a separate OpenDirect endpoint.

Anyone setting SELLER_ENDPOINTS explicitly is unaffected; only the documented examples and demo defaults moved.

Test plan

  • ad-buyer create-operator-key --label "Primary operator" with the same DATABASE_URL as the server; key printed once
  • GET /health succeeds without a key; GET /bookings returns 401 without a key
  • GET /bookings and POST /auth/api-keys/operator succeed with Authorization: Bearer <key> or X-Api-Key: <key>
  • POST /auth/api-keys/operator without an existing operator key returns 401 (no HTTP bootstrap)
  • MCP over HTTP denies tools without an operator key; stdio still works without a key
  • With only API_KEY set and an empty api_keys table, the env shim still authenticates; after minting a DB key, the shim is ignored
  • ./run-demo.sh brings up the seller on :8000 and the buyer on :8001 and completes a booking end to end
  • uv 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.py
  • uv run --locked ruff check src/ tests/ && uv run --locked ruff format --check src/ tests/

@atc964 atc964 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  2. 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.

  3. 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.

  4. 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.

@therevoltingx
therevoltingx marked this pull request as draft August 19, 2026 19:09
@therevoltingx

Copy link
Copy Markdown
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants