diff --git a/agentkit/api/auth.py b/agentkit/api/auth.py index d7d726c..5146271 100644 --- a/agentkit/api/auth.py +++ b/agentkit/api/auth.py @@ -1,5 +1,6 @@ """API key authentication dependency validating X-API-Key header.""" +import hmac import logging from fastapi import Depends, Request, Security @@ -23,8 +24,28 @@ async def verify_api_key( api_key: str | None = Security(api_key_header), settings: Settings = Depends(get_current_settings), ) -> str: - """Validate client API key from X-API-Key header against configured authorized keys.""" - if not api_key or api_key not in settings.api_keys_list: + """Validate client API key from X-API-Key header against configured authorized keys. + + Normalises the presented key (stripping whitespace) and uses constant-time comparison + (`hmac.compare_digest`) across all configured keys to eliminate timing oracles. + """ + if not api_key: logger.warning("Unauthorized API request with missing or invalid key.") raise AuthenticationError("Invalid or missing API key.") - return api_key + + normalized_key = api_key.strip() + if not normalized_key: + logger.warning("Unauthorized API request with missing or invalid key.") + raise AuthenticationError("Invalid or missing API key.") + + presented_bytes = normalized_key.encode("utf-8") + matched = False + for valid_key in settings.api_keys_list: + if hmac.compare_digest(presented_bytes, valid_key.encode("utf-8")): + matched = True + + if not matched: + logger.warning("Unauthorized API request with missing or invalid key.") + raise AuthenticationError("Invalid or missing API key.") + + return normalized_key diff --git a/docs/PROGRESS.md b/docs/PROGRESS.md index 2c7b115..c12d868 100644 --- a/docs/PROGRESS.md +++ b/docs/PROGRESS.md @@ -61,6 +61,7 @@ This document tracks execution progress across all backlog tasks, including pull | AUD-09 | AUD | Live End-to-End Infrastructure Verification & Documentation Proof | `docs/e2e-verification-report` | #53 | 3 | Done | Standardized model, verified live stack, and documented in E2E_REPORT.md | | AUD-10 | AUD | Rate Limiter Outage Fallback & Provider-Valid Session History | `fix/ratelimit-and-session-repair` | #56 | 3 | Done | In-memory fallback on Redis connection failures; synthetic tool results repair aborted sessions | | M13-T06 | M13 | Whole-Project Audit Remediation | `fix/audit-hardening` | #47 | 3 | Done | Fail-closed config and LLM resolution, SSRF post-connect check, atomic fallback | +| AUDIT-17 | AUD | Constant-Time & Normalised API Key Verification | `fix/api-key-constant-time-compare` | Pending | 2 | Done | Normalise presented keys and verify against authorized keys via constant-time hmac.compare_digest | --- @@ -407,3 +408,10 @@ This document tracks execution progress across all backlog tasks, including pull - **PR**: #47 - **Commits**: 3 - **Notes**: Remediated 17 findings from a full-codebase audit, rebased onto the AUD-01/AUD-04/AUD-05 follow-ups. Critical: removed the `ak_test_key_12345` default from `Settings.API_KEYS` and made the field required; replaced the `FakeLLMClient` fallback in `get_llm_client` with a `ServiceUnavailableError` (503) boundary around provider construction; kept the AUD-04 factory-level `RetryingLLMClient` wrapping and made it configurable through new `LLM_MAX_RETRIES`/`LLM_RETRY_BASE_DELAY_S`/`LLM_RETRY_MAX_DELAY_S` settings; made the per-request `Agent` assembly's override explicit as `app.state.agent_override`; forwarded `get_run_trace`/`list_runs` through `TieredMemoryStore`; moved `http_fetch` DNS off the event loop with `asyncio.to_thread`, added a post-connect `server_addr` check against the validated addresses to close the DNS-rebinding TOCTOU window, and replaced full-body buffering with a byte-budgeted stream; replaced the rate limiter's non-scripting fallback with `WATCH`/`MULTI`/`EXEC`, leaving the AUD-10 in-memory fallback for Redis outages intact. Moderate: corrected the `RedisMemoryStore.truncate_messages` off-by-one that returned `max_messages + 1` entries when a system prompt was present; switched generated `tool_call_id`s to full UUIDs; wrapped response parsing in both LLM adapters inside the error boundary so malformed responses and malformed tool-argument JSON raise `LLMProviderError`; guarded `RetryingLLMClient.__getattr__` against recursion on a partially constructed instance; sanitized `/health` details so the unauthenticated endpoint no longer echoes driver error strings; made `sql_readonly` mask string literals and quoted identifiers before its keyword scan, allow `#>`/`#>>`/`#-`, apply the row limit in SQL and push `statement_timeout` to PostgreSQL. Minor: stopped logging raw run goals at INFO (length only, content at DEBUG); replaced `split(";")` in `scripts/seed.py` with a statement-aware splitter handling literals, comments and dollar-quotes; bounded calculator exponentiation by result bit length. Added `tests/unit/test_audit_regressions.py` and a real-Redis concurrency regression test pinning the fallback fix. Recorded in ADR-009, ADR-010, ADR-011. Suite: 298 passed, 25 skipped; coverage 92.86% on core/tools/llm; `ruff check`, `ruff format --check` and `mypy agentkit tests` all clean. + +### AUDIT-17: Constant-Time & Normalised API Key Verification +- **Status**: Done +- **Branch**: `fix/api-key-constant-time-compare` +- **Issue**: #74 +- **Commits**: 2 +- **Notes**: Remediated timing oracle in API key verification (`verify_api_key`) in `agentkit/api/auth.py`. Replaced the short-circuiting `api_key in settings.api_keys_list` check with constant-time matching (`hmac.compare_digest`) across all configured keys. Presented keys are normalised (stripped) identical to configured keys, preventing whitespace drift and ensuring uniform rate-limiting and logging hygiene. Added regression tests in `tests/unit/test_api_auth.py` verifying direct and endpoint integration behaviors. diff --git a/tests/unit/test_api_auth.py b/tests/unit/test_api_auth.py index fc3f13e..b47b746 100644 --- a/tests/unit/test_api_auth.py +++ b/tests/unit/test_api_auth.py @@ -28,6 +28,25 @@ async def test_verify_api_key_direct() -> None: with pytest.raises(AuthenticationError): await verify_api_key(api_key="wrong_key", settings=settings) + # Whitespace-padded valid key normalises and succeeds identically to unpadded + assert await verify_api_key(api_key=" valid_key_1 ", settings=settings) == "valid_key_1" + assert await verify_api_key(api_key="\tvalid_key_2\n", settings=settings) == "valid_key_2" + + # Whitespace-only key raises AuthenticationError + with pytest.raises(AuthenticationError): + await verify_api_key(api_key=" ", settings=settings) + + # Non-ASCII / unicode invalid key raises AuthenticationError without crashing + with pytest.raises(AuthenticationError): + await verify_api_key(api_key="invalid_utf8_🔑", settings=settings) + + # Unicode key matching succeeds when configured + unicode_settings = Settings(API_KEYS="unicode_key_🔑") + assert ( + await verify_api_key(api_key=" unicode_key_🔑 ", settings=unicode_settings) + == "unicode_key_🔑" + ) + def test_api_key_auth_endpoint_integration() -> None: settings = Settings(API_KEYS="secret_token_123") @@ -66,3 +85,8 @@ def public_route() -> dict[str, str]: resp_valid = client.get("/protected", headers={"X-API-Key": "secret_token_123"}) assert resp_valid.status_code == 200 assert resp_valid.json() == {"status": "authorized"} + + # Protected route with whitespace-padded valid header -> 200 (identical behavior) + resp_padded = client.get("/protected", headers={"X-API-Key": " secret_token_123 "}) + assert resp_padded.status_code == 200 + assert resp_padded.json() == {"status": "authorized"}