feat(extensions): add StaticKeysTenantExtension — env-configured per-user API keys with per-schema isolation - #3675
Conversation
Sanderhoff-alt
left a comment
There was a problem hiding this comment.
The authentication path does not behave as the description states: the constant-time fallback can never match, and it turns a bad key into a 500 for some inputs. Two other points on tenant-id collisions and the untyped key map.
|
Fixing a tenant-isolation edge case found in review: mixed-case user IDs During code review we found that
Fix (commit Added regression tests covering: lowercase normalization, mixed-case + dash normalization, case-insensitive duplicate-user acceptance, and mixed-case dash-collision rejection, plus an auth-level test asserting a mixed-case key resolves onto the lowercased schema and a |
nicoloboschi
left a comment
There was a problem hiding this comment.
please move this work to https://github.com/vectorize-io/hindsight/tree/main/hindsight-extensions --- thanks!
…istry Env-configured static API keys with per-user schema isolation, shipped as a standalone extension package (hindsight_ext_static_keys_tenant) following the supabase-tenant pattern: pyproject for tests, Dockerfile for image packaging, registry README entry, and developer docs pointer. Carries over the reviewed implementation: no third-party deps beyond the server, constant-time byte key comparison, fail-fast init validation (schema collisions, >63-char schema names, duplicate keys), and lowercase user-id normalization matching Postgres identifier folding.
4f0f820 to
6f65f3e
Compare
|
Done — the work has been moved to the extensions registry as requested. What changed (PR now shows only these files):
Nothing remains in The extension is imported via |
|
@nicoloboschi thanks for the pointer to the registry — the move is fine and the package now follows the
If you would prefer this bundled in |
nicoloboschi
left a comment
There was a problem hiding this comment.
Thanks — the move to hindsight-extensions/ is done properly: the layout, pyproject.toml, Dockerfile (with ARG HINDSIGHT_IMAGE), entrypoint test, registry row and docs blurb all mirror supabase-tenant. I checked the branch out locally: 40 tests pass, ruff check and ruff format --check are clean.
Two things must change before merge, plus a few smaller ones.
Must fix
1. No CI job — none of this ever runs in CI.
supabase-tenant has test-extension-supabase-tenant (.github/workflows/test.yml:3928), a detect-changes filter (:180) and a gate entry (:5603), and that job also builds its image. This PR touches no workflow file, so its 40 tests and its Dockerfile are never exercised on any future change. Please add the same three pieces.
2. API keys leak into startup logs. See inline comments — the plaintext key ends up in ValueError messages on misconfiguration.
Should fix
3. Undocumented key-format constraints. A key cannot contain a comma (it is the entry separator), and a non-ASCII key can never authenticate: header values arrive latin-1-decoded while env values are utf-8-decoded, so encode("utf-8", "surrogateescape") on the two sides does not produce the same bytes. It fails closed, which is fine, but a permanent silent 401 is a miserable debugging session — please state "keys must be ASCII and must not contain a comma" in the README.
4. mcp_auth_disabled on a multi-user extension. You raised this yourself; my vote is refuse at init rather than ship parity. On ApiKeyTenantExtension the flag downgrades one shared key to none. Here it hands anyone unauthenticated MCP access to the base schema, in a deployment whose entire purpose is per-user isolation.
Nits
list_tenants()returning every configured user rather than only the provisioned ones is the right call forrun-db-migration, and better than whatsupabase-tenantdoes. It does mean the poller's fallback path issues one EXISTS probe per configured user on every idle cycle, against schemas that may not exist yet (worker/poller.py:440; the exception is swallowed, so it is harmless). Worth a README line pointing athindsight-admin run-db-migrationto pre-provision.- Two concurrent first requests from the same user both call
run_migration. Inherited fromsupabase-tenant, so not a blocker, but a per-schemaasyncio.Lockwould close it. - The branch is behind
main(still merges cleanly), and the PR description is stale: it still describeshindsight_api.extensions.builtin.multi_key_tenantand "30 tests".
… metering ids Review round 2 (nicoloboschi), must-fix vectorize-io#2 + inline comments: - The ValueError messages for a malformed HINDSIGHT_API_TENANT_USERS entry quoted the raw entry (user_id:api_key pair), so a misconfiguration like 'rafael:' would print the key of a nearby entry into startup logs — one paste into an issue and the key is disclosed. Errors now report the entry's index (and the user id once validated), never the key. - The duplicate-key error named the key itself; it now names the two conflicting user ids and the key's sha256-derived key_id. - _KeyEntry gains a stable, non-secret key_id (sha256 truncated to 16 hex chars), and RequestContext.api_key_id now carries it instead of a duplicate of tenant_id — metering can finally tell which of a user's keys authenticated, and errors can name a key without disclosing it. - Reworded the constant-time comment: the loop stops at the first match, so comparisons still depend on the matching key's position; harmless (invalid keys traverse the whole list) but the old text overpromised.
…rtup Review round 2 (nicoloboschi), should-fix vectorize-io#4. On ApiKeyTenantExtension the flag downgrades one shared key to none; here it would hand unauthenticated MCP clients the base schema in a deployment built for per-user isolation. The extension now raises ValueError at init when the variable is set, and authenticate_mcp always delegates to authenticate() (no bypass). Documented in the package README's variable table.
Review round 2 (nicoloboschi), should-fix vectorize-io#3 + poller nit: - README states the two key-format constraints (ASCII, no comma) and why: the comma is the pair separator, and a non-ASCII key can never authenticate because header values arrive latin-1-decoded while env values are utf-8-decoded — the bytes never match, so the key would fail closed with a permanent silent 401. - Documents hindsight-admin run-db-migration as the way to pre-provision all configured tenant schemas, so the worker's idle-cycle fallback probes hit real schemas instead of raising swallowed EXISTS errors.
Review round 2 (nicoloboschi), nit. Two concurrent first requests for the same user both saw the schema missing and both called run_migration (race inherited from supabase-tenant). A per-schema asyncio.Lock now serializes first initialization, with a re-check inside the lock so the loser of the race skips the redundant migration. Concurrent requests use distinct locks, so unrelated users never wait on each other.
Review round 2 (nicoloboschi), must-fix vectorize-io#1. The registry package had no CI coverage: its 40+ tests and its Dockerfile were never exercised on any change. Mirrors the supabase-tenant wiring exactly — a detect-changes filter and output mapping for hindsight-extensions/static-keys-tenant/**, a test-extension-static-keys-tenant job (uv sync, pytest, docker build on the latest-slim base), and the job in the report-pr-status gate.
Follow-up to the constant-time comment (review round 2, inline nit): _KeyEntry now stores the compare_digest-ready bytes (utf-8/surrogateescape, the same codec bearer-token bytes are recovered with), so authenticate() encodes only the incoming key per request instead of re-encoding every configured key. Loop behavior is unchanged — bytes vs bytes, no fast path.
|
All points addressed — one commit per item, pushed to the PR branch: Must fix #1 — CI job ( Must fix #2 — key leaks ( Should fix #3 — key constraints documented ( Should fix #4 — Nits:
PR description rewritten (the old one still said Checks: 50 tests pass ( Branch is rebased onto current |
Summary
Adds a
TenantExtensionmapping static API keys (from env) to per-user PostgreSQL schemas — shipped in the extensions registry (hindsight-extensions/static-keys-tenant/), not the server, per review: it follows thesupabase-tenantlayout (package, test-onlypyproject.toml, Dockerfile, README, tests, CI job). Closes #3674.Fully self-hosted multi-user memory isolation with no users table and no new services: a bridge between
ApiKeyTenantExtension(single shared key) andSupabaseTenantExtension(external IdP).Configuration
HINDSIGHT_API_TENANT_MCP_AUTH_DISABLEDis deliberately not supported — setting it fails at startup (MCP clients always authenticate with a user's key; no isolation bypass).Behavior
Authorization: Bearer <key>→ authenticated as the mapped user{prefix}_{user_id}provisioned lazily on first access, serialized per schema with anasyncio.Lock(no concurrent double-migration); provisioning failure raisesAuthenticationError(not cached)hmac.compare_digestAuthenticationError); non-ASCII bearer tokens → 401, never a 500authenticate()setscontext.tenant_idand a stable non-secretapi_key_id(truncated sha256 of the key) — metering can distinguish a user's multiple keyslist_tenants()returns all configured users so the worker and maintenance sweep per-user schemas;hindsight-admin run-db-migrationpre-provisions them (documented)/healthstays publicTesting
uv run pytestin the package, no DB): init validation (missing/invalid config, SQL-injection ids, schema collisions, duplicate keys, key-format edge cases), auth (valid/invalid/missing/non-ASCII, metering fields, per-key ids, schema provision-once + concurrency + failure-not-cached), MCP (auth required, flag refused),list_tenants, loader integration, package entrypointtest-extension-static-keys-tenantruns the suite and builds the image on thelatest-slimbase (detect-changes filter + gate wired)ruff/ruff format --check/tycleanCompatibility
Fully opt-in; no changes to the server package. Only activates when
HINDSIGHT_API_TENANT_EXTENSIONpoints at it.Related