From 3a7b1fbbd6b4409a45146b6a44420158e9dc0508 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20L=C3=B6nnhager?= Date: Mon, 16 Dec 2024 13:13:01 +0100 Subject: [PATCH 1/4] Force WireGuard handshake before PQ handshake --- talpid-wireguard/build.rs | 8 +++ talpid-wireguard/src/connectivity/mod.rs | 4 +- talpid-wireguard/src/ephemeral.rs | 63 ++++++++++++++++++++++-- talpid-wireguard/src/lib.rs | 2 + 4 files changed, 71 insertions(+), 6 deletions(-) diff --git a/talpid-wireguard/build.rs b/talpid-wireguard/build.rs index ab3500330c26..0a7360569d70 100644 --- a/talpid-wireguard/build.rs +++ b/talpid-wireguard/build.rs @@ -14,6 +14,14 @@ fn main() { // Enable DAITA by default on desktop and android println!("cargo::rustc-check-cfg=cfg(daita)"); println!("cargo::rustc-cfg=daita"); + + // Ensure that the WireGuard tunnel works before exchanging ephemeral peers. + // This is useful after updating the WireGuard config, to force a WireGuard handshake. This + // should reduce the number of PQ timeouts. + println!("cargo::rustc-check-cfg=cfg(force_wireguard_handshake)"); + if matches!(target_os.as_str(), "linux" | "macos" | "windows") { + println!("cargo::rustc-cfg=force_wireguard_handshake"); + } } fn declare_libs_dir(base: &str) { diff --git a/talpid-wireguard/src/connectivity/mod.rs b/talpid-wireguard/src/connectivity/mod.rs index 512d8715f17d..968263b81720 100644 --- a/talpid-wireguard/src/connectivity/mod.rs +++ b/talpid-wireguard/src/connectivity/mod.rs @@ -6,8 +6,6 @@ mod mock; mod monitor; mod pinger; -#[cfg(target_os = "android")] -pub use check::Cancellable; -pub use check::Check; +pub use check::{Cancellable, Check}; pub use error::Error; pub use monitor::Monitor; diff --git a/talpid-wireguard/src/ephemeral.rs b/talpid-wireguard/src/ephemeral.rs index 31f3957253e9..9b146de66f75 100644 --- a/talpid-wireguard/src/ephemeral.rs +++ b/talpid-wireguard/src/ephemeral.rs @@ -1,6 +1,8 @@ //! This module takes care of obtaining ephemeral peers, updating the WireGuard configuration and //! restarting obfuscation and WG tunnels when necessary. +#[cfg(force_wireguard_handshake)] +use super::connectivity; #[cfg(target_os = "android")] // On Android, the Tunnel trait is not imported by default. use super::Tunnel; use super::{config::Config, obfuscation::ObfuscatorHandle, CloseMsg, Error, TunnelType}; @@ -31,6 +33,9 @@ pub async fn config_ephemeral_peers( retry_attempt: u32, obfuscator: Arc>>, close_obfs_sender: sync_mpsc::Sender, + #[cfg(force_wireguard_handshake)] connectivity: &mut connectivity::Check< + connectivity::Cancellable, + >, ) -> std::result::Result<(), CloseMsg> { let iface_name = { let tunnel = tunnel.lock().await; @@ -44,8 +49,16 @@ pub async fn config_ephemeral_peers( log::trace!("Temporarily lowering tunnel MTU before ephemeral peer config"); try_set_ipv4_mtu(&iface_name, talpid_tunnel::MIN_IPV4_MTU); - config_ephemeral_peers_inner(tunnel, config, retry_attempt, obfuscator, close_obfs_sender) - .await?; + config_ephemeral_peers_inner( + tunnel, + config, + retry_attempt, + obfuscator, + close_obfs_sender, + #[cfg(force_wireguard_handshake)] + connectivity, + ) + .await?; log::trace!("Resetting tunnel MTU"); try_set_ipv4_mtu(&iface_name, config.mtu); @@ -75,6 +88,9 @@ pub async fn config_ephemeral_peers( retry_attempt: u32, obfuscator: Arc>>, close_obfs_sender: sync_mpsc::Sender, + #[cfg(force_wireguard_handshake)] connectivity: &mut connectivity::Check< + connectivity::Cancellable, + >, #[cfg(target_os = "android")] tun_provider: Arc>, ) -> Result<(), CloseMsg> { config_ephemeral_peers_inner( @@ -83,6 +99,8 @@ pub async fn config_ephemeral_peers( retry_attempt, obfuscator, close_obfs_sender, + #[cfg(force_wireguard_handshake)] + connectivity, #[cfg(target_os = "android")] tun_provider, ) @@ -95,8 +113,14 @@ async fn config_ephemeral_peers_inner( retry_attempt: u32, obfuscator: Arc>>, close_obfs_sender: sync_mpsc::Sender, + #[cfg(force_wireguard_handshake)] connectivity: &mut connectivity::Check< + connectivity::Cancellable, + >, #[cfg(target_os = "android")] tun_provider: Arc>, ) -> Result<(), CloseMsg> { + #[cfg(force_wireguard_handshake)] + establish_tunnel_connection(tunnel, connectivity).await?; + let ephemeral_private_key = PrivateKey::new_from_random(); let close_obfs_sender = close_obfs_sender.clone(); @@ -134,6 +158,10 @@ async fn config_ephemeral_peers_inner( &tun_provider, ) .await?; + + #[cfg(force_wireguard_handshake)] + establish_tunnel_connection(tunnel, connectivity).await?; + let entry_ephemeral_peer = request_ephemeral_peer( retry_attempt, &entry_config, @@ -214,7 +242,6 @@ async fn reconfigure_tunnel( *obfs_guard = super::obfuscation::apply_obfuscation_config( &mut config, close_obfs_sender, - #[cfg(target_os = "android")] tun_provider.clone(), ) .await @@ -268,6 +295,36 @@ async fn reconfigure_tunnel( Ok(config) } +/// Ensure that the WireGuard tunnel works. This is useful after updating the WireGuard config, to +/// force a WireGuard handshake. This should reduce the number of PQ timeouts. +#[cfg(force_wireguard_handshake)] +async fn establish_tunnel_connection( + tunnel: &Arc>>, + connectivity: &mut connectivity::Check, +) -> Result<(), CloseMsg> { + use talpid_types::ErrorExt; + + let shared_tunnel = tunnel.lock().await; + let tunnel = shared_tunnel.as_ref().expect("tunnel was None"); + let ping_result = connectivity.establish_connectivity(tunnel); + drop(shared_tunnel); + + match ping_result { + Ok(true) => Ok(()), + Ok(false) => { + log::warn!("Timeout while checking tunnel connection"); + Err(CloseMsg::PingErr) + } + Err(error) => { + log::error!( + "{}", + error.display_chain_with_msg("Failed to check tunnel connection") + ); + Err(CloseMsg::PingErr) + } + } +} + async fn request_ephemeral_peer( retry_attempt: u32, config: &Config, diff --git a/talpid-wireguard/src/lib.rs b/talpid-wireguard/src/lib.rs index 2d282c6315c6..cf0a57e41084 100644 --- a/talpid-wireguard/src/lib.rs +++ b/talpid-wireguard/src/lib.rs @@ -274,6 +274,8 @@ impl WireguardMonitor { args.retry_attempt, obfuscator.clone(), ephemeral_obfs_sender, + #[cfg(force_wireguard_handshake)] + &mut connectivity_monitor, ) .await?; From e84c2917760d890465031e48bc5a716dc857cd48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20L=C3=B6nnhager?= Date: Tue, 17 Dec 2024 08:39:39 +0100 Subject: [PATCH 2/4] Reset connection monitor state and timeout between each ping attempt --- talpid-wireguard/src/connectivity/check.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/talpid-wireguard/src/connectivity/check.rs b/talpid-wireguard/src/connectivity/check.rs index 702ce97f2d4d..508e8cff63d0 100644 --- a/talpid-wireguard/src/connectivity/check.rs +++ b/talpid-wireguard/src/connectivity/check.rs @@ -125,6 +125,8 @@ impl Check { // checks if the tunnel has ever worked. Intended to check if a connection to a tunnel is // successful at the start of a connection. pub fn establish_connectivity(&mut self, tunnel_handle: &TunnelType) -> Result { + self.conn_state = ConnState::new(Instant::now(), Default::default()); + // Send initial ping to prod WireGuard into connecting. self.ping_state .pinger From fc266a77aea8b029d867b3bb1346a4175e5c2647 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20L=C3=B6nnhager?= Date: Tue, 17 Dec 2024 09:45:48 +0100 Subject: [PATCH 3/4] Ignore failed ping (since multihop is flaky) --- talpid-wireguard/src/ephemeral.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/talpid-wireguard/src/ephemeral.rs b/talpid-wireguard/src/ephemeral.rs index 9b146de66f75..c8df8835e745 100644 --- a/talpid-wireguard/src/ephemeral.rs +++ b/talpid-wireguard/src/ephemeral.rs @@ -118,8 +118,10 @@ async fn config_ephemeral_peers_inner( >, #[cfg(target_os = "android")] tun_provider: Arc>, ) -> Result<(), CloseMsg> { + // NOTE: This one often fails with multihop on Windows, even though the handshake afterwards + // succeeds. So we try anyway if it fails. #[cfg(force_wireguard_handshake)] - establish_tunnel_connection(tunnel, connectivity).await?; + let _ = establish_tunnel_connection(tunnel, connectivity).await; let ephemeral_private_key = PrivateKey::new_from_random(); let close_obfs_sender = close_obfs_sender.clone(); From 86414a57b9bce42216040fec761251c56570614c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20L=C3=B6nnhager?= Date: Mon, 16 Dec 2024 14:23:56 +0100 Subject: [PATCH 4/4] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index feaf7934598f..f2940ea9a02f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ Line wrap the file at 100 chars. Th ### Changed - (Linux and macOS only) Update to DAITA v2. The main difference is that many different machines are provided by relays instead of a bundled list. The bundled `maybenot_machines` file was removed. +- Test tunnel before ephemeral peer exchange. This is an attempt to fix timeout issues. ### Fixed #### macOS