From 9c7befe5951a2ee1c8ea02e0969ddfb4c03739a1 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 3 Aug 2026 22:18:33 +0200 Subject: [PATCH] Hash and verify passwords off the event loop (B-55) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Argon2 is deliberately expensive — tens of milliseconds of CPU per call. Called inline from the async handlers for register, login, change-password and the admin reset, that cost froze the entire process for its duration: every other request, plus all six background tasks (scheduler, confirmation poller, RBF bumper, listener, both reconcilers). A burst of unauthenticated login attempts was therefore not just slow logins, it delayed draws and confirmations. hash_password_async/verify_password_async wrap the existing pair in run_in_threadpool, and every async caller now uses them. The synchronous functions stay: they're what the wrappers call, and what tests and scripts (no running loop) use directly. The regression test runs a heartbeat task alongside the hashing and counts how often the loop got to run it — 1 tick with the old inline call, many with the threadpooled one. Also drops the running "already fixed and removed" list from BUGS.md: the file tracks open findings, and `git log --all --grep 'B-nn'` is the record of how a closed one was closed. Co-Authored-By: Claude Opus 5 --- BUGS.md | 26 +++-------------- CLAUDE.md | 6 ++-- app/api/routes/admin.py | 4 +-- app/api/routes/users.py | 11 +++++-- app/auth/routes.py | 11 +++++-- app/auth/security.py | 23 +++++++++++++++ tests/unit/test_security.py | 58 +++++++++++++++++++++++++++++++++++++ 7 files changed, 106 insertions(+), 33 deletions(-) diff --git a/BUGS.md b/BUGS.md index 6efcacc..0a470ef 100644 --- a/BUGS.md +++ b/BUGS.md @@ -5,14 +5,10 @@ Third full-codebase audit, opened after the 2026-07-26 (B-01 … B-24) and fixed finding, B-51. The list opened at B-52 … B-72 and holds only what is still **open**: a finding is -removed from this file once it is fixed. Per CLAUDE.md's convention each entry gets -its own commit with its own regression test, and the `B-nn` marker goes in a comment -next to the fix so `git log --all --grep 'B-nn'` finds it later. - -Already fixed and removed: B-52 (a round past ~50 participants deadlocked the -payout — `025754c`), B-53 (a bet could pay into the pool of a round it was left -out of — `64f6229`), B-54 (`X-Forwarded-For` was read from the front, so every -IP-keyed control was bypassable — `907e32e`). +removed from this file once it is fixed, and is not listed here afterwards. Per +CLAUDE.md's convention each entry gets its own commit with its own regression test, +and the `B-nn` marker goes in a comment next to the fix, so +`git log --all --grep 'B-nn'` is the record of how any closed finding was closed. State of the tree at audit time: 264 unit tests, all passing; `tests/integration/` still empty; withdrawal and the RBF bump path still never live-broadcast. @@ -44,20 +40,6 @@ remains the last prerequisite for running unattended. ## High — security -### B-55 — Argon2 hashing runs on the event loop - -`app/auth/security.py:20-39`, called from `app/auth/routes.py` and -`app/api/routes/users.py`. - -`hash_password`/`verify_password` are synchronous and cost tens of milliseconds -each, so every login, registration and password change blocks the whole process — -including all six background tasks (scheduler, confirmation poller, RBF bumper, -listener, both reconcilers). A burst of unauthenticated login attempts (which, -per B-54, is not effectively throttled) is a cheap denial of service that also -delays draws and confirmations. - -Fix: run both through `starlette.concurrency.run_in_threadpool`. - ### B-56 — `RateLimiter._buckets` is never pruned `app/auth/rate_limit.py:37`. diff --git a/CLAUDE.md b/CLAUDE.md index d95705c..891ea64 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin ## Project status -All 10 stages of the original build order are code-complete and unit-tested — 278 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. +All 10 stages of the original build order are code-complete and unit-tested — 281 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. Verified on mainnet with real money: registration + address derivation, deposit crediting (1-conf), a real 10 PLM bet (broadcast → confirmed → change credited back), and one full round cycle (close → draw on a real block hash → 70/30 payout with sat math checked against the broadcast tx → confirmation → close → next round auto-opened). **Withdrawal and the RBF bump path have never been exercised against a live broadcast** — unit-tested only. @@ -33,7 +33,7 @@ PYTHONPATH=. python scripts/decrypt_master_key.py # ops recovery: decrypt+pr PYTHONPATH=. python scripts/encrypt_master_key.py # ops bootstrap: import an externally-generated xprv (--overwrite to replace) PYTHONPATH=. python scripts/electrum_smoke_test.py # manual check: connect, handshake, subscribe to headers, print the tip -python -m pytest # all 278 tests +python -m pytest # all 281 tests python -m pytest tests/unit/test_hd.py # one file python -m pytest tests/unit/test_hd.py::test_derivation_is_deterministic # one test ``` @@ -61,7 +61,7 @@ The `Caddyfile` sends baseline security headers — HSTS, `X-Content-Type-Option - Python 3.12+, FastAPI, SQLAlchemy 2 async + Alembic, SQLite via aiosqlite, `embit` for keys/PSBT/tx parsing. - **PLM access via the Electrum protocol only** (no full node/P2P). Dev bootstrap server: `santantonio.sytes.net:50002` (SSL). -- Auth: Argon2 hashing + JWT (HS256, 24h, **no revocation** — B-34). +- Auth: Argon2 hashing + JWT (HS256, 24h, **no revocation** — B-34). Argon2 costs tens of ms of CPU per call by design, so every async caller goes through `hash_password_async`/`verify_password_async` (`run_in_threadpool`, B-55) — inline it froze the whole process, background tasks included, for the duration of every login. The sync pair stays for tests and scripts. - Secrets: master xprv Fernet-encrypted at rest, encryption key in an env var (never in the DB or git). `validate_runtime_secrets()` (`app/config.py`, called from the lifespan — deliberately *not* a `Settings` validator, so imports and tests need no real secrets) makes the server **refuse to serve** if `JWT_SECRET` < 32 chars or `XPRV_ENCRYPTION_KEY` is empty. An empty `ADMIN_TOKEN` is deliberately non-fatal: `require_admin` then denies everything, i.e. a locked panel, not an open one. - **Operational config lives in the DB, not in env vars**: every business/round parameter is one row of `round_config` (`app/rounds/config.py`), editable live from `/admin` — no redeploy, no restart. Defaults for a fresh instance are column defaults on `RoundConfig` (`app/db/models.py`), *not* `app/config.py`. Only secrets and infra wiring (master key, JWT secret, Electrum hosts, admin token, DB URL) stay in `.env`, since those need a restart anyway. diff --git a/app/api/routes/admin.py b/app/api/routes/admin.py index 81b1d39..ca357b9 100644 --- a/app/api/routes/admin.py +++ b/app/api/routes/admin.py @@ -8,7 +8,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.api.timeutil import isoformat_utc from app.audit.log import write_audit_log -from app.auth.security import hash_password +from app.auth.security import hash_password_async from app.config import settings from app.db.models import AuditLog, BugReport, PendingTransaction, Round, User from app.db.session import get_session @@ -217,7 +217,7 @@ async def reset_user_password( raise HTTPException(status.HTTP_404_NOT_FOUND, "user not found") new_password = secrets.token_urlsafe(12) - user.password_hash = hash_password(new_password) + user.password_hash = await hash_password_async(new_password) # B-34: this endpoint exists precisely for the "account compromised" case — # without bumping token_version, whoever was already logged in (the # attacker, if that's who prompted the reset) stayed logged in on their diff --git a/app/api/routes/users.py b/app/api/routes/users.py index a5d3758..8a84531 100644 --- a/app/api/routes/users.py +++ b/app/api/routes/users.py @@ -6,7 +6,12 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.api.errors import http_error from app.api.timeutil import isoformat_utc from app.auth.dependencies import get_current_user -from app.auth.security import MIN_PASSWORD_LENGTH, create_access_token, hash_password, verify_password +from app.auth.security import ( + MIN_PASSWORD_LENGTH, + create_access_token, + hash_password_async, + verify_password_async, +) from app.db.models import Round, RoundParticipant, User from app.db.session import get_session from app.wallet.balance import compute_pending_balance @@ -59,7 +64,7 @@ async def change_password( """Self-service password change — requires the current password, unlike the admin-only /admin/users/{id}/reset-password (which is for a user who's actually locked out and can't provide it).""" - if not verify_password(body.current_password, user.password_hash): + if not await verify_password_async(body.current_password, user.password_hash): raise http_error( status.HTTP_401_UNAUTHORIZED, "current_password_incorrect", "current password is incorrect" ) @@ -71,7 +76,7 @@ async def change_password( minimum=MIN_PASSWORD_LENGTH, ) - user.password_hash = hash_password(body.new_password) + user.password_hash = await hash_password_async(body.new_password) # B-34: bumping token_version invalidates every token issued before this # point — including this very request's own bearer token, and any an # attacker who knew the old password might be holding. A fresh token is diff --git a/app/auth/routes.py b/app/auth/routes.py index 4fd2cd9..59eba0e 100644 --- a/app/auth/routes.py +++ b/app/auth/routes.py @@ -7,7 +7,12 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.api.client_ip import client_ip as _client_ip from app.api.errors import http_error from app.auth.rate_limit import AuthRateLimiters -from app.auth.security import MIN_PASSWORD_LENGTH, create_access_token, hash_password, verify_password +from app.auth.security import ( + MIN_PASSWORD_LENGTH, + create_access_token, + hash_password_async, + verify_password_async, +) from app.db.models import User from app.db.session import get_session from app.wallet.hd import derive_user_address @@ -74,7 +79,7 @@ async def register( if existing is not None: raise http_error(status.HTTP_409_CONFLICT, "username_taken", "username already taken") - password_hash = hash_password(body.password) + password_hash = await hash_password_async(body.password) for _ in range(_MAX_REGISTER_RETRIES): max_index = await session.scalar(select(func.max(User.derivation_index))) @@ -130,7 +135,7 @@ async def login( raise _rate_limited_error(retry_after) user = await session.scalar(select(User).where(User.username == body.username)) - if user is None or not verify_password(body.password, user.password_hash): + if user is None or not await verify_password_async(body.password, user.password_hash): # Same code path (and therefore the same response) whether the username # doesn't exist or the password is wrong — no enumeration oracle here. limiters.login.record_failure(username_key) diff --git a/app/auth/security.py b/app/auth/security.py index 6b83de4..69c26bb 100644 --- a/app/auth/security.py +++ b/app/auth/security.py @@ -5,6 +5,7 @@ import logging import jwt from argon2 import PasswordHasher from argon2.exceptions import InvalidHashError, VerificationError +from starlette.concurrency import run_in_threadpool from app.config import settings @@ -39,6 +40,28 @@ def verify_password(password: str, password_hash: str) -> bool: return False +# --- B-55: the two Argon2 calls above must never run on the event loop ---------- +# Argon2 is deliberately expensive — tens of milliseconds of CPU per call, by +# design. Called straight from an async handler that stalls the *whole* process +# for that long: every other request, and all six background tasks (scheduler, +# confirmation poller, RBF bumper, listener, both reconcilers). A burst of +# unauthenticated login attempts was therefore a cheap way to delay draws and +# confirmations, not just to slow down logins. The threadpool keeps the cost +# where it belongs — on a worker thread, with the loop free to run everything +# else meanwhile. +# +# The synchronous functions stay: they're what the wrappers call, and what tests +# and scripts (no running loop) use directly. Every async caller must use these. + + +async def hash_password_async(password: str) -> str: + return await run_in_threadpool(hash_password, password) + + +async def verify_password_async(password: str, password_hash: str) -> bool: + return await run_in_threadpool(verify_password, password, password_hash) + + def create_access_token(user_id: int, token_version: int = 0) -> str: expires_at = datetime.now(timezone.utc) + timedelta(minutes=settings.jwt_expire_minutes) # "tv" lets get_current_user (app/auth/dependencies.py) reject a token issued diff --git a/tests/unit/test_security.py b/tests/unit/test_security.py index fa2b073..6458375 100644 --- a/tests/unit/test_security.py +++ b/tests/unit/test_security.py @@ -1,3 +1,5 @@ +import asyncio + from app.auth import security @@ -42,3 +44,59 @@ def test_verify_password_still_rejects_a_wrong_password(): stored = hash_password("correct-horse-battery") assert verify_password("correct-horse-battery", stored) is True assert verify_password("wrong", stored) is False + + +# --- B-55: Argon2 must not run on the event loop -------------------------------- + + +async def _count_loop_ticks_during(coro) -> tuple[object, int]: + """Runs `coro` while a heartbeat task tries to run as often as the event loop + lets it. A blocking call starves the heartbeat completely; a threadpooled one + leaves the loop free the whole time.""" + ticks = 0 + + async def heartbeat() -> None: + nonlocal ticks + while True: + ticks += 1 + await asyncio.sleep(0) + + task = asyncio.create_task(heartbeat()) + await asyncio.sleep(0) # let the heartbeat reach its loop before timing starts + try: + result = await coro + finally: + task.cancel() + return result, ticks + + +async def test_hash_password_async_keeps_the_event_loop_free(): + """Argon2 costs tens of milliseconds of CPU by design. Run inline from an async + handler it froze the whole process for that long — every other request plus all + six background tasks (scheduler, confirmation poller, RBF bumper, listener, both + reconcilers) — which made a burst of unauthenticated login attempts a cheap way + to delay draws and confirmations.""" + hashed, ticks = await _count_loop_ticks_during(security.hash_password_async("s3cret-passphrase")) + + assert security.verify_password("s3cret-passphrase", hashed) + assert ticks > 1 # the loop kept running while the hashing happened + + +async def test_verify_password_async_keeps_the_event_loop_free(): + stored = security.hash_password("correct-horse-battery") + + ok, ticks = await _count_loop_ticks_during( + security.verify_password_async("correct-horse-battery", stored) + ) + + assert ok is True + assert ticks > 1 + + +async def test_verify_password_async_rejects_a_wrong_password(): + """Same answers as the synchronous function it wraps — including the B-13 + unparseable-hash case, which must read as "wrong password", not as an error.""" + stored = security.hash_password("correct-horse-battery") + + assert await security.verify_password_async("wrong", stored) is False + assert await security.verify_password_async("whatever", "not-an-argon2-hash") is False