feat: Use CIDR format for connection-manager allow/deny lists #2783

acul71 · 2024-10-24T03:27:06Z

Description

This PR updates the connection manager to treat multiaddrs in the allow/deny lists using the standard IP CIDR format (e.g. /ip4/52.55.0.0/ipcidr/16) rather than string prefixes (e.g. /ip4/52.55). This allows us to validate multiaddrs accurately and ensures better control over IP address matching.

Changes:

Converted the .allow and .deny properties in the connection manager to use IpNet[] for IP range validation.
Updated tests to use the IpNet format for both allow and deny lists.
This will improve validation, making it more aligned with IP network management best practices by using CIDR blocks.

Relevant issues:

Related to: Use standard IP cidr format for allow/deny lists #1510.

Notes & open questions

How developer will know that they have to use IpNet format in allow and deny list?
This PR focuses on using the IpNet format to ensure the connection manager handles IPs in CIDR notation.
Any future changes related to this feature would involve expanding IP management logic.

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation if necessary (this includes comments as well)
I have added tests that prove my fix is effective or that my feature works

packages/libp2p/test/connection-manager/index.spec.ts

packages/libp2p/src/connection-manager/index.ts

packages/libp2p/src/connection-manager/connection-pruner.ts

packages/libp2p/src/connection-manager/index.ts

…r handling for invalid multiaddrs

…Net with encapsulation

…g behavior for allowlist connections

… IpNet objects

…-1510

acul71 · 2024-11-16T02:33:17Z

@SgtPooki @achingbrain

Commit History and Changes

This document summarizes the commits made to the repository, detailing the changes and the rationale behind each commit.

1. Commit: Add `multiaddrToIpNet` utility function

Summary:

Added a new utility function multiaddrToIpNet in the packages/utils/src/multiaddrToIpNet.ts file.

Explanation:

Purpose: This function converts a multiaddr string or object into an IpNet object. If the multiaddr doesn't already include the /ipcidr/32 segment, it encapsulates it with /ipcidr/32 to ensure the correct format for IP network parsing.
Impact: This function is used to parse and encapsulate multiaddresses correctly for use in IP-based operations throughout the project.

2. Commit: Update `connection-manager` to use `multiaddrToIpNet`

Summary:

Refactored packages/libp2p/src/connection-manager/connection-pruner.ts to use the newly created multiaddrToIpNet function.

Explanation:

Purpose: The code was modified to replace the previous logic that directly handled multiaddr-to-IP network conversions with the new multiaddrToIpNet utility, improving modularity and maintainability.
Impact: This change simplifies the conversion process and centralizes logic in the multiaddrToIpNet function, reducing duplication across the codebase.

3. Commit: Refactor `connection-manager` to handle allow/deny lists

Summary:

Refactored the parsing of allow and deny lists in packages/libp2p/src/connection-manager/index.ts.

Explanation:

Purpose: Simplified the parsing of the allow and deny lists, moving the logic into a new parseIpNetList function. This improves the clarity and maintainability of the code.
Impact: The new function reduces duplication and clarifies how multiaddresses are converted to IpNet objects within the DefaultConnectionManager.

4. Commit: Add tests for `allow` and `deny` list parsing

Summary:

Added a new test in packages/libp2p/test/connection-manager/index.spec.ts to ensure that the allow and deny lists are correctly parsed and stored as IpNet objects in ConnectionManager.

Explanation:

Purpose: This test ensures that the ConnectionManager correctly processes multiaddresses from allow and deny lists and converts them to IpNet objects.
Impact: Provides confidence that the code correctly handles and stores network information for allowed and denied connections, which is vital for connection management.

5. Commit: Fix test for pruning behavior in connection manager

Summary:

Fixed a test in packages/libp2p/test/connection-manager/index.spec.ts that ensures a connection on the allow list is not closed when pruning connections.

Explanation:

Purpose: The test now correctly verifies that a connection listed in the allow list is not closed when pruning, ensuring expected behavior for connections deemed "safe" to keep alive.
Impact: This fix addresses issues with pruning connections and ensures the stability of critical connections within the connection manager.
Note: It took many hours to figure out why the test should not close connection that is on the allowlist when pruning was going into a timeout. After extensive debugging, it was discovered that the remoteAddr property was missing in the connection object. This caused the Promise to never resolve, leading to the timeout. Once the remoteAddr was correctly assigned, the test passed as expected.

6. Commit: Add `@chainsafe/netmask` dependency

Summary:

Added @chainsafe/netmask dependency to packages/libp2p/package.json.

Explanation:

Purpose: The new package @chainsafe/netmask was added to handle IP network operations such as subnet matching and parsing. This is a key dependency for working with IP networks within the project.
Impact: Ensures the availability of necessary tools for IP network parsing and operations, supporting the changes made in connection-manager.

7. Commit: Update `package.json` to reflect new package installation

Summary:

Committed the changes to package.json to include the newly added dependency @chainsafe/netmask.

Explanation:

Purpose: This commit finalizes the installation of the @chainsafe/netmask package by adding it to the dependencies section of package.json.
Impact: Ensures that the dependency is correctly tracked for future installations and builds.

Conclusion

These commits primarily focus on enhancing the handling of IP networks within the connection management system, adding utility functions, improving parsing mechanisms, and ensuring correct functionality through tests. These changes improve the overall maintainability, reliability, and flexibility of the codebase in dealing with network-related operations.

achingbrain

Looking good, really close - comments inline.

achingbrain · 2024-11-16T10:33:17Z

packages/utils/src/multiaddrToIpNet.ts

Please follow the existing conventions in this monorepo. All file names are kebab-case, this one should be too.

achingbrain · 2024-11-16T10:36:07Z

packages/utils/src/multiaddrToIpNet.ts

+ * @returns {IpNet} The converted IpNet object.
+ * @throws {Error} Throws an error if the multiaddr is not valid.
+ */
+export function multiaddrToIpNet (ma: string | Multiaddr): IpNet {


This function is only used in the libp2p module so please define it there in the src/connection-manager/utils.ts file. Please also move the tests to that module.

If we later find that we need it elsewhere we can move it to the utils package, but right now there's no need to expand the public API.

achingbrain · 2024-11-16T10:36:38Z

packages/utils/test/multiaddrToIpNet.spec.ts

+    const ma = '/ip4/127.0.0.1/ipcidr/32'
+    const ipNet = multiaddrToIpNet(ma)
+    expect(ipNet.toString()).to.equal('127.0.0.1/32')
+  })


Please can you add some tests for IPv6 addresses

achingbrain · 2024-11-16T10:43:35Z

packages/libp2p/src/connection-manager/index.ts

@@ -571,7 +583,7 @@ export class DefaultConnectionManager implements ConnectionManager, Startable {
  async acceptIncomingConnection (maConn: MultiaddrConnection): Promise<boolean> {


Please can you add some tests that show that incoming connections are denied for multiaddrs in the deny list and allowed for ones in the allow list.

They should show that individual addresses and ranges are handled correctly in both IPv4 and IPv6 formats.

It should be a case of adding more tests like this one and this one.

acul71 requested a review from a team as a code owner October 24, 2024 03:27

SgtPooki reviewed Oct 29, 2024

View reviewed changes

achingbrain force-pushed the main branch from cda1479 to 0a3406a Compare November 2, 2024 11:39

SgtPooki reviewed Nov 7, 2024

View reviewed changes

packages/libp2p/src/connection-manager/index.ts Outdated Show resolved Hide resolved

acul71 added 14 commits November 15, 2024 00:06

Use standard IP cidr format for allow/deny lists (no test)

bc80c72

make one line of code

f5080a6

Fix lint issue

8a778e3

nit: instead of ma => we should indicate that it's an ipNet

f6c95cd

fix: allow both specific IP addresses and ipcidr

cad6894

Refactor allow/deny list parsing with encapsulate() and improved erro…

e21dda3

…r handling for invalid multiaddrs

chore: add @chainsafe/netmask package for IpNet support

e95753f

chore: add @chainsafe/netmask package for IpNet support

56a3b41

feat(utils): add multiaddrToIpNet function to convert multiaddr to Ip…

719773d

…Net with encapsulation

fix: update allow list to use multiaddrToIpNet for conversion

db47949

test: add test for parsing allow list as IpNet objects and fix prunin…

d65b996

…g behavior for allowlist connections

refactor: simplify allow/deny list parsing with utility function

18bd35c

test: add test for correct parsing and storage of allow/deny lists as…

c8f0230

… IpNet objects

Merge branch 'main' of https://github.com/libp2p/js-libp2p into issue…

a871a44

…-1510

acul71 force-pushed the issue-1510 branch from d268455 to a871a44 Compare November 16, 2024 02:28

achingbrain requested changes Nov 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use CIDR format for connection-manager allow/deny lists #2783

feat: Use CIDR format for connection-manager allow/deny lists #2783

acul71 commented Oct 24, 2024 •

edited

Loading

acul71 commented Nov 16, 2024

achingbrain left a comment

achingbrain Nov 16, 2024

achingbrain Nov 16, 2024

achingbrain Nov 16, 2024

achingbrain Nov 16, 2024

		@@ -571,7 +583,7 @@ export class DefaultConnectionManager implements ConnectionManager, Startable {
		async acceptIncomingConnection (maConn: MultiaddrConnection): Promise<boolean> {

feat: Use CIDR format for connection-manager allow/deny lists #2783

Are you sure you want to change the base?

feat: Use CIDR format for connection-manager allow/deny lists #2783

Conversation

acul71 commented Oct 24, 2024 • edited Loading

Description

Changes:

Relevant issues:

Notes & open questions

Change checklist

acul71 commented Nov 16, 2024

Commit History and Changes

1. Commit: Add multiaddrToIpNet utility function

Summary:

Explanation:

2. Commit: Update connection-manager to use multiaddrToIpNet

Summary:

Explanation:

3. Commit: Refactor connection-manager to handle allow/deny lists

Summary:

Explanation:

4. Commit: Add tests for allow and deny list parsing

Summary:

Explanation:

5. Commit: Fix test for pruning behavior in connection manager

Summary:

Explanation:

6. Commit: Add @chainsafe/netmask dependency

Summary:

Explanation:

7. Commit: Update package.json to reflect new package installation

Summary:

Explanation:

Conclusion

achingbrain left a comment

Choose a reason for hiding this comment

achingbrain Nov 16, 2024

Choose a reason for hiding this comment

achingbrain Nov 16, 2024

Choose a reason for hiding this comment

achingbrain Nov 16, 2024

Choose a reason for hiding this comment

achingbrain Nov 16, 2024

Choose a reason for hiding this comment

acul71 commented Oct 24, 2024 •

edited

Loading

1. Commit: Add `multiaddrToIpNet` utility function

2. Commit: Update `connection-manager` to use `multiaddrToIpNet`

3. Commit: Refactor `connection-manager` to handle allow/deny lists

4. Commit: Add tests for `allow` and `deny` list parsing

6. Commit: Add `@chainsafe/netmask` dependency

7. Commit: Update `package.json` to reflect new package installation