From 7c28e02e0996e2d2a8e98e780c089859a32c080f Mon Sep 17 00:00:00 2001 From: Grant Ramsay Date: Sun, 10 Aug 2025 12:41:35 +0100 Subject: [PATCH 1/4] feat: implement HMAC-SHA256 for API key hashing and add corresponding tests Signed-off-by: Grant Ramsay --- app/managers/api_key.py | 15 ++++++++-- pyproject.toml | 1 + tests/unit/test_admin.py | 1 - tests/unit/test_api_key_manager.py | 46 ++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 3 deletions(-) diff --git a/app/managers/api_key.py b/app/managers/api_key.py index 32f5a542..d23bebcd 100644 --- a/app/managers/api_key.py +++ b/app/managers/api_key.py @@ -1,6 +1,7 @@ """Define the API Key Manager.""" import hashlib +import hmac import secrets from typing import Optional from uuid import UUID @@ -8,6 +9,7 @@ from fastapi import Depends, HTTPException, Request, status from sqlalchemy.ext.asyncio import AsyncSession +from app.config.settings import get_settings from app.database.db import get_database from app.database.helpers import ( add_new_api_key_, @@ -36,8 +38,17 @@ class ApiKeyManager: @staticmethod def _hash_key(key: str) -> str: - """Hash an API key.""" - return hashlib.sha256(key.encode()).hexdigest() + """Hash an API key using HMAC-SHA256. + + This intentionally uses HMAC-SHA256 rather than a slow, memory-hard + password hash (e.g., bcrypt or argon2) because our API keys are + generated with `secrets.token_urlsafe(32)`, giving ~192 bits of + entropy. They are not human-chosen or guessable, so brute-forcing + them is computationally infeasible. HMAC also prevents + length-extension attacks possible with raw SHA256 hashing. + """ + secret_key = get_settings().secret_key.encode() + return hmac.new(secret_key, key.encode(), hashlib.sha256).hexdigest() @classmethod def generate_key(cls) -> str: diff --git a/pyproject.toml b/pyproject.toml index f612d2d9..0d03c52a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -180,6 +180,7 @@ convention = "google" "TD003", "FIX002", "RUF012", + "SLF001", # sometimes tests need to access private members ] "app/managers/auth.py" = ["ERA001"] "app/resources/auth.py" = ["ERA001"] diff --git a/tests/unit/test_admin.py b/tests/unit/test_admin.py index 160dc3a3..86540dbb 100644 --- a/tests/unit/test_admin.py +++ b/tests/unit/test_admin.py @@ -1,6 +1,5 @@ """Unit tests for the admin interface.""" -# ruff: noqa: SLF001 import json from datetime import datetime, timedelta, timezone diff --git a/tests/unit/test_api_key_manager.py b/tests/unit/test_api_key_manager.py index 8f2a82b1..07a9cf4b 100644 --- a/tests/unit/test_api_key_manager.py +++ b/tests/unit/test_api_key_manager.py @@ -1,5 +1,7 @@ """Test the API Key Manager functionality.""" +import hashlib +import hmac from uuid import uuid4 import pytest @@ -318,3 +320,47 @@ async def test_api_key_auth_user_not_found_in_db_no_auto_error( auth = ApiKeyAuth(auto_error=False) result = await auth(request=mock_req, db=test_db) assert result is None + + def test_hash_key_uses_hmac(self, mocker) -> None: + """Test that API key hashing uses HMAC-SHA256 with secret key.""" + # Mock the settings + mock_settings = mocker.patch("app.managers.api_key.get_settings") + test_secret = "test_secret_key_12345" # noqa: S105 + mock_settings.return_value.secret_key = test_secret + + test_key = "pk_test_key_123456789" + result = ApiKeyManager._hash_key(test_key) + + # Calculate expected HMAC manually + expected = hmac.new( + test_secret.encode(), test_key.encode(), hashlib.sha256 + ).hexdigest() + + assert result == expected + assert len(result) == 64 # SHA256 hex digest length # noqa: PLR2004 + + def test_hash_key_different_secrets_different_hashes(self, mocker) -> None: + """Test that different secret keys produce different hashes.""" + test_key = "pk_same_key_123" + + # First hash with secret 1 + mock_settings = mocker.patch("app.managers.api_key.get_settings") + mock_settings.return_value.secret_key = "secret1" # noqa: S105 + hash1 = ApiKeyManager._hash_key(test_key) + + # Second hash with secret 2 + mock_settings.return_value.secret_key = "secret2" # noqa: S105 + hash2 = ApiKeyManager._hash_key(test_key) + + assert hash1 != hash2 + + def test_hash_key_deterministic(self, mocker) -> None: + """Test that the same key produces the same hash consistently.""" + mock_settings = mocker.patch("app.managers.api_key.get_settings") + mock_settings.return_value.secret_key = "consistent_secret" # noqa: S105 + + test_key = "pk_deterministic_test" + hash1 = ApiKeyManager._hash_key(test_key) + hash2 = ApiKeyManager._hash_key(test_key) + + assert hash1 == hash2 From 79014cec0e0354269688042d79864cf01acfa62c Mon Sep 17 00:00:00 2001 From: Grant Ramsay Date: Sun, 10 Aug 2025 12:48:15 +0100 Subject: [PATCH 2/4] ignore the codeql error - it will be secure d/t entropy and HMAC Signed-off-by: Grant Ramsay --- app/managers/api_key.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/managers/api_key.py b/app/managers/api_key.py index d23bebcd..55a1760a 100644 --- a/app/managers/api_key.py +++ b/app/managers/api_key.py @@ -37,6 +37,7 @@ class ApiKeyManager: KEY_LENGTH = 32 @staticmethod + # codeql[py/weak-sensitive-data-hashing] See comment below def _hash_key(key: str) -> str: """Hash an API key using HMAC-SHA256. From 9a3f05e1c89bee75ca8e17498d7b1e7169367ef4 Mon Sep 17 00:00:00 2001 From: Grant Ramsay Date: Sun, 10 Aug 2025 13:12:41 +0100 Subject: [PATCH 3/4] docs: note the breaking change in this PR Signed-off-by: Grant Ramsay --- README.md | 8 ++++++++ SECURITY.md | 2 +- docs/important.md | 13 ++++++++++++- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0954594d..0037bd77 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,7 @@ Documentation for this project is now availiable on it's own page at finished will include full usage information and how-to's. - [Important note on Versioning](#important-note-on-versioning) +- [Breaking Changes](#breaking-changes) - [Changes from version 0.4.x](#changes-from-version-04x) - [Functionality](#functionality) - [Installation](#installation) @@ -50,6 +51,13 @@ previous version. This will be in the form of a `.patch` file which can be applied to their project using the `git apply` command. This will be documented in the release notes. +## Breaking Changes + +There will be breaking changes implemented from time to time, as the template is +still evolving. These may be due to security issues or changes in philosophy. +These can always be found [here](https://api-template.seapagan.net/important/) +on the website. + ## Changes from version 0.4.x Starting from version 0.5.0, the template has been refactored to use SQLAlchemy diff --git a/SECURITY.md b/SECURITY.md index ded5683f..8dfcf5f7 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -4,7 +4,7 @@ | Version | Supported | | ------- | ------------------ | -| >=0.6.0 | :white_check_mark: | +| >=0.7.1 | :white_check_mark: | Until we reach a 1.0 milestone, we will generally only support the latest release of the project. If you are having issues with an older version, please diff --git a/docs/important.md b/docs/important.md index e9c9006c..8973eb7a 100644 --- a/docs/important.md +++ b/docs/important.md @@ -8,6 +8,17 @@ API. None. +## Breaking changes in 0.7.1 + +### Modified the API Key Hashing method + +The API key is **NEVER** stored in the database, however a **hashed** version of +this is so that we can authenticate. Previously it used a plain SHA256 +algorythm, and has now been switched to using `HMAC` in conjunction with SHA256 +instead. This allows using the `SECRET_KEY` already set to make the API keys +more secure. As a result, **any existing API Keys are now invalid and will need to +be deleted and regenerated**. + ## Breaking Changes in 0.7.0 ### Modified the Authentication backend @@ -37,7 +48,7 @@ Several function signatures have changed, generally to fix boolean inconsistencies. Boolean parameters should be passed as named parameters instead of positional parameters. This is to make the code more readable and maintainable. The `UserManager.set_ban_status` function is one of these changes -that causes a breaking change. However, this method is only called from and API +that causes a breaking change. However, this method is only called from an API endpoint for the moment, so it should not affect any existing code that depends on it unless you are using it directly in your code. From 63cebccbdcf773edf787d1b6f995097b202195e6 Mon Sep 17 00:00:00 2001 From: Grant Ramsay Date: Sun, 10 Aug 2025 16:12:03 +0100 Subject: [PATCH 4/4] docs: remove placeholder for breaking changes in 0.7.1 Signed-off-by: Grant Ramsay --- docs/important.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/important.md b/docs/important.md index 8973eb7a..55e6bdc2 100644 --- a/docs/important.md +++ b/docs/important.md @@ -6,10 +6,6 @@ API. ## Breaking Changes in `HEAD` -None. - -## Breaking changes in 0.7.1 - ### Modified the API Key Hashing method The API key is **NEVER** stored in the database, however a **hashed** version of