Add optional API key authentication for /checkin/ and /verify/ endpoints#152
Add optional API key authentication for /checkin/ and /verify/ endpoints#152rodchristiansen wants to merge 1 commit intograhamgilbert:masterfrom
Conversation
- Add APIKeyAuthMiddleware in server/middleware.py - Middleware checks X-API-Key header against CRYPT_API_KEY env var - Returns 401 Unauthorized if key is missing/invalid - Backward compatible: if CRYPT_API_KEY not set, endpoints remain open - Uses timing-safe comparison to prevent timing attacks - Update system_settings.py to include middleware - Add documentation to README
There was a problem hiding this comment.
Pull request overview
Adds optional shared-secret API key protection for the unauthenticated escrow endpoints to reduce risk of unauthorized /checkin/ and /verify/ calls, while keeping existing deployments backward compatible when the key isn’t configured.
Changes:
- Introduces
APIKeyAuthMiddlewarethat enforcesX-API-Keyon/checkin/and/verify/whenCRYPT_API_KEYis set. - Registers the middleware in Django
MIDDLEWARE. - Documents configuration and client usage in
README.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/middleware.py | Adds new middleware implementing optional API key authentication for escrow endpoints. |
| fvserver/system_settings.py | Enables the middleware in the global middleware chain. |
| README.md | Documents enabling/configuring API key auth and how clients should send the header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Fall back to settings | ||
| if hasattr(settings, 'CRYPT_API_KEY'): | ||
| return getattr(settings, 'CRYPT_API_KEY', '').strip() | ||
|
|
There was a problem hiding this comment.
_get_api_key() assumes settings.CRYPT_API_KEY is a string and calls .strip() unconditionally when the attribute exists. If a deployment sets CRYPT_API_KEY = None (or any non-str), this will raise at startup and break all requests. Consider handling None/non-string values safely (e.g., coerce to ''/str before strip, or return None when falsy).
| # Fall back to settings | |
| if hasattr(settings, 'CRYPT_API_KEY'): | |
| return getattr(settings, 'CRYPT_API_KEY', '').strip() | |
| # Fall back to settings | |
| if hasattr(settings, 'CRYPT_API_KEY'): | |
| raw_key = getattr(settings, 'CRYPT_API_KEY', None) | |
| if not raw_key: | |
| return None | |
| if isinstance(raw_key, str): | |
| api_key = raw_key.strip() | |
| else: | |
| # Coerce non-string values to string safely | |
| api_key = str(raw_key).strip() | |
| return api_key or None |
| # Get API key from request header | ||
| request_api_key = request.META.get(self.API_KEY_HEADER) | ||
|
|
||
| if not request_api_key: | ||
| logger.warning( | ||
| "API request to %s without API key from %s", | ||
| request.path, | ||
| request.META.get('REMOTE_ADDR', 'unknown') | ||
| ) | ||
| return JsonResponse( | ||
| {'error': 'API key required. Include X-API-Key header.'}, | ||
| status=401 | ||
| ) | ||
|
|
||
| # Validate API key (constant-time comparison to prevent timing attacks) | ||
| if not hmac.compare_digest(request_api_key, self.api_key): | ||
| logger.warning( |
There was a problem hiding this comment.
The incoming header value isn’t normalized before comparison. A client/proxy can include leading/trailing whitespace in X-API-Key, which will cause a mismatch even when the key is otherwise correct. Consider stripping the request-provided key (and possibly rejecting empty-after-strip) before calling compare_digest().
|
|
||
| if self.api_key: | ||
| logger.info("API key authentication enabled for /checkin/ and /verify/ endpoints") | ||
| else: | ||
| logger.warning( | ||
| "No CRYPT_API_KEY configured. API endpoints are UNPROTECTED. " | ||
| "Set CRYPT_API_KEY environment variable to enable authentication." | ||
| ) |
There was a problem hiding this comment.
This middleware is always installed via MIDDLEWARE, and when CRYPT_API_KEY is not set it logs a WARNING that endpoints are "UNPROTECTED". In environments that intentionally run without an API key (backward compatibility), this can create noisy/alert-worthy logs on every startup. Consider lowering this to INFO/DEBUG or only warning when an explicit setting indicates API key auth is required.
| if self.api_key: | |
| logger.info("API key authentication enabled for /checkin/ and /verify/ endpoints") | |
| else: | |
| logger.warning( | |
| "No CRYPT_API_KEY configured. API endpoints are UNPROTECTED. " | |
| "Set CRYPT_API_KEY environment variable to enable authentication." | |
| ) | |
| # Optional setting to indicate that API key auth is required and absence is a misconfiguration. | |
| self.require_api_key = getattr(settings, 'REQUIRE_CRYPT_API_KEY_AUTH', False) | |
| if self.api_key: | |
| logger.info("API key authentication enabled for /checkin/ and /verify/ endpoints") | |
| else: | |
| if self.require_api_key: | |
| logger.warning( | |
| "No CRYPT_API_KEY configured while REQUIRE_CRYPT_API_KEY_AUTH is True. " | |
| "API endpoints are UNPROTECTED and this is likely a misconfiguration. " | |
| "Set CRYPT_API_KEY environment variable to enable authentication." | |
| ) | |
| else: | |
| logger.info( | |
| "No CRYPT_API_KEY configured. API endpoints are UNPROTECTED " | |
| "(backward-compatible mode). Set CRYPT_API_KEY environment " | |
| "variable to enable authentication." | |
| ) |
| def __call__(self, request): | ||
| # Only check protected API endpoints | ||
| if self._is_protected_path(request.path): | ||
| # If no API key is configured, allow request (backward compatible) | ||
| if not self.api_key: | ||
| return self.get_response(request) | ||
|
|
||
| # Get API key from request header | ||
| request_api_key = request.META.get(self.API_KEY_HEADER) | ||
|
|
||
| if not request_api_key: | ||
| logger.warning( | ||
| "API request to %s without API key from %s", | ||
| request.path, | ||
| request.META.get('REMOTE_ADDR', 'unknown') | ||
| ) | ||
| return JsonResponse( | ||
| {'error': 'API key required. Include X-API-Key header.'}, | ||
| status=401 | ||
| ) | ||
|
|
||
| # Validate API key (constant-time comparison to prevent timing attacks) | ||
| if not hmac.compare_digest(request_api_key, self.api_key): | ||
| logger.warning( | ||
| "Invalid API key for %s from %s", | ||
| request.path, | ||
| request.META.get('REMOTE_ADDR', 'unknown') | ||
| ) | ||
| return JsonResponse( | ||
| {'error': 'Invalid API key.'}, | ||
| status=403 | ||
| ) | ||
|
|
||
| logger.debug("API key validated for %s", request.path) | ||
|
|
||
| return self.get_response(request) |
There was a problem hiding this comment.
The new auth behavior is security-sensitive and currently has no automated tests. Since the repo already has Django TestCase coverage in server/tests.py, it would be good to add tests for: (1) when CRYPT_API_KEY is unset, requests to /checkin/ and /verify/ succeed as before; (2) when set, missing key returns 401; (3) wrong key returns 403; (4) correct key passes through.
|
Thanks for this, but we’re moving away from the python codebase in #149 |
Adds an optional
APIKeyAuthMiddlewarethat protects the/checkin/and/verify/endpoints with a shared API key. Guards against unauthorized FileVault key escrow from unknown clients.How it works
Set
CRYPT_API_KEYin the environment to enable. Clients must include the key in theX-API-Keyheader:curl -X POST https://crypt.example.com/checkin/ \ -H "X-API-Key: your-secret-key" \ -d "serial=ABC123&recovery_password=..."Generate a key:
python3 -c "import secrets; print(secrets.token_urlsafe(32))"Changes
server/middleware.py— newAPIKeyAuthMiddleware; useshmac.compare_digestfor constant-time comparison to prevent timing attacksfvserver/system_settings.py— adds middleware toMIDDLEWAREREADME.md— documents configuration, client usage, and key generationBackward compatibility
If
CRYPT_API_KEYis not set, the endpoints remain fully open — no change in behavior for existing deployments.