diff --git a/src/client.rs b/src/client.rs index f6f1ff2d3..e52baa662 100644 --- a/src/client.rs +++ b/src/client.rs @@ -207,33 +207,12 @@ impl Drop for OffererGuard { } } -/// Race the WebRTC connect attempt against the other transport futures, preferring P2P. -/// -/// A plain `select_ok` would almost always pick the relay: its TCP connect completes in one -/// round trip while WebRTC needs candidate trickle + ICE checks + DTLS + SCTP. Instead: -/// - an `is_p2p` result from either side (WebRTC, or e.g. IPv6 direct from `others`) is committed -/// immediately; -/// - a relayed result from *either* side inside the preference window is *held*, giving the other -/// side until the window expires to land something direct. `webrtc_fut` is a whole punch -/// attempt, not just the WebRTC connect, so it too can end in a relay, and committing that -/// immediately would preempt a direct punch still in flight on the other branch. The window -/// starts when the first relay is ready, not when setup begins, so rendezvous latency cannot -/// consume the preference budget. Unfinished `others` keep racing so a slower direct transport -/// can still win; when the window ends (or the other side fails) the held connection is -/// committed; -/// - a failure on one side commits the survivor as soon as it succeeds; two failures compose -/// into one error. +/// Race WebRTC against the other transports, preferring P2P: `select_ok` would always pick the +/// relay, whose TCP connect beats ICE + DTLS + SCTP by an order of magnitude. An `is_p2p` result +/// wins outright; a relayed one — from either side, since `webrtc_fut` is a whole punch attempt +/// that can also end in a relay — is held for `window_ms` to give the other side a chance. /// /// `others` must be non-empty (`select_ok` requires it). -/// Whether a transport label names a peer-to-peer path rather than the RustDesk relay. -/// -/// The preference window exists to let one of these beat a relay that connects sooner, so a -/// label misclassified here inverts the race: a direct connection is parked as if it were a -/// relay, and the relay is then committed the moment it arrives. -fn is_direct_transport(typ: &str) -> bool { - !matches!(typ, "Relay" | "WebSocket") -} - async fn race_transports_prefer_webrtc<'a, T: 'a>( webrtc_fut: BoxFuture<'a, ResultType>, others: Vec>>, @@ -583,28 +562,10 @@ impl Client { /// Whether to build a WebRTC offerer for this connection. /// - /// Off by default against a private rendezvous server, like the UDP/IPv6 punch options: - /// a self-hosted deployment opts in once its server (and TURN, if any) is ready. - /// - /// Skips it when a SOCKS proxy is configured: WebRTC's ICE binds its own UDP sockets and - /// speaks STUN directly, which would bypass the proxy policy and can leak the real IP. - /// WebSocket mode does NOT skip it: ws only tunnels the signaling/relay legs to the server - /// (and `connect_tcp` is ws-aware for them), while ICE remains the only viable P2P path in - /// ws deployments where classic punching is forced to relay. Independent of the udp-punch - /// option too — the offer rides any request, and a server or peer without WebRTC support - /// drops the field, so the race simply proceeds without it. - /// Under relay-by-POLICY (force-always-relay option or an explicit relay request) the pc - /// uses Relay-only ICE, which gathers nothing and can never connect unless a TURN server - /// is configured, so skip building a guaranteed-dead pc + STUN/TURN gathering + answerer - /// signaling in that case. WebSocket-forced relay is deliberately NOT policy: ws only - /// tunnels the signaling/relay legs, so the offer keeps full ICE and may land a direct - /// connection - the flagship path for ws deployments (the offer envelope's `ice_policy` - /// key tells the peer). - /// When policy relay *and* TURN are configured, WebRTC via TURN is a valid "relayed" path: - /// `connect` keeps a WebRTC win instead of replacing it with the RustDesk relay, and the - /// RelayResponse path races it without a P2P preference delay. Any request carrying an offer - /// keeps its rendezvous socket for trickle signaling and never reuses it for TCP punching; a - /// separate offer-less request provides the TCP fallback when force-relay is not requested. + /// A SOCKS proxy rules it out: ICE binds its own UDP sockets and speaks STUN directly, past + /// the proxy and with the real IP. Relay-by-policy without TURN rules it out too, since + /// Relay-only ICE can then gather nothing. WebSocket does not: it tunnels only the signaling + /// and relay legs, so the offer keeps full ICE and direct is exactly what it is there for. fn should_create_webrtc_offerer(interface: &impl Interface) -> bool { if !crate::get_webrtc_enabled() { return false; @@ -638,13 +599,9 @@ impl Client { /// Bridge local ICE candidates to the peer over the punch socket, and feed the peer's back /// into the pc. /// - /// This socket must not be reconnected on error, unlike the controlled side's sender which - /// dials a fresh connection per candidate. Its address *is* the return route: the server - /// mangles it into `PunchHole.socket_addr`, the peer echoes it back as - /// `IceCandidate.socket_addr`, and the server resolves it through `tcp_punch`. A reconnect - /// would arrive from a new address that no route points at, and the server drops the old - /// entry when this connection closes — so once it dies, both directions are dead and - /// abandoning WebRTC for another transport is the correct response, not retrying. + /// Never reconnect this socket: its address *is* the return route (the server mangles it into + /// `PunchHole.socket_addr` and resolves the echo through `tcp_punch`), so a new address is one + /// nothing points at. Once it dies, both directions are dead — abandon WebRTC, do not retry. fn spawn_webrtc_ice_bridge( mut socket: Stream, mut local_ice_rx: Option>, @@ -1571,17 +1528,10 @@ impl Client { let sign_pk = match sign_pk { Some(v) => v, None => { - // No trusted peer identity: either the deployment provides none (empty - // signed_id_pk, key-less) or the blob does not verify under our configured root - // (typical benign cause: self-hosted server with the client not configured with - // its key, so verification runs against the built-in RS_PUB_KEY). Either way no - // fingerprint binding is possible, which is the same cryptographic state as - // key-less: DTLS-encrypted to an unauthenticated peer. Proceed like TCP's - // non-secure fallback with is_secured() left false. Bailing here instead would - // only push the session onto a relay where the same blob fails verification - // again and the payload then runs in plaintext — strictly worse than - // unauthenticated DTLS, while an active attacker reaches the same warned, - // unauthenticated endpoint either way. + // No trusted peer identity (key-less deployment, or a blob that does not verify + // under our root), so no fingerprint binding is possible. Fall through like TCP's + // non-secure path with is_secured() false: bailing would only move the session to + // a relay that fails the same check and then runs in plaintext. // send an empty message out in case server is setting up secure and waiting for first message conn.send(&Message::new()).await?; return Ok(option_pk); @@ -1595,11 +1545,9 @@ impl Client { if let Ok((id, their_pk_b, signed_fp)) = decode_id_pk_dtls(&si.id, &sign_pk) { if id == peer_id { // WebRTC only: bind the DTLS channel to the verified peer identity. - // webrtc-rs already verified the peer certificate matches the remote - // SDP fingerprint, so requiring the peer to have signed that same - // fingerprint defeats a rendezvous/relay MITM that swaps SDPs. Fail - // closed: an unreadable/empty/mismatched fingerprint aborts the - // WebRTC connection rather than silently accepting it. + // webrtc-rs already bound the certificate to the remote SDP, so + // requiring the peer to have SIGNED that fingerprint defeats a + // rendezvous/relay MITM that swaps SDPs. Fail closed. if is_webrtc { let actual_fp = conn.dtls_fingerprint(false).await.ok_or_else( || anyhow!("WebRTC DTLS fingerprint unavailable"), @@ -2605,14 +2553,11 @@ pub struct LoginConfigHandler { // reconnect before the real reboot disconnect. restart_remote_device_at: Option, pub force_relay: bool, - // The policy component of force_relay: peer_relay plus proxy, WITHOUT the WebSocket - // transport. ws kills classic TCP/UDP punching (force_relay stays set for those paths) - // but says nothing about ICE, so the WebRTC decisions - offer ICE policy, prefer-P2P - // racing - key off this instead: relay-by-policy must stay Relay-only ICE, while - // relay-by-transport may still go direct over full ICE. + // force_relay minus the WebSocket transport. ws forces relay for classic punching but says + // nothing about ICE, so every WebRTC decision keys off this: policy means Relay-only ICE, + // transport may still go direct. pub policy_relay: bool, - // The peer-scoped component: this peer's saved force-always-relay option, or an explicit - // relay request for it. The only part that may be written back into the peer's config. + // The peer-scoped part of it, and the only part that may be written back to the peer. pub peer_relay: bool, pub direct: Option, pub received: bool, @@ -2726,12 +2671,9 @@ impl LoginConfigHandler { self.session_id = sid; self.supported_encoding = Default::default(); self.clear_restarting_remote_device(); - // Three scopes, and only the first belongs in the peer's saved config: what was decided - // about THIS PEER (its saved option, or an explicit relay request such as an `/r` id or - // a retry-via-relay), versus what is true of this client right now (a proxy, WebSocket). - // Persisting either of the latter turns a transient local setup into a permanent property - // of the peer — and relay-by-policy means Relay-only ICE, so WebRTC never goes direct to - // it again. + // Three scopes: what was decided about this PEER, what this CLIENT is set up as (proxy), + // and what its TRANSPORT forces (ws). Only the first may be written back to the peer's + // config — persisting the others would make a local setup a permanent peer property. self.peer_relay = config::option2bool("force-always-relay", &self.get_option("force-always-relay")) || force_relay; @@ -5284,7 +5226,7 @@ async fn udp_nat_connect( #[cfg(test)] mod webrtc_race_tests { - use super::{is_direct_transport, race_transports_prefer_webrtc, request_allows_tcp_punch}; + use super::{race_transports_prefer_webrtc, request_allows_tcp_punch}; use hbb_common::{ anyhow::anyhow, futures::future::{BoxFuture, FutureExt}, @@ -5317,35 +5259,19 @@ mod webrtc_race_tests { assert!(!request_allows_tcp_punch("webrtc://offer")); } - // The transport labels the RelayResponse race actually runs on. Its predicate has to - // recognise the WebRTC branch's own label as direct, or a WebRTC connection that completes - // BEFORE the relay is parked as if it were a relay and the relay is committed on arrival — - // inverting the race exactly on the fast networks where ICE beats a TCP relay connect. - #[test] - fn transport_labels_are_classified_as_direct_or_relayed() { - for direct in ["WebRTC", "TCP", "UDP", "IPv6"] { - assert!(is_direct_transport(direct), "{direct} is a direct path"); - } - for relayed in ["Relay", "WebSocket"] { - assert!( - !is_direct_transport(relayed), - "{relayed} goes via the relay" - ); - } - } - + // A direct result must win even when it lands first — the LAN ordering, where ICE beats the + // relay's TCP connect. Parking it as if it were a relay commits the relay on arrival. #[tokio::test] async fn direct_result_wins_even_when_it_arrives_first() { - // Same ordering as a LAN: the preferred branch connects before the relay does. let got = race_transports_prefer_webrtc( - ok_after(10, "WebRTC"), - vec![ok_after(120, "Relay")], + ok_after(10, "direct"), + vec![ok_after(120, "relay")], 60_000, - |result| is_direct_transport(result), + |result| *result == "direct", ) .await .unwrap(); - assert_eq!(got, "WebRTC"); + assert_eq!(got, "direct"); } #[tokio::test] diff --git a/src/common.rs b/src/common.rs index eada0adcc..90abefb3f 100644 --- a/src/common.rs +++ b/src/common.rs @@ -2454,23 +2454,11 @@ pub fn is_udp_disabled() -> bool { pub const OPTION_ENABLE_KCP_CC: &str = "enable-kcp-congestion-control"; -/// Whether to run KCP with its built-in congestion window (nc=0) instead of the pure turbo -/// profile (nc=1) it has always shipped with. +/// Run KCP with its congestion window (nc=0) instead of the turbo profile it has always shipped. /// -/// Opt-in, deliberately: switching it on is a transport-behavior change for every session, and -/// which profile wins depends on why packets are being lost. -/// -/// - nc=1 never shrinks the send window. On a link that is genuinely congested it keeps pushing, -/// deepening the loss it is reacting to and crowding out other traffic on the same uplink. -/// - nc=0 adds KCP's congestion window, whose backoff is blunt: a fast retransmit halves it, but -/// an RTO sets `cwnd = 1` outright (ikcp.c) and the recovery slow-starts from one packet. On a -/// link with random loss but no congestion — Wi-Fi interference, a long-haul path — that reads -/// loss as congestion and can stall an interactive video stream for seconds. -/// -/// Neither is safely decidable from reasoning, and a loopback benchmark cannot settle it: with -/// no bottleneck queue there is no congestion to control, so it would flatter nc=1 by -/// construction. Until there is evidence from a shaped link or the field, keep the profile users -/// already run and let anyone who wants the other one ask for it. +/// Opt-in: which profile wins depends on why packets are lost — nc=1 deepens real congestion, +/// while nc=0 reads random loss as congestion and its RTO backoff drops cwnd to 1. Undecidable +/// without a shaped link, so keep what users run today. #[inline] pub fn get_kcp_cc_enabled() -> bool { Config::get_option(OPTION_ENABLE_KCP_CC) == "Y" diff --git a/src/kcp_stream.rs b/src/kcp_stream.rs index eead2a472..2c4cfe4bd 100644 --- a/src/kcp_stream.rs +++ b/src/kcp_stream.rs @@ -127,19 +127,10 @@ impl KcpStream { let udp = udp_socket.clone(); tokio::spawn(async move { let mut buf = vec![0; 1500]; - // A connected UDP socket surfaces ICMP port-unreachable as an error on - // send/recv (WSAECONNRESET 10054 on Windows, ECONNREFUSED on Linux). For UDP - // these are advisory: a stray ICMP from a NAT rebind glitch or a momentary - // peer hiccup does not mean the path is dead, and KCP retransmits through it. - // Treat socket errors as packet loss instead of tearing the session down; - // a truly dead link is reaped by the KCP pong timeout / app-level timeouts. - // The short sleep prevents a persistently failing socket from busy-spinning. - // - // These repeat every 10ms while the socket stays broken, so throttle the line — - // debug output is written to the log file. One throttle per direction: an ICMP - // error on a connected socket is reported once and cleared, so the steady state is - // an alternation (send succeeds, the next recv reports the error), and a shared - // counter would be reset by the succeeding direction on every cycle. + // Socket errors are ICMP unreachable on a connected UDP socket — advisory, and + // routine while a hole forms — so treat them as loss and let KCP's pong timeout reap + // a link that is really dead. One throttle PER DIRECTION: the error is reported once + // and cleared, so send-ok/recv-err alternates and a shared counter never fires. loop { tokio::select! { _ = &mut stop_receiver => { diff --git a/src/rendezvous_mediator.rs b/src/rendezvous_mediator.rs index 233fb485b..dfa7339a8 100644 --- a/src/rendezvous_mediator.rs +++ b/src/rendezvous_mediator.rs @@ -687,15 +687,9 @@ impl RendezvousMediator { /// Build the WebRTC answerer for a punch-hole offer and return the SDP answer that rides in /// the punch reply (PunchHoleSent / RelayResponse). /// - /// This is awaited inline on the punch-reply critical path (handle_punch_hole runs as its own - /// spawned task, so only this reply is delayed), acceptable only because everything awaited - /// here is local-only — - /// pc construction + DTLS cert keygen + SDP answer, sub-millisecond in practice. Trickle ICE - /// makes that possible: the answer carries no candidates; STUN/TURN gathering runs afterward - /// and trickles via IceCandidate messages. Keep network I/O out of this path — actual - /// connection setup (wait_connected + create_tcp_connection) belongs in the detached task - /// below. On error the caller degrades to an empty answer and the punch proceeds without - /// WebRTC. + /// Awaited inline on the punch-reply path, which only holds because everything here is local + /// (pc + keygen + SDP; trickle means the answer carries no candidates). Keep network I/O out + /// — connection setup belongs in the detached task below. async fn spawn_webrtc_answerer( &self, ph: &PunchHole, @@ -870,24 +864,15 @@ impl RendezvousMediator { ph.control_permissions.clone().into_option(), ph.controlled_context.clone().into_option(), ); - // WebRTC opens its own ICE sockets, so it must not run under a SOCKS proxy: candidates - // and STUN bypass the proxy and leak the real IP. WebSocket mode does NOT disable it — - // ws only tunnels the signaling/relay legs to the server, classic punching stays forced - // to relay (`relay` above), and the answer rides the RelayResponse, leaving ICE as the - // only P2P path there. force_relay depends on why it was set, and the offer's envelope - // says which: an `ice_policy: "all"` declaration means the controller's relay is - // transport-forced (ws) and its offer carries every candidate type, so answer with - // full ICE and let a direct pair form; without it the offer is Relay-only ICE by - // policy, viable (and answerable) only through TURN. + // The controller's force_relay alone does not say whether ICE must be Relay-only; its + // offer envelope does. `ice_policy: "all"` means the relay was forced by the transport + // (ws), so answer with full ICE and let a direct pair form. let webrtc_relay_only = ph.force_relay && !WebRTCStream::endpoint_declares_all_ice(&ph.webrtc_sdp_offer); - // Like the udp/ipv6 legs, the answerer follows the request and does not consult this - // machine's own enable-webrtc option. That option is LocalConfig, which the UI process - // writes and never syncs over IPC — this code runs in the server process, which on - // Windows resolves LocalConfig under a different profile entirely and would read the - // private-server default of "N", silently refusing to answer in exactly the self-hosted - // deployments the transport is for. The option still gates the feature where it can: - // an offer only exists because a controller had it enabled. + // No enable-webrtc check here: it is LocalConfig, which the UI process writes and never + // syncs over IPC, so this (server) process would read the private-server default of "N" + // and refuse to answer in exactly the self-hosted deployments the transport is for. + // A proxy still rules it out — ICE would bypass it and leak the real IP. let webrtc_viable = !ph.webrtc_sdp_offer.is_empty() && !Config::is_proxy() && (!webrtc_relay_only || WebRTCStream::has_turn_server()); @@ -952,16 +937,10 @@ impl RendezvousMediator { return Ok(()); } if !ph.webrtc_sdp_offer.is_empty() { - // WebRTC-only request (udp_port <= 0): return the answer over a short-lived TCP - // connection to the rendezvous server, like create_relay does. It must NOT ride - // the mediator channel: that channel is UDP in the default setup, and the answer - // is the largest message of the punch exchange — a single lost or fragmented - // datagram costs a whole 3s retry round; hbbs also applies UDP-punch semantics - // (source-address observation / is_udp) to PunchHoleSent received over UDP, - // which this request never asked for. - // No TCP punch connection is created or accepted; the controller retains its - // request socket for trickled ICE signaling. IPv6, when present, was started - // above and its address is carried in this same response. + // Return the answer over its own short-lived TCP connection rather than the mediator + // channel: that channel is UDP by default, and hbbs applies UDP-punch semantics + // (source-address observation) to a PunchHoleSent that arrives on it. No TCP punch + // is made — the controller keeps its request socket for trickled ICE. let mut msg_out = Message::new(); msg_out.set_punch_hole_sent(msg_punch); let mut socket = connect_tcp(&*self.host, CONNECT_TIMEOUT).await?;