Skip to content

Commit 39b0804

Browse files
Athroniaethclaude
andcommitted
fix(security): verify hash before status checks to prevent key-state enumeration
OWASP API2:2023: move hash verification before ensure_valid() so a caller with a wrong secret always receives 401 InvalidKey, regardless of whether the key is inactive or expired. Adds regression test and updates mermaid flow diagrams to reflect the STATE_CHECK step after COMPARE. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent a7d3e86 commit 39b0804

File tree

4 files changed

+33
-5
lines changed

4 files changed

+33
-5
lines changed

README.md

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ This library try to follow best practices and relevant RFCs for API key manageme
3434
valid but inactive/expired keys
3535
- **[RFC 6750](https://datatracker.ietf.org/doc/html/rfc6750)**: Supports `Authorization: Bearer <api_key>` header for
3636
key transmission (also supports deprecated `X-API-Key` header and `api_key` query param)
37+
- **[OWASP API2:2023](https://owasp.org/API-Security/editions/2023/en/0xa2-broken-authentication/)**: Hash
38+
verification is performed before status/scope checks to prevent key-state enumeration — a caller with
39+
a wrong secret always receives `401 Invalid`, regardless of whether the key is inactive or expired.
3740

3841
## Installation
3942

@@ -204,6 +207,10 @@ flowchart LR
204207
COMPARE{"`**Compare db api key hash
205208
to received api key hash**`"}:::processNode
206209
210+
STATE_CHECK{"`**Check state & scopes**
211+
_(active? not expired?
212+
required scopes?)_`"}:::processNode
213+
207214
%% ── Outcomes ────────────────────────────────────────────
208215
REJECT(["`🔴 **Reject API Key**`"]):::rejectNode
209216
ACCEPT(["`🟢 **Accept API Key**`"]):::acceptNode
@@ -220,7 +227,8 @@ flowchart LR
220227
221228
%% ── Accept path ─────────────────────────────────────────
222229
CACHED -- "yes" --> ACCEPT
223-
COMPARE -- "equals" --> ACCEPT
230+
COMPARE -- "equals" --> STATE_CHECK
231+
STATE_CHECK -- "valid" --> ACCEPT
224232
ACCEPT -- "`**APIKey.touch()**
225233
_(update last_used_at)_`" --> CACHE_STORE
226234
@@ -231,6 +239,7 @@ flowchart LR
231239
PREFIX_CHECK -- "no" --> REJECT
232240
QUERY_DB -- "not found" --> REJECT
233241
COMPARE -- "not equals" --> REJECT
242+
STATE_CHECK -- "invalid (403)" --> REJECT
234243
235244
%% ── Notes (annotations) ─────────────────────────────────
236245
NOTE_FORMAT["`**Format API Key**

docs/schema.mermaid

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ flowchart LR
3737
COMPARE{"`**Compare db api key hash
3838
to received api key hash**`"}:::processNode
3939

40+
STATE_CHECK{"`**Check state & scopes**
41+
_(active? not expired?
42+
required scopes?)_`"}:::processNode
43+
4044
%% ── Outcomes ────────────────────────────────────────────
4145
REJECT(["`🔴 **Reject API Key**`"]):::rejectNode
4246
ACCEPT(["`🟢 **Accept API Key**`"]):::acceptNode
@@ -53,7 +57,8 @@ flowchart LR
5357

5458
%% ── Accept path ─────────────────────────────────────────
5559
CACHED -- "yes" --> ACCEPT
56-
COMPARE -- "equals" --> ACCEPT
60+
COMPARE -- "equals" --> STATE_CHECK
61+
STATE_CHECK -- "valid" --> ACCEPT
5762
ACCEPT -- "`**APIKey.touch()**
5863
_(update last_used_at)_`" --> CACHE_STORE
5964

@@ -64,6 +69,7 @@ flowchart LR
6469
PREFIX_CHECK -- "no" --> REJECT
6570
QUERY_DB -- "not found" --> REJECT
6671
COMPARE -- "not equals" --> REJECT
72+
STATE_CHECK -- "invalid (403)" --> REJECT
6773

6874
%% ── Notes (annotations) ─────────────────────────────────
6975
NOTE_FORMAT["`**Format API Key**

src/keyshield/services/base.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -472,19 +472,19 @@ async def _verify_entity(self, entity: ApiKey, key_secret: str, required_scopes:
472472
The entity with updated last_used_at.
473473
474474
Raises:
475+
InvalidKey: If the hash does not match.
475476
KeyInactive: If the key is disabled.
476477
KeyExpired: If the key is expired.
477-
InvalidKey: If the hash does not match.
478478
InvalidScopes: If scopes are insufficient.
479479
"""
480480
# Todo: IDK if this line ise usefully
481481
# assert entity.key_hash is not None, "key_hash must be set for existing API keys" # nosec B101
482482

483-
entity.ensure_valid(scopes=required_scopes)
484-
485483
if not self._hasher.verify(entity.key_hash, key_secret):
486484
raise InvalidKey("API key is invalid (hash mismatch)")
487485

486+
entity.ensure_valid(scopes=required_scopes)
487+
488488
return await self.touch(entity)
489489

490490
def _get_parts(self, api_key: str) -> ParsedApiKey:

tests/unit/test_service.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,19 @@ async def test_verify_inactive_raises(self, service: ApiKeyService):
260260
with pytest.raises(KeyInactive):
261261
await service.verify_key(full_key)
262262

263+
@pytest.mark.asyncio
264+
async def test_verify_inactive_with_wrong_secret_raises_invalid_key(self, service: ApiKeyService):
265+
"""verify_key() raises InvalidKey (not KeyInactive) for wrong secret on inactive key.
266+
267+
Regression test for OWASP API2:2023: the caller must not be able to enumerate
268+
key state (active/inactive) without knowing the correct secret.
269+
"""
270+
entity, _ = await service.create(name="inactive", is_active=False)
271+
bad_key = _full_key(entity.key_id, key_secret_factory())
272+
273+
with pytest.raises(InvalidKey):
274+
await service.verify_key(bad_key)
275+
263276
@pytest.mark.asyncio
264277
async def test_verify_expired_raises(self, service: ApiKeyService):
265278
"""verify_key() raises KeyExpired for expired key."""

0 commit comments

Comments
 (0)