Add interface / source-IP binding for connections (#2286) - #576
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds configurable source-IP and interface binding with strict and fallback behavior, platform-specific socket pinning, TCP/UDP/WebSocket integration, dependency updates, and loopback tests. ChangesNetwork binding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WebSocketClient
participant BoundTcpConnector
participant TcpSocket
participant TLSWebSocket
WebSocketClient->>BoundTcpConnector: resolve target and select local binding
BoundTcpConnector->>TcpSocket: create, pin, and connect TCP socket
TcpSocket-->>BoundTcpConnector: return connected stream
BoundTcpConnector->>TLSWebSocket: provide pre-connected stream
TLSWebSocket-->>WebSocketClient: complete TLS and WebSocket handshake
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/bind_interface.rs (1)
85-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the strict-mode negative case to this test.
The most security-relevant guarantee — strict mode must not leak out another interface — is only asserted at the decision-logic level (Lines 61-65). Add a case that sets
("ip", "203.0.113.7", "Y")and assertsconnect_tcpreturns an error, so a future regression in the strict path is caught end to end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bind_interface.rs` around lines 85 - 95, Add an end-to-end strict binding case in the existing bind-interface test: configure set_bind with ("ip", "203.0.113.7", "Y"), call socket_client::connect_tcp, and assert it returns an error. Keep the existing non-strict fallback and no-binding success cases unchanged.src/tcp.rs (1)
79-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated per-platform device-pinning block in
src/tcp.rsandsrc/udp.rs. The root cause is that the "get device → pin socket → honor strict" sequence is inlined at each call site instead of living next tobind_socket_to_interfaceinsrc/config.rs; the result is four near-identicalcfgarms that must stay in sync.
src/tcp.rs#L79-L107: replace bothcfgarms with a single call to a new shared helper (e.g.config::apply_bind_device(&socket, addr.is_ipv4())) that resolves the raw handle and the strict flag internally.src/udp.rs#L44-L71: replace bothcfgarms with the same helper call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tcp.rs` around lines 79 - 107, Extract the duplicated device-pinning and strict-error handling into a shared config helper alongside bind_socket_to_interface, such as apply_bind_device, resolving the platform-specific raw handle and strict flag internally; replace both cfg arms in src/tcp.rs lines 79-107 and src/udp.rs lines 44-71 with the same helper call using the socket reference and address IP version, preserving strict-mode error propagation.src/config.rs (1)
919-931: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching interface enumeration.
get_bind_source_ipre-reads three options and, vialocal_ip_exists/interface_source_ip, callsnetif::get_interfaces()on every invocation. It's hit per socket creation (tcp::new_socket,udp::new_socket, and twice inlisten_any), so UDP punch bursts and reconnect loops pay a full interface enumeration each time. A short-TTL cache (or memoizing whenmodeis empty, which is the default) would keep the default path free.⚡ Cheap early-out for the default (unconfigured) case
pub fn get_bind_source_ip(is_ipv4: bool) -> Option<IpAddr> { - let mode = Self::get_option(keys::OPTION_BIND_MODE); let value = Self::get_option(keys::OPTION_BIND_VALUE); + if value.is_empty() { + return None; + } + let mode = Self::get_option(keys::OPTION_BIND_MODE); let strict = Self::get_bool_option(keys::OPTION_BIND_STRICT);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.rs` around lines 919 - 931, Optimize get_bind_source_ip by avoiding repeated option reads and interface enumeration when bind mode is unconfigured, returning the default result immediately for the empty-mode case. For configured modes, add a short-TTL cache or equivalent memoization around the local_ip_exists/interface_source_ip interface lookup path, preserving existing bind decision behavior.Cargo.toml (1)
74-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid pinning deprecated
default-net0.14 if migration is feasible.
default-netis superseded bynetdev, soversion = "0.14"leaves this crate outside newer fixes. The dependency is already under[target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies], so mobile builds are not the issue here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` around lines 74 - 77, Update the target-specific netif dependency in Cargo.toml to use the maintained netdev crate instead of the deprecated default-net 0.14 package. Adjust the dependency declaration and any corresponding Rust imports or API usage so interface/address enumeration and outgoing socket binding retain their current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/bind_probe.rs`:
- Around line 27-32: The bind probe’s option setup via Config::set_option
permanently modifies persisted bind settings. In the probe’s configuration flow,
reuse the BindRestore save/restore pattern from tests/bind_interface.rs (or
ensure restoration occurs before std::process::exit), covering OPTION_BIND_MODE,
OPTION_BIND_VALUE, and OPTION_BIND_STRICT while preserving the probe’s temporary
values during execution.
In `@src/tcp.rs`:
- Around line 259-263: Update the direct-access listener path around
get_bind_source_ip so fail-closed loopback sentinels are not treated as resolved
bind addresses; return an explicit error or preserve the unspecified bind
behavior instead, and emit a warn log when loopback fallback is used. Also
preserve the prior dual-stack listener behavior by handling valid IPv4 and IPv6
source addresses independently rather than selecting only the IPv4-first result.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 74-77: Update the target-specific netif dependency in Cargo.toml
to use the maintained netdev crate instead of the deprecated default-net 0.14
package. Adjust the dependency declaration and any corresponding Rust imports or
API usage so interface/address enumeration and outgoing socket binding retain
their current behavior.
In `@src/config.rs`:
- Around line 919-931: Optimize get_bind_source_ip by avoiding repeated option
reads and interface enumeration when bind mode is unconfigured, returning the
default result immediately for the empty-mode case. For configured modes, add a
short-TTL cache or equivalent memoization around the
local_ip_exists/interface_source_ip interface lookup path, preserving existing
bind decision behavior.
In `@src/tcp.rs`:
- Around line 79-107: Extract the duplicated device-pinning and strict-error
handling into a shared config helper alongside bind_socket_to_interface, such as
apply_bind_device, resolving the platform-specific raw handle and strict flag
internally; replace both cfg arms in src/tcp.rs lines 79-107 and src/udp.rs
lines 44-71 with the same helper call using the socket reference and address IP
version, preserving strict-mode error propagation.
In `@tests/bind_interface.rs`:
- Around line 85-95: Add an end-to-end strict binding case in the existing
bind-interface test: configure set_bind with ("ip", "203.0.113.7", "Y"), call
socket_client::connect_tcp, and assert it returns an error. Keep the existing
non-strict fallback and no-binding success cases unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2592928c-ba97-40c1-bfaa-73257b6dd0c3
📒 Files selected for processing (6)
Cargo.tomlexamples/bind_probe.rssrc/config.rssrc/tcp.rssrc/udp.rstests/bind_interface.rs
|
Thanks a lot for your effort. I’ve reviewed the code, but it would require more time to review, clean up, and fix than to reimplement it internally. We’ll close this item and handle the implementation ourselves, likely with AI assistance. No worries—we’ll get it done soon. |
|
After review again, it seems not too bad. |
|
Do we need to introduce all three options? Do you have any examples of other established apps using a similar approach? I’d like to see some reference designs. |
|
Short answer: no, three is one too many. The mode selector is derivable from the value. Established tools split this two ways: Two keys, one per form — no mode selector, whichever is set decides:
One key that accepts either form:
Either beats what I sent. I'd suggest the second: a single On the third key,
The difference is what the tool is for. For a torrent client, failing closed costs you a download. For RustDesk it can cost you the machine: if the bound interface disappears and we fail closed, the host stops listening, and you have lost access to the very box you would have needed it for. Falling back silently is the opposite failure — the VPN user in #2286 gets put back on the interface they asked us to keep off. Both are real and they point in opposite directions, which is why I made the choice explicit and defaulted it to the side that cannot lock you out. So: two keys if you want my recommendation — I am reworking both PRs to that shape now, so you have something concrete to look at instead of a proposal. If you would rather have a single key and always fall back, that is a small change on top and I will do it. Going in at the same time, independently of the naming:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/config.rs (1)
3173-3198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-strict pinning failure is only visible at
debuglevel.When a device is explicitly configured but pinning fails and strict is off, traffic silently egresses via the routing table. A
warnhere (once per socket is noisy, but at least notdebug) makes the degraded state diagnosable.♻️ Suggested change
match bind_socket_to_interface(socket.as_raw_fd(), &device, is_ipv4) { Err(e) if Config::get_bool_option(keys::OPTION_BIND_STRICT) => Err(e), - _ => Ok(()), + Err(e) => { + log::warn!("failed to pin socket to {device}, falling back to routing table: {e}"); + Ok(()) + } + Ok(()) => Ok(()), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.rs` around lines 3173 - 3198, Update both platform-specific apply_bind_device implementations so a bind_socket_to_interface failure with OPTION_BIND_STRICT disabled emits a warn-level diagnostic before returning Ok(()). Preserve strict-mode error propagation and the existing no-device behavior, and include the device and underlying error in the warning.tests/bind_interface.rs (1)
15-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests mutate and persist the host's real config file.
Config::set_optionwritesCONFIG2to disk, so a crash (or an abort that skipsDrop) leavesbind-interface/bind-strictset on the developer's or CI machine, and other test binaries running concurrently in separate processes are not covered byLOCK. Also, once one test panics,LOCK.lock().unwrap()poisons and the other test fails for an unrelated reason — useunwrap_or_else(|e| e.into_inner())so the real assertion failure is what's reported.🧪 Suggested tweak
- let _guard = LOCK.lock().unwrap(); + let _guard = LOCK.lock().unwrap_or_else(|e| e.into_inner());Pointing an app-config path override (e.g.
Config::set_home/APP_DIR) at a temp dir for these tests would remove the host-state dependency entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bind_interface.rs` around lines 15 - 45, Update bind tests around BindRestore::save and set_bind to use a unique temporary app-config directory via the existing Config::set_home/APP_DIR override before reading or writing options, preventing changes to the host configuration. Keep the temporary directory alive for the test and retain restoration for the isolated config. Change LOCK.lock().unwrap() to recover poisoned locks with unwrap_or_else(|e| e.into_inner()) so prior panics do not mask test failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.rs`:
- Around line 2942-2946: Update the mobile `interface_source_ip` and
`decide_bind` flow so a valid interface name is treated as pin-capable even when
its source IP cannot be enumerated. Validate interface availability using the
platform-supported interface lookup or introduce a distinct pin-only decision,
ensuring strict mode preserves device pinning without returning `FailClosed`,
while nonexistent interfaces still fail appropriately.
- Around line 2935-2939: Update the IPv6 branch using the surrounding
interface-address selection logic to prefer the first address that is not
link-local, identified via the manual segments()[0] & 0xffc0 == 0xfe80 check;
fall back to the first IPv6 address when no non-link-local address exists. Leave
the IPv4 behavior unchanged.
- Around line 3330-3336: Update the comments above OPTION_BIND_INTERFACE and
OPTION_BIND_STRICT to describe only the current bind-interface address/interface
behavior and bind-strict fallback behavior, removing references to the obsolete
mode/value/strict three-option design.
---
Nitpick comments:
In `@src/config.rs`:
- Around line 3173-3198: Update both platform-specific apply_bind_device
implementations so a bind_socket_to_interface failure with OPTION_BIND_STRICT
disabled emits a warn-level diagnostic before returning Ok(()). Preserve
strict-mode error propagation and the existing no-device behavior, and include
the device and underlying error in the warning.
In `@tests/bind_interface.rs`:
- Around line 15-45: Update bind tests around BindRestore::save and set_bind to
use a unique temporary app-config directory via the existing
Config::set_home/APP_DIR override before reading or writing options, preventing
changes to the host configuration. Keep the temporary directory alive for the
test and retain restoration for the isolated config. Change LOCK.lock().unwrap()
to recover poisoned locks with unwrap_or_else(|e| e.into_inner()) so prior
panics do not mask test failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7a7e937-5aba-4a36-9eb6-750ddeec72bb
📒 Files selected for processing (5)
Cargo.tomlsrc/config.rssrc/tcp.rssrc/udp.rstests/bind_interface.rs
|
Pushed the rework, rebased on current
One test fix worth flagging on its own: the integration test used
I could not verify Windows/macOS/iOS: there is no cross-toolchain on this machine, so those paths are unchanged-by-inspection only and |
|
Pushed again. Both of CodeRabbit's findings on the reworked code were real, and the websocket gap is now closed too. WebSocket transport is bound now. This was the item from your original list I had left out, and it turned out to matter more than a missing checkbox: I found this the hard way: the loopback integration test was failing on my machine for what looked like an unrelated reason, and the cause was that Android/iOS strict is no longer wrongly fail-closed. You were right that this contradicted the comment.
IPv6 link-local. Confirmed, though the framing was slightly off: on this host the real NIC lists a global address first and link-local last, so it is not "most interfaces". But Stale comment on the option keys: removed.
Still not verified here: Windows/macOS/iOS, for lack of a cross-toolchain. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/websocket.rs`:
- Around line 130-139: Update the connection flow around connect_bound_tcp and
client_async_tls_with_config to use one shared ms_timeout deadline for the
entire TCP connect plus TLS/WebSocket handshake, rather than resetting a full
timeout for each step. Compute the remaining duration after connect_bound_tcp
completes and apply only that remainder to the handshake, preserving the
existing timeout behavior and try_connect retry limits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 26de0bad-1527-474d-bad0-38b4570ab826
📒 Files selected for processing (6)
Cargo.tomlsrc/config.rssrc/tcp.rssrc/udp.rssrc/websocket.rstests/bind_interface.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/udp.rs
- src/tcp.rs
- src/config.rs
Bind sockets to a chosen network interface or source IP, for incoming and outgoing connections, via the bind-interface/bind-strict options (off by default). The value is a source address when it parses as one and an interface name otherwise, the same rule libtorrent uses for outgoing_interfaces. An interface name also pins the socket to the device (SO_BINDTODEVICE on Linux/Android, IP_BOUND_IF on macOS/iOS, IP_UNICAST_IF on Windows) so egress follows it regardless of the routing table. Non-strict falls back to all interfaces when the target is unavailable, strict does not. For egress, strict then binds a source address that cannot reach the peer, so the connect fails instead of leaving via another interface. A listener must not use that sentinel: binding loopback would leave the host "listening" while unreachable from everywhere, with nothing to see from the outside. So listen_any reports an unavailable target as an error, and logs the address it does bind, noting that a bound listener cannot be dual-stack. The websocket transport dials through a socket we bind ourselves rather than letting tungstenite open an unbound one, so it is covered too. Without that, any install with allow-websocket set would silently ignore the binding. Where addresses cannot be enumerated (Android/iOS), an interface name resolves to no source ip. Rather than failing closed on a name that would have worked, that case pins the device only and lets a nonexistent name fail at bind time. For IPv6, skip link-local when picking an interface's source address: bound without a scope id it cannot reach off-link peers. Interfaces that have nothing else (docker0, bridges) still fall back to it. Interface enumeration uses netdev, the maintained successor of default-net. Adds unit tests for the decision matrix, and loopback integration tests for the socket layer, the listener and the websocket transport.
YES, I like this. |
3 participants
Engine side of the interface binding requested in rustdesk/rustdesk#2286 ("Pull request is welcome"). The UI/wiring half is rustdesk/rustdesk#15675.
Off by default — existing installs behave exactly as before until an option is set.
What it does
Binds sockets to a chosen network interface or source IP, for both incoming and outgoing connections. The motivating case: after sleep/wake with a VPN up, the VPN owns the default route, so RustDesk registers and listens via the VPN and the host is no longer reachable on the LAN.
Two options (
src/config.rs):bind-interfacebind-strictY/ empty (default)One key covers both forms: the value is a source address when it parses as one and an interface name otherwise, the same rule libtorrent uses for
outgoing_interfaces. An interface name also pins the socket to the device, so egress follows the interface regardless of the routing table — which is what beats a full-tunnel VPN:SO_BINDTODEVICEIP_BOUND_IFIP_UNICAST_IFNaming and the collapse from three options to two follow the reference designs discussion in this PR.
Fallback, and why the listener differs
Non-strict falls back to all interfaces when the target is unavailable; strict does not. For egress, strict then binds a source address that cannot reach the peer, so the connect fails rather than leaving via another interface.
A listener must not use that sentinel. Binding loopback would leave the host "listening" while unreachable from everywhere, with nothing visible from the outside — so
listen_anyreports an unavailable target as an error instead, and logs the address it does bind. Note a bound listener cannot be dual-stack: v4-mapped acceptance only works on the unspecified address.Scope
Covered: outgoing connections (rendezvous, relay, direct peer), the incoming direct-connection listener, and the WebSocket transport. The last one matters more than it looks:
websocket.rsused to dial via tungstenite's ownconnect_async, which opens an unbound socket — so on any install withallow-websocketset, the binding would have been silently ignored for outgoing connections. It now dials through a socket we bind ourselves, and there is a test asserting the source address actually arrives at the peer.Not covered: the WebRTC transport and the SOCKS/HTTP proxy path, which have their own connection code.
Changes
src/config.rs— options,get_bind_source_ip,get_bind_listen_ip,get_bind_device,apply_bind_device,bind_socket_to_interface, and the decision logicsrc/tcp.rs,src/udp.rs— apply the binding to outgoing sockets and tolisten_anysrc/websocket.rs— dial through a bound socket instead of tungstenite's own connectCargo.toml—netdev(aliased tonetifto avoid clashing with the existingrustdesk-org/default_netfork, which only exposesget_mac) for interface enumeration;winsock2feature onwinapitests/bind_interface.rs— loopback integration testsTesting
Both pass on Linux (107 unit + 3 integration tests), as does the rest of
cargo test(socket_client::tests::test_nat64fails here, but it fails the same way on a cleanmain— it needs working IPv6 to nip.io).Not verified locally: Windows/macOS/iOS. I have no cross-toolchain on this machine, so those paths are unchanged-by-inspection only, and
IP_UNICAST_IF/IP_BOUND_IFhave not been re-run since the rework. A check there would be welcome.Summary by CodeRabbit