Add JWT verification #317
Add JWT verification #317
Conversation
|
Warning Rate limit exceeded@neekolas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThe pull request introduces JWT authentication for gRPC server interceptors. A new configuration is added to Changes
Sequence DiagramsequenceDiagram
participant Client
participant GRPCServer
participant AuthInterceptor
participant JWTVerifier
Client->>GRPCServer: gRPC Request with JWT Token
GRPCServer->>AuthInterceptor: Intercept Request
AuthInterceptor->>AuthInterceptor: Extract Token
AuthInterceptor->>JWTVerifier: Verify Token
alt Token Valid
JWTVerifier-->>AuthInterceptor: Verification Success
AuthInterceptor->>GRPCServer: Allow Request
GRPCServer->>Client: Process Request
else Token Invalid
JWTVerifier-->>AuthInterceptor: Verification Failure
AuthInterceptor-->>Client: Return Authentication Error
end
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 (
|
![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
🧹 Outside diff range and nitpick comments (4)
pkg/interceptors/server/auth.go (2)
58-82: Refactor common logic in interceptors to a helper functionThe
UnaryandStreammethods share similar logic for token extraction and verification. Refactor this common code into a helper function to reduce duplication and improve maintainability.Example refactoring:
func (i *AuthInterceptor) authenticate(ctx context.Context) (context.Context, error) { token, err := extractToken(ctx) if err != nil { return nil, err } if err := i.verifier.Verify(token); err != nil { return nil, status.Errorf( codes.Unauthenticated, "invalid auth token", ) } ctx = context.WithValue(ctx, constants.VerifiedNodeRequestCtxKey{}, true) return ctx, nil }Then update the interceptors to use this helper:
// In Unary interceptor ctx, err := i.authenticate(ctx) if err != nil { return nil, err } return handler(ctx, req) // In Stream interceptor ctx, err := i.authenticate(stream.Context()) if err != nil { return err } stream = &wrappedServerStream{ ServerStream: stream, ctx: ctx, } return handler(srv, stream)Also applies to: 85-117
🧰 Tools
🪛 GitHub Check: Lint
[failure] 78-78:
SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions (staticcheck)
71-77: Avoid exposing internal error details in responsesReturning detailed error messages to the client may expose sensitive information. Consider logging the detailed error server-side and returning a generic error message to the client.
Apply this diff to modify the error handling:
if err := i.verifier.Verify(token); err != nil { + i.logger.Warn("Invalid auth token", zap.Error(err)) return nil, status.Error( codes.Unauthenticated, - "invalid auth token: %v", - err, + "invalid auth token", ) }Repeat similar changes in the
Streammethod.Also applies to: 98-104
pkg/interceptors/server/auth_test.go (2)
125-166: Add test case for missing token inTestStreamInterceptorThe
TestStreamInterceptorfunction lacks a test case for when the token is missing but metadata is present. Adding this case ensures comprehensive test coverage.Example test case to add:
{ name: "missing token", setupContext: func() context.Context { md := metadata.New(map[string]string{}) return metadata.NewIncomingContext(context.Background(), md) }, setupVerifier: func() {}, wantError: nil, wantVerifiedNode: false, },
18-110: Refactor tests to reduce duplicationThe test structures in
TestUnaryInterceptorandTestStreamInterceptorare similar. Consider refactoring shared test logic into helper functions or table-driven tests to reduce code duplication.Example refactoring:
- Create a common test function that accepts parameters for unary and stream interceptors.
- Use it to run both sets of tests with shared logic.
Also applies to: 113-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.mockery.yaml(1 hunks)contracts/lib/forge-std(1 hunks)contracts/lib/openzeppelin-contracts(1 hunks)pkg/constants/constants.go(1 hunks)pkg/interceptors/server/auth.go(1 hunks)pkg/interceptors/server/auth_test.go(1 hunks)pkg/mocks/authn/mock_JWTVerifier.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- contracts/lib/openzeppelin-contracts
- pkg/constants/constants.go
- contracts/lib/forge-std
- pkg/mocks/authn/mock_JWTVerifier.go
🧰 Additional context used
🪛 GitHub Check: Lint
pkg/interceptors/server/auth.go
[failure] 78-78:
SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions (staticcheck)
[failure] 110-110:
SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions (staticcheck)
🔇 Additional comments (1)
.mockery.yaml (1)
7-9: Configuration for mocking JWTVerifier interface is correct
Adding the JWTVerifier interface to the .mockery.yaml configuration enables mock generation for testing, which is appropriate.
neekolas
force-pushed
the
12-09-add_jwt_verification
branch
from
0c6d7b1 to
6860139
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pkg/interceptors/server/auth.go (2)
25-36: Add parameter validation in constructor.Consider validating the input parameters to prevent nil pointer dereferences.
func NewAuthInterceptor(verifier authn.JWTVerifier, logger *zap.Logger) *AuthInterceptor { + if verifier == nil { + panic("verifier cannot be nil") + } + if logger == nil { + panic("logger cannot be nil") + } return &AuthInterceptor{ verifier: verifier, logger: logger, } }
38-55: Add token format validation.Consider adding basic token format validation before returning it. This can help catch obviously malformed tokens early.
func extractToken(ctx context.Context) (string, error) { md, ok := metadata.FromIncomingContext(ctx) if !ok { return "", status.Error(codes.Unauthenticated, "missing metadata") } values := md.Get(constants.NODE_AUTHORIZATION_HEADER_NAME) if len(values) == 0 { return "", status.Error(codes.Unauthenticated, "missing auth token") } if len(values) > 1 { return "", status.Error(codes.Unauthenticated, "multiple auth tokens provided") } + token := values[0] + if len(token) == 0 { + return "", status.Error(codes.Unauthenticated, "empty auth token") + } + // Optional: Add basic JWT format validation (three dot-separated segments) + if !strings.Contains(token, ".") || len(strings.Split(token, ".")) != 3 { + return "", status.Error(codes.Unauthenticated, "malformed auth token") + } - return values[0], nil + return token, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.mockery.yaml(1 hunks)contracts/lib/forge-std(1 hunks)contracts/lib/openzeppelin-contracts(1 hunks)pkg/constants/constants.go(1 hunks)pkg/interceptors/server/auth.go(1 hunks)pkg/interceptors/server/auth_test.go(1 hunks)pkg/mocks/authn/mock_JWTVerifier.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- contracts/lib/openzeppelin-contracts
- contracts/lib/forge-std
- pkg/constants/constants.go
- .mockery.yaml
- pkg/interceptors/server/auth_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/interceptors/server/auth.go
78-78: SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions
(staticcheck)
110-110: SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions
(staticcheck)
🪛 GitHub Check: Lint
pkg/interceptors/server/auth.go
[failure] 78-78:
SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions (staticcheck)
[failure] 110-110:
SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions (staticcheck)
🔇 Additional comments (6)
pkg/mocks/authn/mock_JWTVerifier.go (1)
1-78: LGTM! Auto-generated mock implementation looks correct.
The mock implementation correctly follows the standard patterns for testify/mock and includes all necessary components:
- Proper interface implementation
- Expectation management
- Error handling for unspecified returns
- Cleanup registration
pkg/interceptors/server/auth.go (5)
15-23: LGTM! Stream wrapper implementation is correct.
The wrapper follows the standard pattern for modifying gRPC stream context.
65-69:
Requests without auth tokens should be rejected.
Currently, requests without auth tokens are allowed to proceed, which could pose a security risk.
78-78:
Use custom type for context key.
Using empty anonymous structs as context keys can lead to collisions.
🧰 Tools
🪛 golangci-lint (1.62.2)
78-78: SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions
(staticcheck)
🪛 GitHub Check: Lint
[failure] 78-78:
SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions (staticcheck)
92-96:
Requests without auth tokens should be rejected.
Currently, requests without auth tokens are allowed to proceed, which could pose a security risk.
110-110:
Use custom type for context key.
Using empty anonymous structs as context keys can lead to collisions.
🧰 Tools
🪛 golangci-lint (1.62.2)
110-110: SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions
(staticcheck)
🪛 GitHub Check: Lint
[failure] 110-110:
SA1029: should not use empty anonymous struct as key for value; define your own type to avoid collisions (staticcheck)
neekolas
force-pushed
the
12-09-add_jwt_verification
branch
from
6860139 to
0946508
Compare
|
LGTM. Only concern is that it reintroduces the git submodules in contracts/lib (deprecated with forge soldeer). Could they be deleted? |
|
Didn't notice that. Must need a rebase. I'll remove. |
neekolas
force-pushed
the
12-09-add_jwt_verification
branch
2 times, most recently
from
d3ffcf5 to
4cb5a92
Compare
neekolas
force-pushed
the
12-09-add_jwt_verification
branch
from
4cb5a92 to
e386b03
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/interceptors/server/auth.go (1)
65-69:⚠️ Potential issueSecurity: Enforce authentication requirement
Currently, requests without tokens are allowed to proceed. This could lead to unauthorized access.
🧹 Nitpick comments (4)
pkg/interceptors/server/auth.go (2)
25-36: Add parameter validation in constructorConsider validating that neither
verifiernorloggeris nil to prevent potential panics.func NewAuthInterceptor(verifier authn.JWTVerifier, logger *zap.Logger) *AuthInterceptor { + if verifier == nil { + panic("verifier cannot be nil") + } + if logger == nil { + panic("logger cannot be nil") + } return &AuthInterceptor{ verifier: verifier, logger: logger, } }
38-55: Enhance security of error messagesThe current error messages might provide too much information. Consider using more generic messages to avoid potential security implications.
if !ok { - return "", status.Error(codes.Unauthenticated, "missing metadata") + return "", status.Error(codes.Unauthenticated, "unauthorized") } values := md.Get(constants.NODE_AUTHORIZATION_HEADER_NAME) if len(values) == 0 { - return "", status.Error(codes.Unauthenticated, "missing auth token") + return "", status.Error(codes.Unauthenticated, "unauthorized") } if len(values) > 1 { - return "", status.Error(codes.Unauthenticated, "multiple auth tokens provided") + return "", status.Error(codes.Unauthenticated, "invalid authorization") }pkg/interceptors/server/auth_test.go (2)
23-82: Add additional test cases for better security coverageConsider adding the following test cases:
- Multiple tokens scenario
- Token tampering scenarios (modified signature, expired token, etc.)
tests := []struct { name string setupContext func() context.Context setupVerifier func() wantError error wantVerifiedNode bool }{ + { + name: "multiple tokens", + setupContext: func() context.Context { + md := metadata.New(map[string]string{ + constants.NODE_AUTHORIZATION_HEADER_NAME: "token1", + }) + md.Append(constants.NODE_AUTHORIZATION_HEADER_NAME, "token2") + return metadata.NewIncomingContext(context.Background(), md) + }, + setupVerifier: func() {}, + wantError: status.Error(codes.Unauthenticated, "unauthorized"), + wantVerifiedNode: false, + }, + { + name: "tampered token", + setupContext: func() context.Context { + md := metadata.New(map[string]string{ + constants.NODE_AUTHORIZATION_HEADER_NAME: "tampered.token.here", + }) + return metadata.NewIncomingContext(context.Background(), md) + }, + setupVerifier: func() { + mockVerifier.EXPECT(). + Verify("tampered.token.here"). + Return(errors.New("token has been tampered")) + }, + wantError: status.Error(codes.Unauthenticated, "unauthorized"), + wantVerifiedNode: false, + },
118-167: Maintain consistency with unary interceptor test casesAdd the same additional test cases suggested for the unary interceptor tests to maintain consistency and coverage.
🛑 Comments failed to post (2)
pkg/interceptors/server/auth.go (2)
92-96: 🛠️ Refactor suggestion
Maintain consistency with Unary interceptor changes
Apply the same security improvements as suggested for the Unary interceptor:
- Enforce authentication requirement
- Use generic error messages
token, err := extractToken(stream.Context()) if err != nil { - i.logger.Debug("failed to find auth token. Allowing request to proceed", zap.Error(err)) - return handler(srv, stream) + return status.Error(codes.Unauthenticated, "unauthorized") } if err := i.verifier.Verify(token); err != nil { return status.Errorf( codes.Unauthenticated, - "invalid auth token: %v", - err, + "unauthorized" ) }Also applies to: 98-104
71-77: 🛠️ Refactor suggestion
Security: Avoid exposing error details
The error message includes the internal error, which could expose sensitive information about the verification process.
if err := i.verifier.Verify(token); err != nil { return nil, status.Errorf( codes.Unauthenticated, - "invalid auth token: %v", - err, + "unauthorized", ) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if err := i.verifier.Verify(token); err != nil { return nil, status.Errorf( codes.Unauthenticated, "unauthorized", ) }
Summary by CodeRabbit
New Features
Bug Fixes
Tests