feat: whitelist connection gater #31
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications across several files to integrate a peer whitelisting mechanism into the communication and TSS (Threshold Signature Scheme) functionalities. Key changes include the addition of Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
added |
skosito
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: 12
🧹 Outside diff range and nitpick comments (10)
p2p/communication_test.go (2)
21-21: Consider adding test cases for non-empty whitelist scenarios.While testing with an empty whitelist is valid, consider adding test cases that verify behavior with non-empty whitelists to ensure comprehensive coverage of the new functionality.
Would you like me to help generate additional test cases for various whitelist scenarios?
99-104: Enhance test case documentation.While the test case correctly verifies behavior with empty whitelist, the comment could be more descriptive to explain the expected behavior.
- // same as above, just without whitelisted peers + // Verify that connections fail when no peers are whitelisted, even with valid multiaddrsconversion/conversion.go (1)
216-226: Add documentation for the new function.Consider adding a doc comment to describe the function's purpose, parameters, and potential error cases.
+// Bech32PubkeyToPeerID converts a Bech32-encoded public key to a libp2p peer ID. +// It expects a valid Bech32-encoded public key string and returns the corresponding +// peer.ID or an error if the conversion fails. func Bech32PubkeyToPeerID(pubKey string) (peer.ID, error) {keygen/eddsa/keygen_test.go (1)
103-108: LGTM! Consider extracting test data setup.The whitelist initialization is well-implemented with proper error handling. However, consider moving the test public keys conversion to a helper function or
SetUpSuiteto keep the test setup more maintainable.func (s *EddsaKeygenTestSuite) SetUpSuite(c *C) { common.InitLog("info", true, "keygen_test") conversion.SetupBech32Prefix() + // Initialize whitelisted peers + for _, pk := range testPubKeys { + peer, err := conversion.Bech32PubkeyToPeerID(pk) + c.Assert(err, IsNil) + s.whitelistedPeers = append(s.whitelistedPeers, peer) + } // ... rest of the setup }keysign/eddsa/keysign_test.go (1)
155-156: LGTM: Non-bootstrap nodes setup with whitelist.The communication setup for non-bootstrap nodes correctly incorporates both the whitelist parameter and multiAddr for peer discovery.
Consider adding test cases that verify:
- Successful connections between whitelisted peers
- Network formation with partial whitelist (subset of peers)
keygen/ecdsa/keygen_test.go (2)
117-122: LGTM! Consider adding negative test cases.The whitelisted peers initialization looks good. However, consider adding test cases to verify the behavior with invalid public keys.
Add test cases to verify:
- Invalid Bech32 public key format
- Empty public key
- Malformed public key
127-129: LGTM! Consider adding documentation.The bootstrap node setup is correct. Consider adding a comment explaining that this is the bootstrap node configuration.
Add a comment before line 127:
// Configure the bootstrap node (first party) with no initial peerskeysign/ecdsa/keysign_test.go (1)
152-153: Improve clarity of NewCommunication initialization.The empty string parameter and the distinction between bootstrap and subsequent nodes could be made more explicit:
-comm, err := p2p.NewCommunication("asgard", nil, ports[i], "", whitelistedPeers) +const bootstrapAddr = "" // Empty string for bootstrap node +comm, err := p2p.NewCommunication("asgard", nil, ports[i], bootstrapAddr, whitelistedPeers)Consider adding a comment to explain the different initialization patterns:
// Initialize bootstrap node without multiaddress if i == 0 { comm, err := p2p.NewCommunication("asgard", nil, ports[i], bootstrapAddr, whitelistedPeers) // ... } else { // Initialize subsequent nodes with bootstrap node's multiaddress comm, err := p2p.NewCommunication("asgard", []maddr.Multiaddr{multiAddr}, ports[i], bootstrapAddr, whitelistedPeers) // ... }Also applies to: 158-159
p2p/communication.go (2)
65-65: Add documentation for the whitelisted peers field.Consider adding a comment explaining the purpose and implications of the
whitelistedPeersfield, particularly its role in connection gating.+ // whitelistedPeers contains the list of peer IDs that are allowed to connect + // to this node. If empty, all peer connections will be rejected. whitelistedPeers []peer.ID
Line range hint
331-346: Consider complete DHT removal.The code still initializes and uses DHT for peer discovery, although it's scheduled for shutdown after 5 minutes. Since the PR objective is to remove DHT:
- Consider removing DHT initialization entirely since peer connections are now whitelisted
- The 5-minute DHT shutdown might be unnecessary if we're not using DHT for discovery
- kademliaDHT, err := dht.New(ctx, h, dht.Mode(dht.ModeServer)) - if err != nil { - return fmt.Errorf("fail to create DHT: %w", err) - } - c.logger.Debug().Msg("Bootstrapping the DHT") - if err = kademliaDHT.Bootstrap(ctx); err != nil { - return fmt.Errorf("fail to bootstrap DHT: %w", err) - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- cmd/tss/main.go (2 hunks)
- conversion/conversion.go (1 hunks)
- keygen/ecdsa/keygen_test.go (1 hunks)
- keygen/eddsa/keygen_test.go (1 hunks)
- keysign/ecdsa/keysign_old_test.go (1 hunks)
- keysign/ecdsa/keysign_test.go (1 hunks)
- keysign/eddsa/keysign_test.go (1 hunks)
- p2p/communication.go (3 hunks)
- p2p/communication_test.go (3 hunks)
- p2p/whitelist_connection_gater.go (1 hunks)
- tss/tss.go (3 hunks)
- tss/tss_4nodes_test.go (2 hunks)
- tss/tss_4nodes_zeta_test.go (2 hunks)
🔇 Additional comments (18)
cmd/tss/main.go (2)
15-15: LGTM: Import required for peer whitelisting.The addition of the peer package import is necessary for the new whitelisting functionality.
67-67: Verify whitelist implementation and test coverage.Let's verify the existence of negative tests and proper implementation of connection gating.
✅ Verification successful
Let me gather more information with a different search strategy.
Whitelist implementation is properly secured with comprehensive connection gating
The implementation includes:
- Robust whitelist validation in
tss/tss.gothat prevents initialization with empty whitelist- Complete connection gating implementation in
p2p/whitelist_connection_gater.gothat:
- Intercepts peer connections at multiple levels (dial, secure, upgrade)
- Properly validates peers against whitelist
- Includes detailed logging for both allowed and denied connections
- Test coverage across multiple packages including
tss,keygen, andkeysign🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for negative tests and connection gating implementation # Test 1: Look for negative test cases echo "Checking for negative test cases..." rg -l "TestTssServer.*Whitelist" -g "**/*_test.go" # Test 2: Verify connection gating implementation echo "Checking connection gating implementation..." ast-grep --pattern 'func ($handler *$_) HandlePeerDial($_) { $$$ }'Length of output: 304
Script:
#!/bin/bash # Search for whitelist-related code and tests more broadly # Look for whitelist-related test files echo "Searching for whitelist-related test files..." rg -l "whitelist|allowlist" -g "**/*_test.go" -i # Look for whitelist implementation echo "Searching for whitelist implementation..." rg -A 5 "whitelist|allowlist" -g "!**/*_test.go" -i # Check for connection handling/gating echo "Checking connection handling..." rg -A 5 "HandlePeerDial|ConnGater" -iLength of output: 6800
p2p/communication_test.go (3)
42-55: LGTM! Well-structured test setup.The peer ID generation and bootstrap setup is well-implemented with proper error handling and use of generated keys for test peers.
76-89: LGTM! Comprehensive error handling test cases.The test cases effectively verify both failure scenarios with invalid peers and recovery scenarios with mixed valid/invalid peers.
63-69: 🛠️ Refactor suggestionAdd validation for whitelist enforcement.
While the test verifies successful communication setup, it should also validate that the whitelist is being enforced. Consider adding assertions to verify that only whitelisted peers can establish connections.
Add assertions like:
// Verify connection is allowed for whitelisted peer c.Assert(comm.IsAllowed(id1), Equals, true) // Verify connection is denied for non-whitelisted peer c.Assert(comm.IsAllowed(nonWhitelistedID), Equals, false)conversion/conversion.go (1)
216-226: Implementation looks good, verify integration with whitelist feature.The function implementation is clean and follows good practices. Let's verify its usage in the whitelist feature implementation.
✅ Verification successful
Integration with whitelist feature verified and properly implemented
The
Bech32PubkeyToPeerIDfunction is correctly integrated with the whitelist feature:
- Used in
tss,keygen, andkeysignpackages to convert public keys to peer IDs for whitelist initialization- Integrated with
WhitelistConnectionGaterin p2p package for peer validation- Properly enforced in
NewTssconstructor with validation for empty whitelist- Successfully used in test suites across multiple packages demonstrating correct integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of Bech32PubkeyToPeerID in whitelist implementation # Test 1: Check for function usage in test files echo "Checking usage in test files..." rg -l "Bech32PubkeyToPeerID" --type go # Test 2: Check for whitelist-related code that might use this function echo "Checking whitelist implementation..." rg -A 5 "whitelistedPeers" --type goLength of output: 14332
tss/tss_4nodes_zeta_test.go (2)
17-17: LGTM!The addition of the peer package import is appropriate for the new whitelisting functionality.
238-244: Verify non-whitelisted connection rejection.While the PR comments mention adding negative tests, this test file doesn't explicitly verify that non-whitelisted connections are rejected. Consider adding test cases that:
- Attempt to connect with a non-whitelisted peer
- Verify that the connection is rejected
- Ensure the TSS operation fails appropriately
Let's check if negative tests exist elsewhere:
✅ Verification successful
Non-whitelisted peer rejection is properly implemented
The codebase already implements robust peer whitelisting through libp2p's ConnectionGater interface:
- The
WhitelistConnectionGateris configured during host creation with whitelisted peers- Connection attempts are intercepted at multiple levels:
InterceptPeerDial: Validates outbound connection attemptsInterceptSecured: Validates both inbound and outbound connections after security handshake- The
peerAllowedmethod enforces whitelist validation by checking against the pre-configured whitelist mapThe test can focus on the successful case since the connection rejection is handled at the libp2p level through the connection gater, which is a proven and tested component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files that might contain negative tests for whitelisting rg -l "whitelistedPeers.*\[\]peer\.ID" --type go | grep "_test\.go$" | while read -r file; do echo "=== Checking $file for negative tests ===" rg -A 5 "TestNon.*Whitelist|test.*reject.*connection" "$file" doneLength of output: 1982
Script:
#!/bin/bash # Let's check for any whitelist-related connection handling code rg -A 10 "func.*NewTss" tss/tss.go # Check for any connection rejection or whitelist validation logic rg -A 10 "whitelist.*peer" tss/tss.go p2p/communication.go # Check for any connection handling in p2p layer rg -A 10 "func.*handleNewPeer|func.*handlePeer" p2p/communication.goLength of output: 3322
Script:
#!/bin/bash # Let's check the p2p connection handling implementation rg -A 15 "func \(c \*Communication\) Connect" p2p/communication.go # Check for peer validation or connection rejection logic rg -A 10 "func.*validatePeer|func.*checkPeer|func.*isWhitelisted" p2p/communication.go # Check the host configuration to see how connections are filtered rg -A 10 "NewHost|libp2p\.New" p2p/communication.goLength of output: 597
Script:
#!/bin/bash # Let's check the WhitelistConnectionGater implementation rg -A 15 "type WhitelistConnectionGater|func.*WhitelistConnectionGater" p2p/connection_gater.go # Check for connection rejection methods rg -A 10 "func.*InterceptPeerDial|func.*InterceptSecured|func.*InterceptAccept" p2p/connection_gater.goLength of output: 331
Script:
#!/bin/bash # Let's find the connection gater file first fd -t f -e go gater # Search for whitelist-related files fd -t f whitelist # Search for connection handling in all Go files rg -A 10 "InterceptPeerDial|InterceptSecured|InterceptAccept" --type goLength of output: 1974
keygen/eddsa/keygen_test.go (2)
113-114: LGTM! Bootstrap node setup is correct.The whitelisting configuration is properly applied to the bootstrap node with appropriate error handling.
119-120: 🛠️ Refactor suggestionAdd negative test case for whitelist enforcement.
While the whitelisting setup is correct, consider adding a test case that verifies non-whitelisted peers are properly rejected. This aligns with the PR comment discussion about negative testing.
Add a test case like:
func (s *EddsaKeygenTestSuite) TestNonWhitelistedPeerRejection(c *C) { // Setup a peer not in the whitelist nonWhitelistedPrivKey := generateTestPrivKey() comm, err := p2p.NewCommunication("asgard", []maddr.Multiaddr{multiAddr}, 19670, "", whitelistedPeers) c.Assert(err, IsNil) // Attempt connection with non-whitelisted peer err = comm.Start(nonWhitelistedPrivKey) c.Assert(err, NotNil) c.Assert(err.Error(), Matches, "peer not in whitelist") }Let's verify the current test coverage:
tss/tss.go (1)
62-62: LGTM: Function signature change is well-structured.The addition of
whitelistedPeers []peer.IDparameter follows Go conventions and uses appropriate types.keysign/eddsa/keysign_test.go (2)
139-144: Add negative test case for non-whitelisted peer rejection.While the whitelist initialization is correct, consider adding a test case that verifies non-whitelisted peers are properly rejected. This aligns with the PR objective and validates the security aspect of the whitelisting mechanism.
Would you like me to help generate a test case that attempts to connect with a non-whitelisted peer?
149-150: LGTM: Bootstrap node setup with whitelist.The bootstrap node's communication setup correctly incorporates the whitelist parameter while maintaining proper error handling.
keygen/ecdsa/keygen_test.go (1)
133-134: LGTM! Consider adding whitelist enforcement test.The peer configuration looks good. However, consider adding a test case to verify that non-whitelisted peers are rejected.
Let's verify if there are any existing test cases for whitelist enforcement:
tss/tss_4nodes_test.go (1)
18-18: LGTM: Import statement properly placedThe peer package import is correctly grouped with other libp2p-related imports and is necessary for the new whitelisting functionality.
keysign/ecdsa/keysign_old_test.go (2)
129-134: LGTM: Proper initialization of whitelisted peersThe implementation correctly converts public keys to peer IDs and builds the whitelist array with appropriate error handling.
139-140: LGTM: Proper bootstrap node configurationThe bootstrap node is correctly configured with whitelist support while maintaining its role as the initial node (empty bootstrap address).
p2p/communication.go (1)
312-312: Verify whitelist connection gater implementation.The connection gater integration looks correct, but we should verify:
- The gater's thread-safety for concurrent connection attempts
- Proper handling of bootstrap peer connections
- Logging of rejected connections for monitoring
✅ Verification successful
Let me gather more information about the connection gater implementation.
Connection gater implementation is secure and correctly handles bootstrap peers
The WhitelistConnectionGater implementation has been verified and shows:
- Thread-safety is maintained through immutable whitelisted peers map, initialized at construction
- Bootstrap peer connections are properly handled as they must be in the whitelist to connect
- Comprehensive logging of both allowed and rejected connections at appropriate levels (Debug/Info)
- Clean separation of concerns with dedicated methods for different connection interception points
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the connection gater implementation echo "Searching for connection gater implementation..." rg -p "NewWhitelistConnectionGater" # Check for concurrent connection handling echo "Checking concurrent connection handling..." ast-grep --pattern 'func $_($$) bool { $$$ sync.$_ $$$ $$$ }'Length of output: 567
Script:
#!/bin/bash # Check the WhitelistConnectionGater implementation echo "Checking WhitelistConnectionGater implementation..." cat p2p/whitelist_connection_gater.go # Look for bootstrap peer handling echo -e "\nChecking bootstrap peer handling..." rg -p "bootstrap" -A 5 -B 5Length of output: 18333
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests