Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for specifying interface names as fallback options for outbound connections on Unix platforms. When CIDR-based address selection fails or is not configured, the proxy can now bind to a specific network interface by name (e.g., "eth0") in addition to the existing IP address fallback option.
Key changes:
- Introduced a new
Fallbackenum to distinguish between IP addresses and interface names (Unix only) - Implemented interface binding using
SO_BINDTODEVICEon Linux andIP_BOUND_IF/IPV6_BOUND_IFon macOS/BSD systems - Refactored module structure by moving
connect,extension, andrandmodules fromsrc/serverto the rootsrclevel
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/connect.rs | Added Fallback enum with FromStr implementation, interface binding logic for TCP sockets, and updated all connector methods to handle the new fallback type |
| src/ext.rs | Moved from src/server/extension.rs, contains extension parsing logic for TTL, Range, and Session extensions |
| src/rand.rs | Moved from src/server/rand.rs, provides thread-local random number generation using xorshift algorithms |
| src/main.rs | Updated CLI argument definitions with improved documentation, changed fallback field type to Fallback, and adjusted module imports |
| src/server.rs | Removed local module definitions (connect, extension, rand) and updated imports to reference root-level modules |
| src/server/socks.rs | Updated imports to reference connect and ext modules from root level |
| src/server/http.rs | Updated imports to reference connect and ext modules from root level |
| src/server/socks/auth.rs | Updated imports to reference ext module from root level |
| Cargo.toml | Updated dependency versions (tokio, hyper, bytes, thiserror, etc.) and added socket2 for low-level socket operations |
Comments suppressed due to low confidence (7)
src/connect.rs:267
- [nitpick] The comment on lines 255-258 mentions using
IP_BOUND_IF, but this is actually handled viabind_device_by_index_v4andbind_device_by_index_v6methods. The comment should be more precise about the different socket options used:IP_BOUND_IFfor IPv4 andIPV6_BOUND_IFfor IPv6.
src/connect.rs:223 - The documentation comment incorrectly describes this function. It states "Creates a [
TcpSocket] and binds it to the provided IP address" but this function now handles both IP addresses and interface names (on Unix). Consider updating to: "Creates a [TcpSocket] and binds it to the fallback (IP address or interface name)".
src/connect.rs:99 - On non-Unix platforms, interface names are not supported and should fail parsing. However, the error message references
{err}which is the error from parsing as anIpAddr, not a meaningful message about interface names being unsupported on the platform. Consider a clearer error message like "Interface names are only supported on Unix platforms. Please provide an IP address."
src/connect.rs:483 - Missing handling for
Fallback::Interfacecase in UDP socket creation. When fallback is an interface name (Unix only), the code doesn't handle binding the UDP socket to that interface. This creates inconsistency with the TCP connector which does support interface binding. Consider adding interface binding support for UDP sockets as well.
src/connect.rs:644 - Missing handling for mixed CIDR and Interface fallback scenarios. When CIDR is V4 and fallback is an interface, or when CIDR is V6 and fallback is an interface, the code falls into the empty
_arm doing nothing. This means the interface setting is silently ignored. Consider either setting the interface or documenting why this combination is not supported.
src/connect.rs:202 - Missing handling for
Fallback::Interfacein the match arm. When CIDR isNoneand fallback is an interface name, the code falls through to the default case which won't properly bind to the interface. Consider adding explicit handling for(None, Some(Fallback::Interface(iface)))to create a socket bound to the specified interface.
src/connect.rs:276 - Potential issue: The
if_nametoindexfunction can return 0 for an invalid interface name (as per POSIX specification), which would fail to create aNonZeroU32. However, the error message format string uses{interface:?}which will print theCStringrepresentation. Consider usinginterface.to_string_lossy()or similar to provide a more readable interface name in the error message.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.