Skip to content

Use real constant-time comparisons for API key auth #67

Description

@Justinabox

Summary

APIKeyAuth.middleware() appears to intend constant-time API-key comparison, but it calls secrets.compare_digest(key, key), which always compares the supplied key to itself. The real authorization decision is then key not in self._keys, a normal set-membership lookup. Functionality still accepts/rejects keys, but the constant-time hardening is ineffective and future maintainers may think this path is side-channel resistant when it is not.

Affected files / lines

  • server.py:68-70if not secrets.compare_digest(key, key) or key not in self._keys: compares the candidate with itself instead of comparing against stored API keys.
  • tests/test_api_auth.py:52-79 covers valid/invalid behavior, but not that stored-key comparison uses compare_digest correctly.

Evidence

Baseline health from this scout run:

git diff --check -> exit 0
PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q
379 passed in 4.27s

Minimal no-hardware probe showing behavior still works but the middleware source contains the ineffective self-compare:

PYTHONPATH=. uv run --no-project --with aiohttp --with pytest-aiohttp --with pyserial-asyncio python - <<'PY'
import asyncio, inspect
from aiohttp import web
from aiohttp.test_utils import TestClient, TestServer
from server import APIKeyAuth

async def main():
    auth = APIKeyAuth(api_keys=['abcd1234'], rate_limit=3, rate_window=60)
    app = web.Application(middlewares=[auth.middleware])
    async def ok(request):
        return web.json_response({'ok': True})
    app.router.add_get('/', ok)
    client = TestClient(TestServer(app)); await client.start_server()
    try:
        for key in ['wrong000', 'abcd1234']:
            resp = await client.get('/', headers={'Authorization': f'Bearer {key}'})
            print(key, resp.status, await resp.text())
    finally:
        await client.close()
    source = inspect.getsource(APIKeyAuth.middleware)
    print('contains_compare_digest_key_key:', 'compare_digest(key, key)' in source)
    print('membership_check_present:', 'key not in self._keys' in source)
asyncio.run(main())
PY

Actual output:

wrong000 403 {"error": "Invalid API key."}
abcd1234 200 {"ok": true}
contains_compare_digest_key_key: True
membership_check_present: True

Expected behavior

If the middleware uses secrets.compare_digest, the authorization decision should compare the provided key against configured keys in a constant-time style, for example any(secrets.compare_digest(key, candidate) for candidate in self._keys), with care around length/timing characteristics and without logging keys.

Actual behavior

The compare_digest call is a no-op for security because any string compares equal to itself; membership in the set performs the actual decision.

Duplicate check

No existing issue/PR matched these searches:

gh issue list --repo Justinabox/Callstack --state all --search 'APIKeyAuth compare_digest timing membership in:title,body'
gh issue list --repo Justinabox/Callstack --state all --search 'constant-time API key auth compare digest in:title,body'
gh pr list --repo Justinabox/Callstack --state all --search 'APIKeyAuth compare_digest constant time'

Suggested fix direction

  • Replace the self-compare plus set membership with a helper that checks candidate keys using secrets.compare_digest against configured keys.
  • Keep the public error message generic (Invalid API key.) and do not log candidate/stored keys.
  • Add a source/behavior regression test that would fail if compare_digest(key, key) reappears, plus existing valid/invalid auth tests.

Acceptance criteria

  • Invalid keys still return 403; valid keys still pass.
  • The auth decision no longer contains compare_digest(key, key) or equivalent self-comparison.
  • Tests verify the middleware uses the stored-key comparison helper and preserves rate-limit behavior.

Verification gates

git diff --check
PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/test_api_auth.py -q
PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions