perf: stop hashing API secrets as if they were human passwords

[cnmaia]

You asked why an API key check takes 180ms when it should be instant. It shouldn't take 180ms, and the reason it did is that the wrong tool was being used for the job.

Measured in production, not on a laptop

/health-check/, no auth	2ms
authenticated request	159ms
bcrypt.checkpw alone	151ms
10 authenticated in parallel	1.59s
/health-check/ during that load	315ms

The last row is the one that matters. bcrypt is synchronous and the dependency calling it is async def, so the event loop is blocked for 150ms at a time, and each instance runs a single uvicorn worker. An endpoint with no authentication at all degraded 150× because authentication monopolised the loop. This was never only an auth problem.

Why the slowness bought nothing

bcrypt is a password hash. Its cost exists to make brute force against low-entropy human passwords expensive — "datamap2024".

What is verified here is not a human password. It is g*aZkbWom3deiAX-vtoT: a system-generated random token, ~120 bits. Nobody reaches that in 2^120 attempts at 150ms each, and nobody reaches it at a nanosecond each either. The cost was charged on nearly every call from the webapp and defended against nothing.

What it is now

HMAC-SHA256 with a server-side pepper, compared with compare_digest, algorithm named in the stored value:

hmac-sha256$<hex>     new
$2b$12$...            bcrypt, still accepted

The pepper is why this is stronger than what it replaces, in a scenario this repository has actually lived through. If the database leaks — and the Postgres password was public for eighteen months — bcrypt lets an attacker test candidate secrets offline. HMAC gives them nothing to test against, because the pepper lives in the environment, not in the table. Plain SHA-256 would not have that property.

Three of the eight clients belong to external people whose secrets we cannot rotate. So a successful bcrypt verification rewrites the stored hash in the new format: every client migrates on its first request, with no secret change and nothing to coordinate with anyone. A failed upgrade is logged and does not fail the request.

	before	after
authenticated request	159ms	4ms
health-check during 10 parallel	315ms	32ms
integration suite	109s	35s

Two things found alongside that matter more than the latency

The wrong secret was logged in plaintext. logging.warn(f"incorrect api_secret {salted_api_secret}"), and the redaction filter did not catch it. During the rotation now in progress, any client still holding the old secret would write it to the log on every request. The api_key of an unknown client was logged too. Both gone, with tests asserting neither reappears.

A per-process cache made disable a lie. ClientService.fetch was memoised with lru_cache; cache_clear() only cleared the instance that served the change. With two instances, rotating a secret left the other one still accepting the old secret, and disabling a client left it enabled there until restart. The cache is gone — verification no longer costs 150ms, so all that remains is a sub-millisecond lookup.

The suite got faster, which broke the DOI tests

Not flakiness. The WireMock stub generated the DOI as {{now format='yyyyMMddHHmmss'}} — a timestamp to the second — so any two DOI creations inside the same second collided on the unique constraint. The fixture was always wrong; 150ms of bcrypt per request had been spacing the calls more than a second apart by accident. It now appends a random suffix.

Verification

• 295 unit tests (24 new), 241 integration tests, all green.
• The upgrade path is proven end to end, not mocked: the seed inserts a bcrypt hash, and after the first authenticated request the row reads hmac-sha256$.
• Tests cover the wrong pepper, an empty pepper, a wrong secret after upgrade, a failed upgrade not failing the request, and that neither the secret nor the api_key reaches the log.
• AUTH_CLIENT_SECRET_PEPPER is required with a minimum length, so a missing or trivial pepper stops the application instead of silently weakening every hash. It is already set in production (48 chars), and I confirmed make reads it intact — the # truncation trap from feat: set and compare credentials without anyone reading them #100 applies to this file.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EQda9NZvkbStEeNU54Tqgh