-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use JWT verifier in API server #365
Conversation
WalkthroughThe changes introduce JWT-based authentication to the API server infrastructure. Modifications span multiple files in the Changes
Sequence DiagramsequenceDiagram
participant Config as Server Configuration
participant APIServer as API Server
participant JWTVerifier as JWT Verifier
participant Interceptors as Authentication Interceptors
Config->>APIServer: Initialize with optional JWTVerifier
alt JWTVerifier is not nil
APIServer->>JWTVerifier: Verify JWT
JWTVerifier-->>Interceptors: Create Unary/Stream Interceptors
Interceptors->>APIServer: Attach Authentication Interceptors
else JWTVerifier is nil
APIServer->>APIServer: Proceed without authentication
end
The sequence diagram illustrates the conditional JWT verification process during API server initialization, showing how the JWT verifier is optionally used to create authentication interceptors. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
prometheus.StreamServerInterceptor, | ||
} | ||
|
||
if jwtVerifier != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Payer APIs can't use the JWT verifier, since they aren't a valid audience for the token, so it's optional
d553971
to
aef0090
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/server/server.go (1)
208-213
: Conditional JWT verifier creation
Creating thejwtVerifier
only if replication is enabled is appropriate, minimizing resource usage. Double-check that you cover other operational modes if they also need JWT.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/api/server.go
(3 hunks)pkg/server/server.go
(2 hunks)pkg/testutils/api/api.go
(3 hunks)
🔇 Additional comments (10)
pkg/testutils/api/api.go (3)
14-14
: Good import usage for JWT validation
Bringing inauthn
is a solid move toward centralized and testable JWT verification logic.
82-83
: Validate and test the JWT verifier
It's good to seejwtVerifier
being instantiated withauthn.NewRegistryVerifier
. Ensure thatmockRegistry
is properly configured for unit tests so the verifier produces valid tokens during local test runs.
113-113
: Consistent parameter passing
The addition ofjwtVerifier
toapi.NewAPIServer
aligns with the new authentication flow. This maintains clarity and a straightforward integration path for JWT-based security.pkg/api/server.go (5)
10-12
: Relevant imports for authentication interceptors
These imports indicate usage of JWT verification and custom interceptors. Ensure no duplication of coverage in other packages to keep dependencies concise.
46-46
: Augmented server constructor
IncorporatingjwtVerifier
into the API server's signature is a clean approach. Confirm existing call sites are updated to provide the parameter (or nil) as needed to avoid runtime errors.
73-79
: Interceptor arrays extracted
Declaring interceptor slices prior togrpc.ChainUnaryInterceptor
/ChainStreamInterceptor
is a clean pattern. This change also clarifies future expansions (e.g. new interceptors).
80-84
: Conditional JWT interceptor initialization
Properly checks for a non-niljwtVerifier
, limiting overhead when authentication is disabled. This approach is efficient and maintains clear separation of concerns.
Line range hint
220-220
: Ensure external references match
PassingjwtVerifier
here is key. Confirm that consumers of this constructor are aware of the new parameter so they can supply either a valid verifier or nil as needed.pkg/server/server.go (2)
11-13
: Imports for resolver logic
Includingauthn
and retainingmlsvalidate
remains consistent with the new integrated security approach. Consider removing unused imports (if any) to keep the package lean.
220-220
: Param passing for optional authentication
jwtVerifier
neatly extendsapi.NewAPIServer
. Validate that replication or other features relying on JWT are exercised in integration tests to confirm token flows are correct.
Summary by CodeRabbit
New Features
Improvements