[WIP] - Add support for connection gating and address factory #1
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds network address filtering and connection gating capabilities to the P2P client, allowing control over private IP usage and improved address management.
- Adds configuration options for private IP filtering and port specification
- Implements address factory to filter private IPs from broadcast addresses and fetch public IP when needed
- Integrates connection gater to block private IP ranges from being dialed
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| config.go | Adds AllowPrivateIPs flag and Port configuration option |
| client.go | Implements address factory, connection gating, and helper functions for IP filtering and public IP retrieval |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
client.go
Outdated
| // Ignore errors because we don't care if we can't find it | ||
| ifconfig, _ := GetPublicIP(context.Background()) | ||
| if len(ifconfig) > 0 { | ||
| addr, _ := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%s", ifconfig, config.Port)) |
There was a problem hiding this comment.
The format specifier for config.Port should be %d instead of %s since Port is an int type.
| addr, _ := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%s", ifconfig, config.Port)) | |
| addr, _ := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%d", ifconfig, config.Port)) |
| privateRanges := []*net.IPNet{ | ||
| {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, | ||
| {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, | ||
| {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, | ||
| {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, | ||
| } |
There was a problem hiding this comment.
The private IP ranges are duplicated between isPrivateIP function (lines 715-720) and the connection gater setup (lines 132-139). Consider extracting these ranges to a shared constant or variable to avoid duplication and ensure consistency.
| if ip == nil || ip.To4() == nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
This function only handles IPv4 addresses due to the ip.To4() check, but the extractIPFromMultiaddr function only extracts IPv4 addresses. IPv6 private addresses (like link-local fe80::/10) will not be detected as private. Consider adding IPv6 support or documenting this limitation.
client.go
Outdated
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return string(body), resp.Body.Close() |
There was a problem hiding this comment.
The response body should be closed in a defer statement after checking for errors, not in the return statement. The current approach may not close the body if there's an error reading it.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), nil |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| privateRanges := []*net.IPNet{ | ||
| {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, | ||
| {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, | ||
| {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, | ||
| {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, | ||
| } |
There was a problem hiding this comment.
The private IP ranges are duplicated between the isPrivateIP function and the connection gater setup (lines 135-142). Consider extracting these ranges into a shared constant or variable to maintain consistency.
client.go
Outdated
| // Function to check if an IP address is private | ||
| func isPrivateIP(addr multiaddr.Multiaddr) bool { | ||
| ipStr, err := extractIPFromMultiaddr(addr) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| ip := net.ParseIP(ipStr) | ||
| if ip == nil || ip.To4() == nil { | ||
| return false | ||
| } | ||
|
|
||
| // Define private IPv4 ranges | ||
| privateRanges := []*net.IPNet{ | ||
| {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, | ||
| {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, | ||
| {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, | ||
| {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, | ||
| } | ||
|
|
||
| // Check if the IP falls into any of the private ranges | ||
| for _, r := range privateRanges { | ||
| if r.Contains(ip) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // Function to extract IP information from a Multiaddr | ||
| func extractIPFromMultiaddr(addr multiaddr.Multiaddr) (string, error) { | ||
| return addr.ValueForProtocol(multiaddr.P_IP4) |
There was a problem hiding this comment.
The function only handles IPv4 addresses by checking ip.To4() == nil, but the connection gater doesn't include IPv6 private ranges. This creates inconsistent behavior between the address factory and connection gater for IPv6 addresses.
| // Function to check if an IP address is private | |
| func isPrivateIP(addr multiaddr.Multiaddr) bool { | |
| ipStr, err := extractIPFromMultiaddr(addr) | |
| if err != nil { | |
| return false | |
| } | |
| ip := net.ParseIP(ipStr) | |
| if ip == nil || ip.To4() == nil { | |
| return false | |
| } | |
| // Define private IPv4 ranges | |
| privateRanges := []*net.IPNet{ | |
| {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, | |
| {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, | |
| {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, | |
| {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, | |
| } | |
| // Check if the IP falls into any of the private ranges | |
| for _, r := range privateRanges { | |
| if r.Contains(ip) { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| // Function to extract IP information from a Multiaddr | |
| func extractIPFromMultiaddr(addr multiaddr.Multiaddr) (string, error) { | |
| return addr.ValueForProtocol(multiaddr.P_IP4) | |
| // Function to check if an IP address is private (IPv4 and IPv6) | |
| func isPrivateIP(addr multiaddr.Multiaddr) bool { | |
| ipStr, err := extractIPFromMultiaddr(addr) | |
| if err != nil { | |
| return false | |
| } | |
| ip := net.ParseIP(ipStr) | |
| if ip == nil { | |
| return false | |
| } | |
| // Define private IPv4 ranges | |
| privateIPv4Ranges := []*net.IPNet{ | |
| {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, | |
| {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, | |
| {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, | |
| {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, | |
| } | |
| // Define private IPv6 ranges | |
| privateIPv6Ranges := []*net.IPNet{ | |
| // Unique local address (ULA): fc00::/7 | |
| {IP: net.ParseIP("fc00::"), Mask: net.CIDRMask(7, 128)}, | |
| // Link-local unicast: fe80::/10 | |
| {IP: net.ParseIP("fe80::"), Mask: net.CIDRMask(10, 128)}, | |
| // Loopback: ::1/128 | |
| {IP: net.ParseIP("::1"), Mask: net.CIDRMask(128, 128)}, | |
| } | |
| if ip.To4() != nil { | |
| // IPv4 | |
| for _, r := range privateIPv4Ranges { | |
| if r.Contains(ip) { | |
| return true | |
| } | |
| } | |
| } else if ip.To16() != nil { | |
| // IPv6 | |
| for _, r := range privateIPv6Ranges { | |
| if r.Contains(ip) { | |
| return true | |
| } | |
| } | |
| } | |
| return false | |
| } | |
| // Function to extract IP information from a Multiaddr (IPv4 or IPv6) | |
| func extractIPFromMultiaddr(addr multiaddr.Multiaddr) (string, error) { | |
| ipStr, err := addr.ValueForProtocol(multiaddr.P_IP4) | |
| if err == nil && ipStr != "" { | |
| return ipStr, nil | |
| } | |
| ipStr, err = addr.ValueForProtocol(multiaddr.P_IP6) | |
| if err == nil && ipStr != "" { | |
| return ipStr, nil | |
| } | |
| return "", fmt.Errorf("no IP address found in multiaddr") |
example/main.go
Outdated
| Name: *name, | ||
| Logger: logger, | ||
| PrivateKey: privKey, | ||
| Port: port, |
There was a problem hiding this comment.
The port variable is a *int (pointer to int) but the Config.Port field expects an int. This should be dereferenced as *port.
| Port: port, | |
| Port: *port, |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
client.go
Outdated
| if ip == nil || ip.To4() == nil { | ||
| return false | ||
| } | ||
|
|
||
| // Define private IPv4 and IPv6 ranges | ||
| privateRanges := []*net.IPNet{ | ||
| // IPv4 | ||
| {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, | ||
| {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, | ||
| {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, | ||
| {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, | ||
| // IPv6 | ||
| {IP: net.ParseIP("fc00::"), Mask: net.CIDRMask(7, 128)}, // Unique local address | ||
| {IP: net.ParseIP("fe80::"), Mask: net.CIDRMask(10, 128)}, // Link-local unicast | ||
| {IP: net.ParseIP("::1"), Mask: net.CIDRMask(128, 128)}, // Loopback | ||
| } | ||
|
|
||
| // Check if the IP falls into any of the private ranges | ||
| for _, r := range privateRanges { | ||
| if r.Contains(ip) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // Function to extract IP information from a Multiaddr | ||
| func extractIPFromMultiaddr(addr multiaddr.Multiaddr) (string, error) { | ||
| return addr.ValueForProtocol(multiaddr.P_IP4) |
There was a problem hiding this comment.
The function only handles IPv4 addresses due to the ip.To4() check, but it defines IPv6 private ranges in privateRanges. IPv6 addresses will always return false even if they are private.
| if ip == nil || ip.To4() == nil { | |
| return false | |
| } | |
| // Define private IPv4 and IPv6 ranges | |
| privateRanges := []*net.IPNet{ | |
| // IPv4 | |
| {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, | |
| {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, | |
| {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, | |
| {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, | |
| // IPv6 | |
| {IP: net.ParseIP("fc00::"), Mask: net.CIDRMask(7, 128)}, // Unique local address | |
| {IP: net.ParseIP("fe80::"), Mask: net.CIDRMask(10, 128)}, // Link-local unicast | |
| {IP: net.ParseIP("::1"), Mask: net.CIDRMask(128, 128)}, // Loopback | |
| } | |
| // Check if the IP falls into any of the private ranges | |
| for _, r := range privateRanges { | |
| if r.Contains(ip) { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| // Function to extract IP information from a Multiaddr | |
| func extractIPFromMultiaddr(addr multiaddr.Multiaddr) (string, error) { | |
| return addr.ValueForProtocol(multiaddr.P_IP4) | |
| if ip == nil { | |
| return false | |
| } | |
| // Define private IPv4 and IPv6 ranges | |
| privateIPv4Ranges := []*net.IPNet{ | |
| {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, | |
| {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, | |
| {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, | |
| {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, | |
| } | |
| privateIPv6Ranges := []*net.IPNet{ | |
| {IP: net.ParseIP("fc00::"), Mask: net.CIDRMask(7, 128)}, // Unique local address | |
| {IP: net.ParseIP("fe80::"), Mask: net.CIDRMask(10, 128)}, // Link-local unicast | |
| {IP: net.ParseIP("::1"), Mask: net.CIDRMask(128, 128)}, // Loopback | |
| } | |
| if ip.To4() != nil { | |
| for _, r := range privateIPv4Ranges { | |
| if r.Contains(ip) { | |
| return true | |
| } | |
| } | |
| } else if ip.To16() != nil { | |
| for _, r := range privateIPv6Ranges { | |
| if r.Contains(ip) { | |
| return true | |
| } | |
| } | |
| } | |
| return false | |
| } | |
| // Function to extract IP information from a Multiaddr | |
| func extractIPFromMultiaddr(addr multiaddr.Multiaddr) (string, error) { | |
| ipStr, err := addr.ValueForProtocol(multiaddr.P_IP4) | |
| if err == nil && ipStr != "" { | |
| return ipStr, nil | |
| } | |
| ipStr, err = addr.ValueForProtocol(multiaddr.P_IP6) | |
| if err == nil && ipStr != "" { | |
| return ipStr, nil | |
| } | |
| return "", fmt.Errorf("no IP address found in multiaddr") |
client.go
Outdated
|
|
||
| // Function to extract IP information from a Multiaddr | ||
| func extractIPFromMultiaddr(addr multiaddr.Multiaddr) (string, error) { | ||
| return addr.ValueForProtocol(multiaddr.P_IP4) |
There was a problem hiding this comment.
This function only extracts IPv4 addresses but the isPrivateIP function is designed to handle both IPv4 and IPv6. For IPv6 addresses, this will fail and cause isPrivateIP to incorrectly return false.
| return addr.ValueForProtocol(multiaddr.P_IP4) | |
| ipStr, err := addr.ValueForProtocol(multiaddr.P_IP4) | |
| if err == nil && ipStr != "" { | |
| return ipStr, nil | |
| } | |
| ipStr, err = addr.ValueForProtocol(multiaddr.P_IP6) | |
| if err == nil && ipStr != "" { | |
| return ipStr, nil | |
| } | |
| return "", fmt.Errorf("no IP address found in multiaddr") |
client.go
Outdated
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return string(body), resp.Body.Close() |
There was a problem hiding this comment.
The response body should be closed before returning, not as part of the return statement. If resp.Body.Close() returns an error, it will be ignored. The body should be closed in a defer statement or before the return.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), nil |
client.go
Outdated
| "10.0.0.0/8", // Private network 10.0.0.0 to 10.255.255.255 | ||
| "172.16.0.0/12", // Private network 172.16.0.0 to 172.31.255.255 | ||
| "192.168.0.0/16", // Private network 192.168.0.0 to 192.168.255.255 | ||
| "127.0.0.0/16", // Local network |
There was a problem hiding this comment.
Incorrect CIDR notation for localhost. The loopback network should be '127.0.0.0/8', not '127.0.0.0/16'. The /16 mask would only cover 127.0.x.x instead of the full 127.x.x.x range.
| "127.0.0.0/16", // Local network | |
| "127.0.0.0/8", // Local network |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
client.go
Outdated
| // Ignore errors because we don't care if we can't find it | ||
| ifconfig, _ := GetPublicIP(context.Background()) | ||
| if len(ifconfig) > 0 { | ||
| addr, _ := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%s", ifconfig, config.Port)) |
There was a problem hiding this comment.
The format string uses %s for config.Port which is an integer. This should be %d to properly format the port number.
| addr, _ := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%s", ifconfig, config.Port)) | |
| addr, _ := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%d", ifconfig, config.Port)) |
client.go
Outdated
| "10.0.0.0/8", // Private network 10.0.0.0 to 10.255.255.255 | ||
| "172.16.0.0/12", // Private network 172.16.0.0 to 172.31.255.255 | ||
| "192.168.0.0/16", // Private network 192.168.0.0 to 192.168.255.255 | ||
| "127.0.0.0/16", // Local network |
There was a problem hiding this comment.
The CIDR mask for localhost should be /8 not /16. The standard localhost range is 127.0.0.0/8 which covers 127.0.0.0 to 127.255.255.255.
| "127.0.0.0/16", // Local network | |
| "127.0.0.0/8", // Local network |
client.go
Outdated
|
|
||
| // Function to extract IP information from a Multiaddr | ||
| func extractIPFromMultiaddr(addr multiaddr.Multiaddr) (string, error) { | ||
| return addr.ValueForProtocol(multiaddr.P_IP4) |
There was a problem hiding this comment.
This function only extracts IPv4 addresses but is used in isPrivateIP which handles both IPv4 and IPv6. It should also handle IPv6 addresses using multiaddr.P_IP6.
| return addr.ValueForProtocol(multiaddr.P_IP4) | |
| // Try IPv4 first | |
| ipStr, err := addr.ValueForProtocol(multiaddr.P_IP4) | |
| if err == nil && ipStr != "" { | |
| return ipStr, nil | |
| } | |
| // Try IPv6 | |
| ipStr, err = addr.ValueForProtocol(multiaddr.P_IP6) | |
| if err == nil && ipStr != "" { | |
| return ipStr, nil | |
| } | |
| return "", fmt.Errorf("no IP address found in multiaddr: %s", addr.String()) |
client.go
Outdated
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return string(body), resp.Body.Close() |
There was a problem hiding this comment.
The response body should be closed before returning, not as part of the return statement. If resp.Body.Close() returns an error, it will be ignored. The body should be closed in a defer statement or the close error should be handled properly.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer func() { | |
| if cerr := resp.Body.Close(); cerr != nil { | |
| log.Printf("error closing response body: %v", cerr) | |
| } | |
| }() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), nil |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| config.Logger.Infof("Extracted IP: %s", ipStr) | ||
| ip := net.ParseIP(ipStr) | ||
| if ip == nil || ip.To4() == nil { |
There was a problem hiding this comment.
The condition ip.To4() == nil will incorrectly return false for all IPv6 addresses, even though the function handles IPv6 private ranges. This should check if the IP is neither IPv4 nor IPv6, or handle IPv6 addresses properly.
| if ip == nil || ip.To4() == nil { | |
| if ip == nil { |
client.go
Outdated
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return string(body), resp.Body.Close() |
There was a problem hiding this comment.
The return statement combines the response body with the Close() error, which will always return an error type as the second value instead of a string. The body should be closed in a defer statement and only the string should be returned.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), nil |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
client.go:89
- This code creates a duplicate AddrsFactory that will be overridden by the addressFactory defined later in the function. This renders the custom announce addresses ineffective when AllowPrivateIPs filtering is applied.
hostOpts = append(hostOpts, libp2p.AddrsFactory(func([]multiaddr.Multiaddr) []multiaddr.Multiaddr {
return announceAddrs
}))
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
client.go
Outdated
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return string(body), resp.Body.Close() |
There was a problem hiding this comment.
Returning the result of resp.Body.Close() as the error. This should be handled separately - close the body and return nil as the error, or store the close error in a variable if needed.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), nil |
client.go
Outdated
| var publicAddrs []multiaddr.Multiaddr | ||
| for _, addr := range addrs { | ||
| // if IP is not private, add it to the list | ||
| logger.Infof("address is private? %v", isPrivateIP(config, addr)) |
There was a problem hiding this comment.
Multiple debug log statements in address filtering logic that will be called frequently during network operations. Consider using Debug level logging or removing verbose logging to avoid performance impact in production.
| logger.Infof("address is private? %v", isPrivateIP(config, addr)) | |
| logger.Debugf("address is private? %v", isPrivateIP(config, addr)) |
client.go
Outdated
|
|
||
| // Function to check if an IP address is private | ||
| func isPrivateIP(config Config, addr multiaddr.Multiaddr) bool { | ||
| config.Logger.Infof("Multiaddr: %s", addr.String()) |
There was a problem hiding this comment.
Multiple debug log statements in address filtering logic that will be called frequently during network operations. Consider using Debug level logging or removing verbose logging to avoid performance impact in production.
client.go
Outdated
| return false | ||
| } | ||
|
|
||
| config.Logger.Infof("Parsed IP: %s", ip.String()) |
There was a problem hiding this comment.
Multiple debug log statements in address filtering logic that will be called frequently during network operations. Consider using Debug level logging or removing verbose logging to avoid performance impact in production.
client.go
Outdated
| // Check if the IP falls into any of the private ranges or is loopback (::1) | ||
| for _, r := range privateRanges { | ||
| if r.Contains(ip) { | ||
| config.Logger.Infof("Found private IP: %s", ip) |
There was a problem hiding this comment.
Multiple debug log statements in address filtering logic that will be called frequently during network operations. Consider using Debug level logging or removing verbose logging to avoid performance impact in production.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
client.go
Outdated
| } | ||
| if len(ifconfig) > 0 { | ||
| logger.Infof("Public IP address: %v", ifconfig) | ||
| addr, err := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%d", ifconfig, config.Port)) |
There was a problem hiding this comment.
The format specifier should be %d instead of %s for config.Port since it's an integer type.
| addr, err := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%d", ifconfig, config.Port)) | |
| addr, err := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%d", ifconfig, config.Port)) |
client.go
Outdated
| "10.0.0.0/8", // Private network 10.0.0.0 to 10.255.255.255 | ||
| "172.16.0.0/12", // Private network 172.16.0.0 to 172.31.255.255 | ||
| "192.168.0.0/16", // Private network 192.168.0.0 to 192.168.255.255 | ||
| "127.0.0.0/16", // Local network |
There was a problem hiding this comment.
The CIDR mask for localhost should be /8 not /16. The correct localhost range is 127.0.0.0/8.
| "127.0.0.0/16", // Local network | |
| "127.0.0.0/8", // Local network |
client.go
Outdated
| func isPrivateIP(config Config, addr multiaddr.Multiaddr) bool { | ||
| config.Logger.Infof("Multiaddr: %s", addr.String()) | ||
| ipStr, err := extractIPFromMultiaddr(addr) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| config.Logger.Infof("Extracted IP: %s", ipStr) |
There was a problem hiding this comment.
These debug log statements should be at debug level rather than info level, or removed entirely for production code as they will create excessive logging.
| } | ||
|
|
||
| ip := net.ParseIP(ipStr) | ||
| if ip == nil || ip.To4() == nil { |
There was a problem hiding this comment.
This logic incorrectly rejects IPv6 addresses. The ip.To4() == nil check means IPv6 addresses will always return false, but IPv6 private ranges are defined below. Remove the ip.To4() == nil condition.
| if ip == nil || ip.To4() == nil { | |
| if ip == nil { |
client.go
Outdated
| var publicAddrs []multiaddr.Multiaddr | ||
| for _, addr := range addrs { | ||
| // if IP is not private, add it to the list | ||
| logger.Infof("address is private? %v", isPrivateIP(config, addr)) |
There was a problem hiding this comment.
This debug log statement should be at debug level rather than info level to avoid excessive logging in production.
| logger.Infof("address is private? %v", isPrivateIP(config, addr)) | |
| logger.Debugf("address is private? %v", isPrivateIP(config, addr)) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| var announceAddrs []multiaddr.Multiaddr | ||
| if len(config.AnnounceAddrs) > 0 { |
There was a problem hiding this comment.
The announceAddrs variable is declared but the existing address factory logic using config.AnnounceAddrs is removed without preserving its functionality. This will break the original announce address feature when both custom announce addresses and the new address factory are used together.
client.go
Outdated
| if len(publicAddrs) == 0 { | ||
| // If no public addresses are set, let's attempt to grab it publicly | ||
| // Ignore errors because we don't care if we can't find it | ||
| ifconfig, err := GetPublicIP(context.Background()) |
There was a problem hiding this comment.
Using context.Background() instead of the existing context (ctx) breaks the cancellation chain. This HTTP request won't be cancelled when the client shuts down, potentially causing resource leaks.
| ifconfig, err := GetPublicIP(context.Background()) | |
| ifconfig, err := GetPublicIP(ctx) |
client.go
Outdated
| "10.0.0.0/8", // Private network 10.0.0.0 to 10.255.255.255 | ||
| "172.16.0.0/12", // Private network 172.16.0.0 to 172.31.255.255 | ||
| "192.168.0.0/16", // Private network 192.168.0.0 to 192.168.255.255 | ||
| "127.0.0.0/16", // Local network |
There was a problem hiding this comment.
The CIDR 127.0.0.0/16 is incorrect for localhost addresses. It should be 127.0.0.0/8 to properly cover the entire 127.x.x.x range used for loopback addresses.
| "127.0.0.0/16", // Local network | |
| "127.0.0.0/8", // Local network (loopback) |
| {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, | ||
| {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, | ||
| {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, | ||
| {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, |
There was a problem hiding this comment.
The private IP ranges are duplicated between the connection gater logic (lines 139-146) and the isPrivateIP function (lines 728-737). This duplication could lead to inconsistencies if one is updated without the other.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
client.go:87
- Duplicate address factory logic. The same AddrsFactory is being configured twice - once here and again later (lines 118-120). This creates redundant configuration and potential conflicts.
hostOpts = append(hostOpts, libp2p.AddrsFactory(func([]multiaddr.Multiaddr) []multiaddr.Multiaddr {
return announceAddrs
}))
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| ip := net.ParseIP(ipStr) | ||
| if ip == nil || ip.To4() == nil { |
There was a problem hiding this comment.
IPv6 addresses will be incorrectly classified as non-private. The condition ip.To4() == nil returns false for all IPv6 addresses, causing the function to return false even for private IPv6 ranges that are defined later in the function.
| if ip == nil || ip.To4() == nil { | |
| if ip == nil { |
| return priv, nil | ||
| } | ||
|
|
||
| // Function to check if an IP address is private |
There was a problem hiding this comment.
Function comment is incomplete. It should document the return value, parameters, and provide examples of what constitutes a private IP address.
| // Function to check if an IP address is private | |
| // isPrivateIP checks if the given multiaddr represents a private IP address. | |
| // | |
| // Parameters: | |
| // addr: multiaddr.Multiaddr - The multiaddr to check. It should contain an IPv4 or IPv6 address. | |
| // | |
| // Returns: | |
| // bool - Returns true if the IP address is private (e.g., RFC1918 for IPv4, unique local or link-local for IPv6, or loopback addresses), false otherwise. | |
| // | |
| // Examples of private IP addresses: | |
| // IPv4: "10.0.0.1", "172.16.5.4", "192.168.1.100", "127.0.0.1" | |
| // IPv6: "::1" (loopback), "fc00::1" (unique local), "fe80::1" (link-local) |
| return false | ||
| } | ||
|
|
||
| // Function to extract IP information from a Multiaddr (supports IPv4 and IPv6) |
There was a problem hiding this comment.
Function comment is incomplete. It should document the return values, error conditions, and provide examples of expected input/output formats.
| // Function to extract IP information from a Multiaddr (supports IPv4 and IPv6) | |
| // extractIPFromMultiaddr extracts the IP address (IPv4 or IPv6) from a multiaddr.Multiaddr. | |
| // | |
| // Returns: | |
| // - string: The extracted IP address as a string if found. | |
| // - error: An error if the IP address cannot be extracted or the multiaddr does not contain an IP4/IP6 protocol. | |
| // | |
| // Error conditions: | |
| // - If the multiaddr does not contain an IP4 or IP6 protocol, an error is returned. | |
| // - If the protocol value cannot be extracted, an error is returned. | |
| // | |
| // Example: | |
| // addr, _ := multiaddr.NewMultiaddr("/ip4/127.0.0.1/tcp/4001") | |
| // ip, err := extractIPFromMultiaddr(addr) | |
| // // ip == "127.0.0.1", err == nil | |
| // | |
| // addr, _ := multiaddr.NewMultiaddr("/ip6/::1/tcp/4001") | |
| // ip, err := extractIPFromMultiaddr(addr) | |
| // // ip == "::1", err == nil | |
| // | |
| // addr, _ := multiaddr.NewMultiaddr("/tcp/4001") | |
| // ip, err := extractIPFromMultiaddr(addr) | |
| // // ip == "", err != nil |
|
mrz1836
changed the title
mrz1836
added
feature
|
Handled in #14 |
This pull request introduces improved handling of network addresses and IP filtering in the P2P client. The main enhancements are the ability to filter out private IPs from being broadcast or dialed (unless explicitly allowed), more flexible configuration of announce addresses and listening ports, and the addition of helper functions to support these features.
Network Address Handling and Filtering:
AllowPrivateIPsoption and a configurablePortto theConfigstruct, enabling users to control whether private IPs are allowed and which port to listen on (config.go).NewClientthat filters out private IPs from advertised addresses unlessAllowPrivateIPsis set, appends user-specified announce addresses, and attempts to fetch a public IP fromifconfig.meif needed (client.go).AllowPrivateIPsis true (client.go).Helper Functions and Utilities:
isPrivateIP,extractIPFromMultiaddr, andGetPublicIPhelper functions to identify private IPs, extract IPs from multiaddrs, and fetch the public IP address, respectively (client.go).Dependency and Import Updates:
client.go). [1] [2]