Log incoming metadata of JWT verified nodes #402
Conversation
WalkthroughThis pull request introduces changes to the authentication (authn) package, modifying the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Finishing Touches
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: 1
🔭 Outside diff range comments (2)
pkg/mocks/authn/mock_JWTVerifier.go (2)
Line range hint
21-35: Update mock implementation to handle uint32 return value correctlyThe current implementation always returns 0 for the uint32 value, which may not be suitable for testing scenarios where specific node IDs need to be verified.
Apply this diff to properly handle both return values:
func (_m *MockJWTVerifier) Verify(tokenString string) (uint32, error) { ret := _m.Called(tokenString) if len(ret) == 0 { panic("no return value specified for Verify") } - var r0 error - if rf, ok := ret.Get(0).(func(string) error); ok { - r0 = rf(tokenString) + var r0 uint32 + var r1 error + if rf, ok := ret.Get(0).(func(string) (uint32, error)); ok { + r0, r1 = rf(tokenString) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(uint32) + } + r1 = ret.Error(1) + } - } else { - r0 = ret.Error(0) - } - return 0, r0 + return r0, r1 }
Line range hint
47-67: Update mock Return and RunAndReturn methods to support new signatureThe Return and RunAndReturn methods need to be updated to support the new (uint32, error) signature.
Apply this diff to update the methods:
-func (_c *MockJWTVerifier_Verify_Call) Return(_a0 error) *MockJWTVerifier_Verify_Call { - _c.Call.Return(_a0) +func (_c *MockJWTVerifier_Verify_Call) Return(_a0 uint32, _a1 error) *MockJWTVerifier_Verify_Call { + _c.Call.Return(_a0, _a1) return _c } -func (_c *MockJWTVerifier_Verify_Call) RunAndReturn(run func(string) error) *MockJWTVerifier_Verify_Call { +func (_c *MockJWTVerifier_Verify_Call) RunAndReturn(run func(string) (uint32, error)) *MockJWTVerifier_Verify_Call { _c.Call.Return(run) return _c }
🧹 Nitpick comments (4)
pkg/authn/claims_test.go (1)
73-73: Add assertions for the returned node IDThe tests currently ignore the returned node ID (using
_). Consider adding assertions to verify the correct node ID is returned in success cases.Example addition:
- _, verificationError := verifier.Verify(token.SignedString) + nodeID, verificationError := verifier.Verify(token.SignedString) if tt.wantErr { require.Error(t, verificationError) } else { require.NoError(t, verificationError) + require.Equal(t, uint32(SIGNER_NODE_ID), nodeID, "Unexpected node ID returned") }Also applies to: 122-122
pkg/authn/verifier_test.go (1)
68-69: Enhance test coverage for node ID verificationThe tests should verify both error cases and successful node ID returns. Consider adding assertions for:
- Successful verification returns the correct node ID
- Failed verification returns 0 as node ID
Example enhancement for TestVerifier:
- _, verificationError := verifier.Verify(token.SignedString) + nodeID, verificationError := verifier.Verify(token.SignedString) require.NoError(t, verificationError) + require.Equal(t, uint32(SIGNER_NODE_ID), nodeID, "Successful verification should return signer's node ID") - _, verificationError = verifier.Verify(tokenForWrongNode.SignedString) + nodeID, verificationError = verifier.Verify(tokenForWrongNode.SignedString) require.Error(t, verificationError) + require.Equal(t, uint32(0), nodeID, "Failed verification should return 0 as node ID")Also applies to: 75-76, 93-94, 108-109, 128-129, 150-151, 172-173, 195-196, 207-208
pkg/interceptors/server/auth.go (2)
59-82: Consider performance and security implications of DNS lookupsThe DNS resolution in
logIncomingAddressIfAvailablehas several concerns:
- DNS lookups are blocking operations that could impact performance
- Failed DNS resolutions still result in logging, which could flood logs
- DNS names are not validated before logging, potentially leading to log injection
Consider these improvements:
- Make DNS resolution asynchronous or use a timeout
- Add rate limiting for failed resolutions
- Validate and sanitize DNS names before logging
Example implementation with timeout:
func (i *AuthInterceptor) logIncomingAddressIfAvailable(ctx context.Context, nodeId uint32) { if i.logger.Core().Enabled(zap.DebugLevel) { if p, ok := peer.FromContext(ctx); ok { clientAddr := p.Addr.String() var dnsName []string - // Attempt to resolve the DNS name host, _, err := net.SplitHostPort(clientAddr) if err == nil { - dnsName, err = net.LookupAddr(host) + // Add timeout for DNS resolution + dnsCtx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) + defer cancel() + resolver := &net.Resolver{} + dnsName, err = resolver.LookupAddr(dnsCtx, host) if err != nil || len(dnsName) == 0 { dnsName = []string{"Unknown"} }
5-6: Consider using x/net/netutil for DNS operationsFor better DNS resolution handling, consider using the more robust
golang.org/x/net/netutilpackage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/authn/claims_test.go(2 hunks)pkg/authn/interface.go(1 hunks)pkg/authn/verifier.go(2 hunks)pkg/authn/verifier_test.go(8 hunks)pkg/interceptors/server/auth.go(4 hunks)pkg/mocks/authn/mock_JWTVerifier.go(2 hunks)
🔇 Additional comments (3)
pkg/authn/verifier.go (1)
Line range hint
30-53: LGTM! Proper error handling and security validationThe implementation:
- Returns 0 with errors for invalid cases
- Validates the token before returning the node ID
- Maintains the security checks in the correct order
pkg/interceptors/server/auth.go (2)
98-108: LGTM: Unary interceptor changes look goodThe changes to capture and log the nodeId in the Unary interceptor are well-implemented.
129-140: LGTM: Stream interceptor changes look goodThe changes to capture and log the nodeId in the Stream interceptor are well-implemented and consistent with the Unary interceptor.
| @@ -1,5 +1,5 @@ | |||
| package authn | |||
|
|
|||
| type JWTVerifier interface { | |||
| Verify(tokenString string) error | |||
| Verify(tokenString string) (uint32, error) | |||
There was a problem hiding this comment.
💡 Codebase verification
Update mock implementation to handle both return values
The interface change is properly implemented in the main code, but the mock implementation needs to be updated. The mock's Return and RunAndReturn methods are incorrectly handling the return values:
- pkg/mocks/authn/mock_JWTVerifier.go: Update
ReturnandRunAndReturnto handle bothuint32anderrorreturn values.
🔗 Analysis chain
Verify all implementations of the JWTVerifier interface
The change to return (uint32, error) instead of just error is a breaking change that requires updates to all implementations of the JWTVerifier interface.
Let's verify all implementations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of JWTVerifier interface
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ *$_) Verify(tokenString string) (uint32, error) {
$$$
}'
Length of output: 115
Script:
#!/bin/bash
# Try different approaches to find implementations
# Search for any Verify method with the new signature
echo "=== Searching for Verify method implementations ==="
ast-grep --pattern 'func $_($_ $_) Verify(tokenString string) (uint32, error)'
# Use ripgrep to find potential implementation files
echo -e "\n=== Searching for files mentioning JWTVerifier ==="
rg -l "JWTVerifier"
# Search for Verify method calls to find potential implementations
echo -e "\n=== Searching for Verify method calls ==="
rg "\.Verify\("
Length of output: 2274
Script:
#!/bin/bash
# Find the verifier implementation
echo "=== Searching for verifier implementation ==="
fd "verifier\.go" --exclude "*test.go" --exclude "*mock*.go"
# Check mock implementation
echo -e "\n=== Checking mock implementation ==="
cat pkg/mocks/authn/mock_JWTVerifier.go
# Search for struct definitions in verifier files
echo -e "\n=== Searching for verifier structs ==="
rg -A 5 "type.*struct" pkg/authn/
Length of output: 4015
Script:
#!/bin/bash
# Check the verifier implementation
echo "=== Checking verifier implementation ==="
cat pkg/authn/verifier.go
# Also check auth.go since it's using the Verify method
echo -e "\n=== Checking auth interceptor usage ==="
cat pkg/interceptors/server/auth.go
Length of output: 7757
Fixes #391
Summary by CodeRabbit
New Features
Bug Fixes
Refactor