Merge branch 'pq-workaround-invalid-mtu'

3 files changed, 26 insertions, 71 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5c4398ca55..e4e1228f4b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -33,6 +33,9 @@ Line wrap the file at 100 chars.                                              Th
 - Add support for all screen orientations.
 
 ### Fixed
+- Fix connectivity issues that would occur when using quantum-resistant tunnels with an incorrectly
+  configured MTU.
+
 #### Linux
 - Fix Bash shell completions for subcommands in the CLI
 - Out IP missing forever when am.i.mullvad.net returns error
diff --git a/talpid-tunnel-config-client/src/lib.rs b/talpid-tunnel-config-client/src/lib.rs
index 14babc848f..2c7b5e58f3 100644
--- a/talpid-tunnel-config-client/src/lib.rs
+++ b/talpid-tunnel-config-client/src/lib.rs
@@ -82,6 +82,17 @@ type RelayConfigService = proto::post_quantum_secure_client::PostQuantumSecureCl
 /// Port used by the tunnel config service.
 pub const CONFIG_SERVICE_PORT: u16 = 1337;
 
+/// MTU to set on the tunnel config client socket. We want a low value to prevent fragmentation.
+/// This is needed for two reasons:
+/// 1. Especially on Android, we've found that the real MTU is often lower than the default MTU, and
+///    we cannot lower it further. This causes the outer packets to be dropped. Also, MTU detection
+///    will likely occur after the PQ handshake, so we cannot assume that the MTU is already
+///    correctly configured.
+/// 2. MH + PQ on macOS has connection issues during the handshake due to PF blocking packet
+///    fragments for not having a port. In the longer term this might be fixed by allowing the
+///    handshake to work even if there is fragmentation.
+const CONFIG_CLIENT_MTU: u16 = 576;
+
 /// Generates a new WireGuard key pair and negotiates a PSK with the relay in a PQ-safe
 /// manner. This creates a peer on the relay with the new WireGuard pubkey and PSK,
 /// which can then be used to establish a PQ-safe tunnel to the relay.
@@ -91,19 +102,10 @@ pub async fn push_pq_key(
     wg_pubkey: PublicKey,
     wg_psk_pubkey: PublicKey,
 ) -> Result<PresharedKey, Error> {
-    push_pq_key_with_opts(service_address, wg_pubkey, wg_psk_pubkey, None).await
-}
-
-pub async fn push_pq_key_with_opts(
-    service_address: IpAddr,
-    wg_pubkey: PublicKey,
-    wg_psk_pubkey: PublicKey,
-    mtu: Option<u16>,
-) -> Result<PresharedKey, Error> {
     let (cme_kem_pubkey, cme_kem_secret) = classic_mceliece::generate_keys().await;
     let kyber_keypair = kyber::keypair(&mut rand::thread_rng());
 
-    let mut client = new_client(service_address, mtu).await?;
+    let mut client = new_client(service_address).await?;
     let response = client
         .psk_exchange_v1(proto::PskRequestV1 {
             wg_pubkey: wg_pubkey.as_bytes().to_vec(),
@@ -166,20 +168,18 @@ fn xor_assign(dst: &mut [u8; 32], src: &[u8; 32]) {
     }
 }
 
-async fn new_client(addr: IpAddr, mtu: Option<u16>) -> Result<RelayConfigService, Error> {
+async fn new_client(addr: IpAddr) -> Result<RelayConfigService, Error> {
     let endpoint = Endpoint::from_static("tcp://0.0.0.0:0");
 
     let conn = endpoint
         .connect_with_connector(service_fn(move |_| async move {
             let sock = TcpSocket::new_v4()?;
 
-            if let Some(mtu) = mtu {
-                #[cfg(target_os = "windows")]
-                try_set_tcp_sock_mtu(sock.as_raw_socket(), mtu);
+            #[cfg(target_os = "windows")]
+            try_set_tcp_sock_mtu(sock.as_raw_socket(), CONFIG_CLIENT_MTU);
 
-                #[cfg(not(target_os = "windows"))]
-                try_set_tcp_sock_mtu(&addr, sock.as_raw_fd(), mtu);
-            }
+            #[cfg(not(target_os = "windows"))]
+            try_set_tcp_sock_mtu(&addr, sock.as_raw_fd(), CONFIG_CLIENT_MTU);
 
             sock.connect(SocketAddr::new(addr, CONFIG_SERVICE_PORT))
                 .await
diff --git a/talpid-wireguard/src/lib.rs b/talpid-wireguard/src/lib.rs
index 63f24ccb4f..a42a01ee54 100644
--- a/talpid-wireguard/src/lib.rs
+++ b/talpid-wireguard/src/lib.rs
@@ -8,12 +8,13 @@ use futures::future::{abortable, AbortHandle as FutureAbortHandle, BoxFuture, Fu
 use futures::{channel::mpsc, StreamExt};
 #[cfg(target_os = "linux")]
 use once_cell::sync::Lazy;
+#[cfg(target_os = "android")]
+use std::borrow::Cow;
 #[cfg(target_os = "linux")]
 use std::env;
 #[cfg(windows)]
 use std::io;
 use std::{
-    borrow::Cow,
     convert::Infallible,
     net::IpAddr,
     path::Path,
@@ -237,29 +238,11 @@ impl WireguardMonitor {
             close_obfs_sender.clone(),
         ))?;
 
-        // TODO: Currently MH + PQ on MacOS has connection issues during the handshake. This seems
-        // be be due to packet fragmentation happening and PF blocking fragmented packets during
-        // the handshake due to them sometimes not having a port. Lowering the MTU for the initial
-        // tunnel which connects to the exit during PSK + MH negotiation causes less fragmentation
-        // and should be a hacky fix for the problem. In the longer term this should be fixed by
-        // allowing the handshake to work even if there is fragmentation and/or setting the MTU
-        // properly so fragmentation does not happen.
-        let init_tunnel_config = if cfg!(target_os = "macos") {
-            let mut init_tunnel_config = config.clone();
-            if psk_negotiation && config.is_multihop() {
-                const MH_PQ_HANDSHAKE_MTU: u16 = 1280;
-                init_tunnel_config.mtu = MH_PQ_HANDSHAKE_MTU;
-            }
-            Cow::Owned(init_tunnel_config)
-        } else {
-            Cow::Borrowed(&config)
-        };
-
         #[cfg(target_os = "windows")]
         let (setup_done_tx, setup_done_rx) = mpsc::channel(0);
         let tunnel = Self::open_tunnel(
             args.runtime.clone(),
-            &init_tunnel_config,
+            &config,
             log_path,
             args.resource_dir,
             args.tun_provider.clone(),
@@ -464,9 +447,8 @@ impl WireguardMonitor {
         let metadata = Self::tunnel_metadata(iface_name, config);
         (on_event)(TunnelEvent::InterfaceUp(metadata, allowed_traffic.clone())).await;
 
-        let mtu = Self::get_exit_psk_socket_mtu(config);
         let exit_psk =
-            Self::perform_psk_negotiation(retry_attempt, config, wg_psk_privkey.public_key(), mtu)
+            Self::perform_psk_negotiation(retry_attempt, config, wg_psk_privkey.public_key())
                 .await?;
 
         log::debug!("Successfully exchanged PSK with exit peer");
@@ -494,7 +476,6 @@ impl WireguardMonitor {
                     retry_attempt,
                     &entry_config,
                     wg_psk_privkey.public_key(),
-                    None,
                 )
                 .await?,
             );
@@ -639,7 +620,6 @@ impl WireguardMonitor {
         retry_attempt: u32,
         config: &Config,
         wg_psk_pubkey: PublicKey,
-        mss: Option<u16>,
     ) -> std::result::Result<PresharedKey, CloseMsg> {
         log::debug!("Performing PQ-safe PSK exchange");
 
@@ -651,11 +631,10 @@ impl WireguardMonitor {
 
         let psk = tokio::time::timeout(
             timeout,
-            talpid_tunnel_config_client::push_pq_key_with_opts(
-                Self::get_tunnel_config_client_addr(config),
+            talpid_tunnel_config_client::push_pq_key(
+                IpAddr::from(config.ipv4_gateway),
                 config.tunnel.private_key.public_key(),
                 wg_psk_pubkey,
-                mss,
             ),
         )
         .await
@@ -669,33 +648,6 @@ impl WireguardMonitor {
         Ok(psk)
     }
 
-    /// TODO: This hack makes sure that the max segment size is small enough to prevent
-    /// fragmentation. This is an issue when using multihop, because the MTU is too small to
-    /// accommodate the headers of the inner tunnel, and lowering it does no good.
-    //
-    /// When we have fixed fragmentation issues for multihop, this workaround can be removed.
-    ///
-    /// Note that TCP imposes max and min bounds on the specified size as well.
-    fn get_exit_psk_socket_mtu(config: &Config) -> Option<u16> {
-        // macOS is averse(?) to large values for MSS, so we just use the minimum
-        const MIN_IPV4_MTU: u16 = 576;
-        const MIN_IPV6_MTU: u16 = 1280;
-
-        if !config.is_multihop() {
-            return None;
-        }
-
-        if Self::get_tunnel_config_client_addr(config).is_ipv4() {
-            Some(MIN_IPV4_MTU)
-        } else {
-            Some(MIN_IPV6_MTU)
-        }
-    }
-
-    fn get_tunnel_config_client_addr(config: &Config) -> IpAddr {
-        IpAddr::V4(config.ipv4_gateway)
-    }
-
     #[allow(unused_variables)]
     fn open_tunnel(
         runtime: tokio::runtime::Handle,