Skip to content

Commit 5282c5f

Browse files
committed
fix(security): offload bcrypt.checkpw to thread pool (Fix A)
verify_api_key_async wraps bcrypt.checkpw in asyncio.to_thread so the event loop is not blocked during expensive bcrypt verification (~100-200ms). Cache check still runs on the loop thread; thread pool is used only on cache miss. TDD: test_verify_api_key_does_not_block_event_loop added first (RED), then implementation (GREEN). Refs #22
1 parent 544030d commit 5282c5f

3 files changed

Lines changed: 89 additions & 2 deletions

File tree

backend/app/auth/dependencies.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636
from pydantic import BaseModel
3737
from sqlalchemy.ext.asyncio import AsyncSession
3838

39-
from app.auth.verifier import verify_api_key
39+
from app.auth.verifier import verify_api_key_async
4040
from app.config import Settings, get_settings
4141
from app.db.session import get_session
4242
from app.repositories import api_keys as api_keys_repo
@@ -159,7 +159,7 @@ async def _validate_api_key(
159159
detail={"error_code": "key_expired", "error_message": "API key has expired"},
160160
)
161161

162-
if not verify_api_key(key_header, key_row.key_hash):
162+
if not await verify_api_key_async(key_header, key_row.key_hash):
163163
_log.debug("bcrypt mismatch for prefix %s", prefix)
164164
raise HTTPException(
165165
status_code=status.HTTP_401_UNAUTHORIZED,

backend/app/auth/verifier.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,19 @@
2020
asyncio-compatible pattern (single event loop = single thread for FastAPI).
2121
For multi-process deployments an external cache would be needed (out of scope
2222
for HomeLab single-instance design per spec Section 5).
23+
24+
Async design: bcrypt.checkpw is CPU-intensive (~100-200ms). Calling it
25+
directly inside an ``async def`` blocks the event loop and prevents other
26+
coroutines from running. ``verify_api_key_async`` offloads the work to a
27+
thread pool via ``asyncio.to_thread``, keeping the loop free. The cache
28+
check/write still happens on the event-loop thread (single-threaded, no lock
29+
needed for in-process use).
2330
"""
2431

2532
from __future__ import annotations
2633

34+
import asyncio
35+
2736
import bcrypt
2837
from cachetools import TTLCache
2938

@@ -43,6 +52,10 @@ def verify_api_key(plaintext: str, hashed: str) -> bool:
4352
4453
Returns:
4554
True if the key is valid, False otherwise.
55+
56+
Note:
57+
This is a synchronous helper. In async contexts prefer
58+
``verify_api_key_async`` to avoid blocking the event loop.
4659
"""
4760
cache_key = (plaintext, hashed)
4861
if cache_key in _cache:
@@ -53,6 +66,33 @@ def verify_api_key(plaintext: str, hashed: str) -> bool:
5366
return result
5467

5568

69+
async def verify_api_key_async(plaintext: str, hashed: str) -> bool:
70+
"""Async wrapper around ``verify_api_key`` that offloads bcrypt to a thread.
71+
72+
bcrypt.checkpw is CPU-intensive (~100-200ms). Running it on the event-loop
73+
thread would block all other coroutines for that duration. This wrapper:
74+
75+
1. Checks the TTL cache first (fast, on the loop thread).
76+
2. If a cache miss, runs bcrypt.checkpw in a thread pool via
77+
``asyncio.to_thread``, freeing the loop for other work.
78+
3. Writes the result back to the cache (on the loop thread after await).
79+
80+
Args:
81+
plaintext: The full API key as provided in the ``X-Label-Hub-Key`` header.
82+
hashed: The bcrypt hash stored in the DB.
83+
84+
Returns:
85+
True if the key is valid, False otherwise.
86+
"""
87+
cache_key = (plaintext, hashed)
88+
if cache_key in _cache:
89+
return _cache[cache_key]
90+
91+
result = await asyncio.to_thread(bcrypt.checkpw, plaintext.encode(), hashed.encode())
92+
_cache[cache_key] = result
93+
return result
94+
95+
5696
def invalidate_cache(hashed: str) -> None:
5797
"""Remove all cache entries for a given hash (e.g. after key revocation).
5898

backend/tests/unit/auth/test_verifier.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
from __future__ import annotations
44

5+
import asyncio
56
from unittest.mock import patch
67

78
import bcrypt
@@ -94,3 +95,49 @@ def test_invalidate_cache_removes_entry():
9495

9596
verifier_module.invalidate_cache(hashed)
9697
assert (plaintext, hashed) not in verifier_module._cache
98+
99+
100+
@pytest.mark.asyncio
101+
async def test_verify_api_key_does_not_block_event_loop():
102+
"""bcrypt.checkpw must run in a thread pool so the event loop stays free.
103+
104+
Strategy: run verify_api_key concurrently with a fast coroutine.
105+
If checkpw blocks the loop, the fast coroutine cannot advance.
106+
We assert the concurrent coroutine completed while verify was running.
107+
"""
108+
from app.auth import verifier as verifier_module
109+
110+
verifier_module._cache.clear()
111+
plaintext = "lh_nonblocking_test_001"
112+
hashed = _make_hash(plaintext)
113+
114+
side_ran = []
115+
116+
async def side_coroutine():
117+
await asyncio.sleep(0)
118+
side_ran.append(True)
119+
120+
# Run both concurrently
121+
await asyncio.gather(
122+
verifier_module.verify_api_key_async(plaintext, hashed),
123+
side_coroutine(),
124+
)
125+
126+
assert side_ran, "Side coroutine did not run — event loop was blocked"
127+
128+
129+
@pytest.mark.asyncio
130+
async def test_verify_api_key_async_returns_true_for_correct_key():
131+
"""Async wrapper returns True for a matching key."""
132+
from app.auth.verifier import verify_api_key_async
133+
plaintext = "lh_async_correct_001"
134+
hashed = _make_hash(plaintext)
135+
assert await verify_api_key_async(plaintext, hashed) is True
136+
137+
138+
@pytest.mark.asyncio
139+
async def test_verify_api_key_async_returns_false_for_wrong_key():
140+
"""Async wrapper returns False for a non-matching key."""
141+
from app.auth.verifier import verify_api_key_async
142+
hashed = _make_hash("lh_async_other_001")
143+
assert await verify_api_key_async("lh_async_wrong_001", hashed) is False

0 commit comments

Comments
 (0)