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

[acul71]

@SgtPooki @wemeetagain @dhuseby

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

[acul71]

@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]

Please can you add some tests for IPv6 addresses

[acul71]

Done.
But

it('should convert a simple IPv6 multiaddr to an IpNet', () => {
    const ma = '/ip6/::1/ipcidr/128'
    const ipNet = multiaddrToIpNet(ma)
    expect(ipNet.toString()).to.equal('0000:0000:0000:0000:0000:0000:0000:0001/128')
  })

Don't work it gives error so I removed it, but it should be a basic test it shouldn't fail

I'm not expert in ip6 but look at this examples: is there a bug in @multiformats/multiaddr ?

import { multiaddr} from '@multiformats/multiaddr'

function test (a) {
  const ma = multiaddr(a)
  console.log(a, ma.toString())
}

test('/ip6/0:0:0:0:0:0:0:1') // /ip6/::1
test('/ip6/::1') // /ip6/::1
test('/ip6/2001:db8:3333:4444:5555:6666:7777:8888/ipcidr/99') // /ip6/2001:db8:3333:4444:5555:6666:7777:8888/ipcidr/99
test('/ip6/2001:db8::/ipcidr/99') // /ip6/2001:db8::/ipcidr/99
test('/ip6/2001:db8::/ipcidr/100')// throw new SyntaxError('Unexpected end of data')
test('/ip6/::1/ipcidr/128') // throw new SyntaxError('Unexpected end of data')
test('/ip6/0000:0000:0000:0000:0000:0000:0000:0001/ipcidr/128') // throw new SyntaxError('Unexpected end of data')
test('/ip6/0:0:0:0:0:0:0:1/ipcidr/128') // throw new SyntaxError('Unexpected end of data')

node test.mjs 
/ip6/0:0:0:0:0:0:0:1 /ip6/::1
/ip6/::1 /ip6/::1
/ip6/2001:db8:3333:4444:5555:6666:7777:8888/ipcidr/99 /ip6/2001:db8:3333:4444:5555:6666:7777:8888/ipcidr/99
/ip6/2001:db8::/ipcidr/99 /ip6/2001:db8::/ipcidr/99
file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/node_modules/multiformats/dist/src/bases/base.js:156
        throw new SyntaxError('Unexpected end of data');
              ^

SyntaxError: Unexpected end of data
    at decode (file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/node_modules/multiformats/dist/src/bases/base.js:156:15)
    at Decoder.decode [as baseDecode] (file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/node_modules/multiformats/dist/src/bases/base.js:199:20)
    at Decoder.decode (file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/node_modules/multiformats/dist/src/bases/base.js:52:25)
    at fromString (file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/node_modules/uint8arrays/dist/src/from-string.node.js:20:25)
    at convertToBytes (file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/node_modules/@multiformats/multiaddr/dist/src/convert.js:100:20)
    at stringToMultiaddrParts (file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/node_modules/@multiformats/multiaddr/dist/src/codec.js:44:23)
    at new Multiaddr (file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/node_modules/@multiformats/multiaddr/dist/src/multiaddr.js:58:21)
    at multiaddr (file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/node_modules/@multiformats/multiaddr/dist/src/index.js:195:12)
    at test (file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/test.mjs:4:14)
    at file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/tmp/test-multiformats/test.mjs:12:1

Node.js v22.10.0

[acul71]

@achingbrain @SgtPooki

In @multifomats/multiaddr/src/convert.ts

export function convertToBytes (proto: string | number, str: string): Uint8Array {
  const protocol = getProtocol(proto)
  switch (protocol.code) {
    case 4: // ipv4
      return ip2bytes(str)
    case 41: // ipv6
      return ip2bytes(str)
    case 42: // ipv6zone
      return str2bytes(str)

    case 6: // tcp
    case 273: // udp
    case 33: // dccp
    case 132: // sctp
      return port2bytes(parseInt(str, 10))

    case 53: // dns
    case 54: // dns4
    case 55: // dns6
    case 56: // dnsaddr
    case 400: // unix
    case 449: // sni
    case 777: // memory
      return str2bytes(str)

    case 421: // ipfs
      return mh2bytes(str)
    case 444: // onion
      return onion2bytes(str)
    case 445: // onion3
      return onion32bytes(str)
    case 466: // certhash
      return mb2bytes(str)
    case 481: // http-path
      return str2bytes(globalThis.decodeURIComponent(str))
    default:
      return uint8ArrayFromString(str, 'base16') // no clue. convert from hex
  }
}

There is no case 43 (ipcidr) so in the default case

The issue arises because the string 100 (or >99) is being interpreted as hexadecimal when passed to uint8ArrayFromString with the base16 encoding. Hexadecimal (base16) encoding expects strings consisting of characters from 0-9 and a-f. In base16, each character represents 4 bits, so the total number of characters must be even to decode into full bytes.

If I add

case 43: // ipcidr
      console.log('convertToBytes case 43:', str)
      str = str.length % 2 !== 0 ? '0' + str : str
      console.log('convertToBytes uint8ArrayFromString', uint8ArrayFromString(str, 'base16'))
      return uint8ArrayFromString(str, 'base16')

It pass but then in ipnet.ts

export class IpNet {
  public readonly network: Uint8Array;
  public readonly mask: Uint8Array;

  /**
   *
   * @param ipOrCidr either network ip or full cidr address
   * @param mask in case ipOrCidr is network this can be either mask in decimal format or as ip address
   */
  constructor(ipOrCidr: string, mask?: string | number) {
    if (mask == null) {
      ({ network: this.network, mask: this.mask } = parseCidr(ipOrCidr));
    } else {
      const ipResult = parseIP(ipOrCidr);
      if (ipResult == null) {
        throw new Error("Failed to parse network");
      }
      mask = String(mask);
      const m = parseInt(mask, 10);
      if (
        Number.isNaN(m) ||
        String(m).length !== mask.length ||
        m < 0 ||
        m > ipResult.length * 8
      ) {
        const maskResult = parseIP(mask);
        if (maskResult == null) {
          throw new Error("Failed to parse mask");
        }
        this.mask = maskResult;
      } else {
        this.mask = cidrMask(m, 8 * ipResult.length);
      }
      this.network = maskIp(ipResult, this.mask);
    }
  }

IpNet('2001:db8:3333:4444:5555:6666:7777:8888','0128')
throw Error: Failed to parse mask

0128 failes because of the leading '0'

The mask Parameter and Issue
The constructor is designed to handle two types of input for mask:

Numeric CIDR masks, like 128 (as an integer or string without leading zeros).
IP masks, like 255.255.255.0 or ffff:ffff:ffff:ffff::.
In your case, the input '0128' is problematic because of the leading zero:

parseInt('0128', 10) correctly parses this as 128 (a valid CIDR mask).
However, the conditional

String(m).length !== mask.length

fails:
String(m).length evaluates to 3 ("128".length).
mask.length evaluates to 4 ("0128".length).
This mismatch triggers the constructor to treat '0128' as an IP mask instead of a CIDR mask.

SUGGESTIONS?

[MarcoPolo]

I'm a bit confused by this thread, but this may help:

The CIDR mask is limited to 0-32 for ipv4 and 0-128 for ipv6. When you convert the string form of the CIDR mast to bytes, you only need a single byte to represent all options. So the string to byte logic should convert the given decimal string number to a byte. And vice versa for the bytes to string part. See the go-multiaddr implementation for an example:

https://github.com/multiformats/go-multiaddr/pull/177/files#diff-38293d4bb2018b8660c43e7007e16a293752d0205fdc660558c419d6d3d199a6R59-R72

[acul71]

@MarcoPolo
Make sense. But the existing code is expecting a uint8Array so I was trying to change the less code possibile. Maybe I have to go take a deeper look...

[achingbrain]

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.