Merge branch 'fix-bridge-constraint-preference'

4 files changed, 104 insertions, 42 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8f4aac6c69..6dfa1acc81 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -41,6 +41,7 @@ Line wrap the file at 100 chars.                                              Th
 - Update Electron from 15.0.0 to 16.0.4.
 - Gradually increase the WireGuard connectivity check timeout, lowering the timeout for the first
   few attempts.
+- Stop preferring OpenVPN when bridge mode is enabled.
 
 #### Android
 - Avoid running in foreground when not connected.
diff --git a/mullvad-daemon/src/relays.rs b/mullvad-daemon/src/relays.rs
index 803deac988..332baf5a85 100644
--- a/mullvad-daemon/src/relays.rs
+++ b/mullvad-daemon/src/relays.rs
@@ -352,17 +352,13 @@ impl RelaySelector {
         retry_attempt: u32,
         wg_key_exists: bool,
     ) -> RelayConstraints {
-        let (preferred_port, preferred_protocol, preferred_tunnel) =
-            if bridge_state != BridgeState::On {
-                self.preferred_tunnel_constraints(
-                    retry_attempt,
-                    &original_constraints.location,
-                    &original_constraints.providers,
-                    wg_key_exists,
-                )
-            } else {
-                (Constraint::Any, TransportProtocol::Tcp, TunnelType::OpenVpn)
-            };
+        let (preferred_port, preferred_protocol, preferred_tunnel) = self
+            .preferred_tunnel_constraints(
+                retry_attempt,
+                &original_constraints.location,
+                &original_constraints.providers,
+                wg_key_exists,
+            );
 
         let mut relay_constraints = original_constraints.clone();
         relay_constraints.openvpn_constraints = Default::default();
@@ -372,9 +368,14 @@ impl RelaySelector {
         match original_constraints.tunnel_protocol {
             // If no tunnel protocol is selected, use preferred constraints
             Constraint::Any => {
-                if original_constraints.openvpn_constraints.port.is_any()
-                    || bridge_state == BridgeState::On
-                {
+                if bridge_state == BridgeState::On {
+                    relay_constraints.openvpn_constraints = OpenVpnConstraints {
+                        port: Constraint::Only(TransportPort {
+                            protocol: TransportProtocol::Tcp,
+                            port: Constraint::Any,
+                        }),
+                    };
+                } else if original_constraints.openvpn_constraints.port.is_any() {
                     relay_constraints.openvpn_constraints = OpenVpnConstraints {
                         port: Constraint::Only(TransportPort {
                             protocol: preferred_protocol,
@@ -400,7 +401,6 @@ impl RelaySelector {
                 let openvpn_constraints = &mut relay_constraints.openvpn_constraints;
                 *openvpn_constraints = original_constraints.openvpn_constraints;
                 if bridge_state == BridgeState::On && openvpn_constraints.port.is_any() {
-                    // FIXME: This is temporary while talpid-core only supports TCP proxies
                     openvpn_constraints.port = Constraint::Only(TransportPort {
                         protocol: TransportProtocol::Tcp,
                         port: Constraint::Any,
@@ -1452,4 +1452,92 @@ mod test {
 
         Ok(())
     }
+
+    #[test]
+    fn test_bridge_constraints() -> Result<(), String> {
+        let relay_selector = new_relay_selector();
+
+        let location = LocationConstraint::Hostname(
+            "se".to_string(),
+            "got".to_string(),
+            "se-got-001".to_string(),
+        );
+        let mut relay_constraints = RelayConstraints {
+            location: Constraint::Only(location.clone()),
+            tunnel_protocol: Constraint::Any,
+            ..RelayConstraints::default()
+        };
+        relay_constraints.openvpn_constraints.port = Constraint::Only(TransportPort {
+            protocol: TransportProtocol::Udp,
+            port: Constraint::Any,
+        });
+
+        let preferred =
+            relay_selector.preferred_constraints(&relay_constraints, BridgeState::On, 0, true);
+        assert_eq!(
+            preferred.tunnel_protocol,
+            Constraint::Only(TunnelType::OpenVpn)
+        );
+        // NOTE: TCP is preferred for bridges
+        assert_eq!(
+            preferred.openvpn_constraints.port,
+            Constraint::Only(TransportPort {
+                protocol: TransportProtocol::Tcp,
+                port: Constraint::Any,
+            })
+        );
+
+        // Ignore bridge state where WireGuard is used
+        let location = LocationConstraint::Hostname(
+            "se".to_string(),
+            "got".to_string(),
+            "se10-wireguard".to_string(),
+        );
+        let relay_constraints = RelayConstraints {
+            location: Constraint::Only(location),
+            tunnel_protocol: Constraint::Any,
+            ..RelayConstraints::default()
+        };
+        let preferred =
+            relay_selector.preferred_constraints(&relay_constraints, BridgeState::On, 0, true);
+        assert_eq!(
+            preferred.tunnel_protocol,
+            Constraint::Only(TunnelType::Wireguard)
+        );
+
+        // Handle bridge setting when falling back on OpenVPN
+        let mut relay_constraints = RelayConstraints {
+            location: Constraint::Any,
+            tunnel_protocol: Constraint::Any,
+            ..RelayConstraints::default()
+        };
+        relay_constraints.openvpn_constraints.port = Constraint::Only(TransportPort {
+            protocol: TransportProtocol::Udp,
+            port: Constraint::Any,
+        });
+        #[cfg(all(unix, not(target_os = "android")))]
+        {
+            let preferred =
+                relay_selector.preferred_constraints(&relay_constraints, BridgeState::On, 0, true);
+            assert_eq!(
+                preferred.tunnel_protocol,
+                Constraint::Only(TunnelType::Wireguard)
+            );
+        }
+        let preferred =
+            relay_selector.preferred_constraints(&relay_constraints, BridgeState::On, 2, true);
+        assert_eq!(
+            preferred.tunnel_protocol,
+            Constraint::Only(TunnelType::OpenVpn)
+        );
+        assert_eq!(
+            preferred.openvpn_constraints.port,
+            Constraint::Only(TransportPort {
+                protocol: TransportProtocol::Tcp,
+                port: Constraint::Any,
+            })
+        );
+
+        Ok(())
+    }
 }
diff --git a/mullvad-types/src/relay_constraints.rs b/mullvad-types/src/relay_constraints.rs
index 7789948797..18cdfda2d9 100644
--- a/mullvad-types/src/relay_constraints.rs
+++ b/mullvad-types/src/relay_constraints.rs
@@ -172,30 +172,6 @@ impl RelaySettings {
             }),
         }
     }
-
-    pub(crate) fn ensure_bridge_compatibility(&mut self) {
-        match self {
-            RelaySettings::Normal(ref mut constraints) => {
-                if constraints.tunnel_protocol == Constraint::Only(TunnelType::Wireguard) {
-                    constraints.tunnel_protocol = Constraint::Any;
-                }
-                if let Constraint::Only(TransportPort {
-                    protocol: TransportProtocol::Udp,
-                    ..
-                }) = constraints.openvpn_constraints.port
-                {
-                    constraints.openvpn_constraints.port = Constraint::Any;
-                }
-            }
-            RelaySettings::CustomTunnelEndpoint(config) => {
-                if config.endpoint().protocol == TransportProtocol::Udp {
-                    log::warn!(
-                        "Using custom tunnel endpoint with UDP, bridges will likely not work"
-                    );
-                }
-            }
-        }
-    }
 }
 
 /// Limits the set of [`crate::relay_list::Relay`]s that a `RelaySelector` may select.
diff --git a/mullvad-types/src/settings/mod.rs b/mullvad-types/src/settings/mod.rs
index 0705764555..36981b82c7 100644
--- a/mullvad-types/src/settings/mod.rs
+++ b/mullvad-types/src/settings/mod.rs
@@ -194,9 +194,6 @@ impl Settings {
     pub fn set_bridge_state(&mut self, bridge_state: BridgeState) -> bool {
         if self.bridge_state != bridge_state {
             self.bridge_state = bridge_state;
-            if self.bridge_state == BridgeState::On {
-                self.relay_settings.ensure_bridge_compatibility();
-            }
             true
         } else {
             false