Skip to content

feat: API keys and GET /v1/me behind one auth hook (step 3.1) - #31

Open
devinder-dev wants to merge 1 commit into
mainfrom
feat/api-auth
Open

devinder-dev wants to merge 1 commit into
mainfrom
feat/api-auth

Conversation

@devinder-dev

Copy link
Copy Markdown
Owner

What

Step 3.1 of Phase 3: API keys, and the first route behind them, GET /v1/me.

Why it works this way

  • One hook for all of /v1. authenticate is an onRequest hook inside the /v1 plugin,
    so every route registered there is protected and /health/* stays open (Fastify
    encapsulation). request.owner is set by the hook; routes never read the header themselves.
  • Keys are relay_live_ + 43 base64url characters (32 random bytes). Postgres stores only
    the SHA-256 hash (bytea) and the first 15 characters as a prefix for display. A lookup is
    one indexed query by hash. No bcrypt: a slow hash protects guessable passwords, and a
    256-bit random key isn't guessable; bcrypt would also make every request slow.
  • The header is checked before any database query. Bearer in any letter case, exactly one
    space, then the strict key pattern. Every failure gives the same 401 unauthorized, so the
    answer never says which part was wrong. Two Authorization headers are refused too (counted
    in the raw headers, because the runtime keeps only the first one in request.headers).
  • Revoked keys don't work: the lookup only matches rows with revoked_at IS NULL. It's the
    one query without an owner filter, because it is how we find the owner.
  • text/plain bodies are refused with 415 (the parser is removed), so only JSON reaches
    the routes.
  • Database errors are logged with safe fields only (code, message, severity, table,
    constraint, routine). Postgres puts row values in detail and where, and later those rows
    hold signing secrets.
  • Owners are created from the command line, not over HTTP: bun src/cli/create-owner.ts --name "Acme". The name is checked first (1–100 characters, no control characters, the same
    limit as the table's CHECK). The key is printed once and never stored in plain text.

Changes

  • API: auth/api-key.ts, auth/authenticate.ts, routes/v1.ts, routes/me.ts,
    repositories/owners.ts, services/owners.ts, schemas/owners.ts, cli/create-owner.ts;
    app.ts (buildApp takes the database and an optional log stream for tests, request.owner,
    no text/plain), errors.ts (unauthorized(), optional details), error-handler.ts.
  • Tests: unit tests for the key format and the header parser; integration tests against a
    real Postgres: a fresh key gets its owner, owner B's key gets B, revoked and unknown keys
    get 401, two headers over a real socket, the CLI, a closed database port gives 503 with
    Retry-After, and no log line holds a full key.
  • Docs: docs/api.md (the auth rows and the 415 row).

No migration: the tables are from migration 001.

Tested

  • bun run typecheck, bun run test:db: 97 pass, 0 fail.
  • create-owner in the built image with a read-only filesystem as uid 1000, against the test database.
  • Database blip: GET /v1/me twice a second while the test Postgres restarted gave one
    503 with Retry-After: 5, then 200 again without a restart of the API. /health/ready
    stayed 200 the whole time. The key appeared 0 times in the logs.

@AbdikhaniAbdirashid

Copy link
Copy Markdown
Collaborator

Read all 20 files. No blockers — this is careful work, and the reasoning is written down where it
matters. A few things I want to say back in my own words, partly so I can defend them on my own.

The hook placement is the important choice. app.addHook("onRequest", authenticate(sql)) inside
the v1Routes plugin means every current and future /v1 route is protected by Fastify's
encapsulation, while the health routes stay open in their own scope. The alternative —
preHandler per route — fails open: forget it once and that route is public with nothing to
show it. This fails closed. That difference is the answer to "how do you know every endpoint is
protected", and the test for unknown /v1 paths plus open operational routes pins it.

SHA-256 with no salt is right here, and I can say why. The secret is 32 bytes from
randomBytes, not a human-chosen password, so guessing is not a viable attack at any speed. bcrypt
and argon2 buy deliberate slowness against dictionary attacks that can't happen here, and would
cost ~100 ms on every single request. A salt would also break the UNIQUE on key_hash and stop
the lookup being one indexed query. The test that Bun's SHA-256 and Postgres agree on the same
bytes is worth having — an encoding mismatch there would mean CLI-made keys silently fail to
authenticate.

logSafe() is the part I'd have missed. On Postgres 18.6 a CHECK failure's detail carries the
whole offending row, which from #32 onwards includes signing_secret. Stripping detail, where,
hint and internal_query before logging closes a real leak, not a theoretical one. Same for the
CLI never printing database errors.

The duplicate Authorization check over rawHeaders is a good catch. request.headers.authorization
collapses duplicates, so a second injected header could be smuggled past the parse. Testing it over
a raw socket is the only way that holds, since an HTTP client would merge the headers for you —
same shape as the nc -G problem in #17: a test that can't distinguish the two cases proves nothing.

Two cosmetic notes, neither worth another round:

  1. me.ts guards with if (owner === null) while feat: endpoints with an SSRF check and a 20-endpoint limit (steps 3.2 + 3.3) #32/feat: events with idempotency and fan-out, delivery reads, and CORS (steps 3.4–3.6) #33 use if (!request.owner). Both work given
    decorateRequest("owner", null); one spelling would read better.
  2. In create-owner.ts, once sql exists every failure becomes the generic "Could not create
    owner". Deliberate, and I agree with the reason, but it also hides an operator-facing cause like
    a duplicate name. Fine for a script only we run.

Approving separately so the chain can move. Reviewing #32 and #33 next, in order.

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

Approved. Comments above are notes and questions, not changes I need before merge.

This branch has not been deployed

No deployments
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