feat: add API key authentication to prover server#1880
Conversation
Enables authentication via X-API-Key or Bearer token headers. Public /health endpoint remains accessible while all other routes require authentication when PROVER_API_KEY env var is set.
WalkthroughAPI key authentication was added to the prover server. A middleware was implemented to enforce API key checks on protected endpoints, excluding Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant AuthMiddleware
participant Handler
Client->>Server: HTTP Request (/prove or /health)
Server->>AuthMiddleware: Pass request
alt /health endpoint
AuthMiddleware->>Handler: Bypass authentication
else Protected endpoint
AuthMiddleware->>AuthMiddleware: Check API key in headers
alt API key valid or not required
AuthMiddleware->>Handler: Forward request
else API key invalid/missing
AuthMiddleware->>Client: Respond 401 Unauthorized
end
end
Handler->>Client: Endpoint response
Estimated code review effort3 (~40 minutes) Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
sergeytimoshin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prover/server/scripts/test_auth.sh(1 hunks)prover/server/server/auth.go(1 hunks)prover/server/server/server.go(2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
prover/server/scripts/test_auth.sh
[warning] 36-36: Did you forget to close this double quoted string?
(SC1078)
[error] 40-40: '(' is invalid here. Did you forget to escape it?
(SC1036)
[error] 40-40: Parsing stopped here. Invalid use of parentheses?
(SC1088)
🪛 Gitleaks (8.27.2)
prover/server/scripts/test_auth.sh
11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
35-35: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (10)
prover/server/scripts/test_auth.sh (1)
1-43: LGTM on the testing approach.The test script comprehensively covers the authentication scenarios:
- Public endpoint access without authentication
- Protected endpoint access without authentication (expecting 401)
- Protected endpoint access with both X-API-Key and Authorization Bearer headers
The hardcoded API key is acceptable as it's clearly marked as a test value.
prover/server/server/server.go (3)
456-461: LGTM! Clean environment variable handling with appropriate logging.The implementation properly reads the API key from the environment and provides clear logging messages for both enabled and disabled authentication states.
540-540: LGTM! Proper CORS header configuration.Adding "X-API-Key" to allowed headers ensures clients can send the authentication header without CORS issues.
548-549: LGTM! Correct middleware ordering.The authentication middleware is properly applied before the CORS handler, ensuring requests are authenticated before CORS processing. The conditional application based on API key presence is a good design choice.
prover/server/server/auth.go (6)
11-23: LGTM! Well-structured middleware pattern.The middleware struct and constructor follow Go HTTP middleware conventions properly, providing clean dependency injection of the API key.
25-43: LGTM! Proper authentication flow with comprehensive logging.The ServeHTTP implementation correctly handles the authentication flow, provides detailed logging for security monitoring, and returns appropriate error responses with helpful messages.
45-56: LGTM! Security-conscious authentication check.Excellent use of
crypto/subtle.ConstantTimeCompareto prevent timing attacks. The logic properly handles the case when no API key is configured (allowing all requests).
58-70: LGTM! Flexible API key extraction supporting multiple header formats.The implementation correctly supports both
X-API-KeyandAuthorization: Bearerheader formats, providing flexibility for different client implementations.
76-88: LGTM! Clean path-based authentication control.The
requiresAuthenticationfunction provides a maintainable way to define public paths. The current implementation correctly excludes/healthfrom authentication requirements.
90-101: LGTM! Efficient conditional middleware application.The conditional middleware only applies authentication to protected paths, avoiding unnecessary overhead for public endpoints while maintaining security for protected ones.
| echo "" | ||
| echo "To run this test:" | ||
| echo "1. Set PROVER_API_KEY environment variable: export PROVER_API_KEY=test-api-key-12345" | ||
| echo "2. Start the prover server |
There was a problem hiding this comment.
Fix unclosed double quote.
The double quote on line 36 is not properly closed, causing a syntax error.
Apply this diff to fix the syntax error:
-echo "2. Start the prover server
+echo "2. Start the prover server"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 36-36: Did you forget to close this double quoted string?
(SC1078)
🤖 Prompt for AI Agents
In prover/server/scripts/test_auth.sh at line 36, the echo statement has an
unclosed double quote causing a syntax error. Close the double quote at the end
of the string to fix the syntax error.
| echo "- /health: 200 (always accessible)" | ||
| echo "- /prove without key: 401 (if API key is set)" | ||
| echo "- /prove with key: 400 or other (depends on valid circuit data)" |
There was a problem hiding this comment.
Fix parsing errors with parentheses.
The parentheses in the expected results comments are causing shell parsing errors.
Apply this diff to fix the syntax errors:
-- /health: 200 (always accessible)
-- /prove without key: 401 (if API key is set)
-- /prove with key: 400 or other (depends on valid circuit data)"
+- /health: 200 \(always accessible\)
+- /prove without key: 401 \(if API key is set\)
+- /prove with key: 400 or other \(depends on valid circuit data\)"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 40-40: '(' is invalid here. Did you forget to escape it?
(SC1036)
[error] 40-40: Parsing stopped here. Invalid use of parentheses?
(SC1088)
🤖 Prompt for AI Agents
In prover/server/scripts/test_auth.sh around lines 40 to 42, the parentheses in
the echo statements are causing shell parsing errors. To fix this, escape the
parentheses by prefixing them with backslashes or enclose the entire string in
single quotes to prevent the shell from misinterpreting them. This will ensure
the echo commands run without syntax errors.
2 participants
Enables authentication via X-API-Key or Bearer token headers.
Public /health endpoint remains accessible while all other routes require authentication when PROVER_API_KEY env var is set.
Summary by CodeRabbit
New Features
/healthendpoint remains publicly accessible without authentication.Chores