Fix bug selecting OpenVPN bridge if transport protocol was set to Auto

Add a test case to cover this case. Thanks Sebastian for reporting the
bug!

Author: MarkusPettersson98

mullvad-relay-selector/src/relay_selector/detailer.rs

@@ -14,10 +14,12 @@ use mullvad_types::{
     relay_constraints::TransportPort,
     relay_list::{OpenVpnEndpoint, OpenVpnEndpointData, Relay, WireguardEndpointData},
 };
-use talpid_types::net::{all_of_the_internet, wireguard::PeerConfig, Endpoint, IpVersion};
+use talpid_types::net::{
+    all_of_the_internet, wireguard::PeerConfig, Endpoint, IpVersion, TransportProtocol,
+};
 
 use super::{
-    query::{OpenVpnRelayQuery, WireguardRelayQuery},
+    query::{OpenVpnRelayQuery, WireguardRelayQuery, BridgeQuery},
     WireguardConfig,
 };
 
@@ -62,14 +64,13 @@ impl WireguardDetailer {
     /// [`WireguradRelayQuery::port`]) or relay addresses cannot be resolved.
     pub fn to_endpoint(&self) -> Option<MullvadEndpoint> {
         match &self.config {
-            WireguardConfig::Singlehop { exit } => self.tmp_singlehop(exit),
-            WireguardConfig::Multihop { exit, entry } => self.tmp_multihop(exit, entry),
+            WireguardConfig::Singlehop { exit } => self.to_singlehop_endpoint(exit),
+            WireguardConfig::Multihop { exit, entry } => self.to_multihop_endpoint(exit, entry),
         }
     }
 
     /// Configure a single-hop connection using the exit relay data.
-    /// TODO(markus): Rename
-    fn tmp_singlehop(&self, exit: &Relay) -> Option<MullvadEndpoint> {
+    fn to_singlehop_endpoint(&self, exit: &Relay) -> Option<MullvadEndpoint> {
         let endpoint = {
             let host = self.get_address_for_wireguard_relay(exit)?;
             let port = self.get_port_for_wireguard_relay(&self.data)?;
@@ -95,8 +96,7 @@ impl WireguardDetailer {
     /// # Note
     /// In a multihop circuit, we need to provide an exit peer configuration in addition to the
     /// peer configuration.
-    /// TODO(markus): Rename
-    fn tmp_multihop(&self, exit: &Relay, entry: &Relay) -> Option<MullvadEndpoint> {
+    fn to_multihop_endpoint(&self, exit: &Relay, entry: &Relay) -> Option<MullvadEndpoint> {
         let exit_endpoint = {
             let ip = exit.ipv4_addr_in;
             // The port that the exit relay listens for incoming connections from entry
@@ -228,35 +228,64 @@ impl OpenVpnDetailer {
         }
     }
 
+    /// TODO(markus): Clean up.
     /// Map `self` to a [`MullvadEndpoint`].
     ///
+    /// If this endpoint is to be used in conjunction with a bridge, the resulting endpoint is
+    /// guaranteed to use transport protocol `TCP`.
+    ///
     /// This function can fail if no valid port + transport protocol combination is found.
     /// See [`OpenVpnEndpointData`] for more details.
     pub fn to_endpoint(&self) -> Option<MullvadEndpoint> {
-        self.get_random_transport_port().map(|endpoint| {
-            MullvadEndpoint::OpenVpn(Endpoint::new(
-                self.exit.ipv4_addr_in,
-                endpoint.port,
-                endpoint.protocol,
-            ))
-        })
+        // If `bridge_mode` is true, this function may only return endpoints which use TCP, not UDP.
+        if BridgeQuery::should_use_bridge(&self.openvpn_constraints.bridge_settings) {
+            self.to_bridged_endpoint()
+        } else {
+            self.to_single_endpoint()
+        }
+    }
+
+    /// TODO(markus): Document
+    fn to_single_endpoint(&self) -> Option<MullvadEndpoint> {
+        use rand::seq::IteratorRandom;
+        let constraints_port = self.openvpn_constraints.port;
+        self.data
+            .ports
+            .iter()
+            .filter(|&endpoint| Self::compatible_port_combo(&constraints_port, endpoint))
+            .choose(&mut rand::thread_rng())
+            .map(|endpoint| {
+                MullvadEndpoint::OpenVpn(Endpoint::new(
+                    self.exit.ipv4_addr_in,
+                    endpoint.port,
+                    endpoint.protocol,
+                ))
+            })
     }
 
-    /// Try to pick a valid OpenVPN port.
-    fn get_random_transport_port(&self) -> Option<&OpenVpnEndpoint> {
+    /// TODO(markus): Document
+    fn to_bridged_endpoint(&self) -> Option<MullvadEndpoint> {
         use rand::seq::IteratorRandom;
         let constraints_port = self.openvpn_constraints.port;
         self.data
             .ports
             .iter()
-            .filter(|endpoint| Self::compatible_port_combo(constraints_port, endpoint))
+            .filter(|endpoint| matches!(endpoint.protocol, TransportProtocol::Tcp))
+            .filter(|endpoint| Self::compatible_port_combo(&constraints_port, endpoint))
             .choose(&mut rand::thread_rng())
+            .map(|endpoint| {
+                MullvadEndpoint::OpenVpn(Endpoint::new(
+                    self.exit.ipv4_addr_in,
+                    endpoint.port,
+                    endpoint.protocol,
+                ))
+            })
     }
 
     /// Returns true if `port_constraint` can be used to connect to `endpoint`.
     /// Otherwise, false is returned.
     fn compatible_port_combo(
-        port_constraint: Constraint<TransportPort>,
+        port_constraint: &Constraint<TransportPort>,
         endpoint: &OpenVpnEndpoint,
     ) -> bool {
         match port_constraint {

mullvad-relay-selector/src/relay_selector/mod.rs

@@ -619,7 +619,7 @@ impl RelaySelector {
     /// - `custom_lists`: User-defined or application-specific settings that may influence bridge selection.
     ///
     /// # Returns
-    /// * On success, returns an `Option` containing the selected bridge, if one is found. Returns `None` if no suitable bridge meets the criteria.
+    /// * On success, returns an `Option` containing the selected bridge, if one is found. Returns `None` if no suitable bridge meets the criteria or bridges should not be used.
     /// * `Error::NoBridge` if attempting to use OpenVPN bridges over UDP, as this is unsupported.
     /// * `Error::NoRelay` if `relay` does not have a location set.
     fn get_openvpn_bridge(
@@ -804,11 +804,7 @@ impl RelaySelector {
         parsed_relays: &ParsedRelays,
     ) -> Option<Relay> {
         // Filter among all valid relays
-        let relays = Self::get_tunnel_endpoints(
-            query,
-            config,
-            parsed_relays,
-        );
+        let relays = Self::get_tunnel_endpoints(query, config, parsed_relays);
         // Pick one of the valid relays.
         helpers::pick_random_relay(&relays).cloned()
     }

mullvad-relay-selector/tests/relay_selector.rs

@@ -902,3 +902,48 @@ fn openvpn_handle_bridge_settings() {
         ),
     };
 }
+
+/// Verify that the relay selector correctly gives back an OpenVPN relay + bridge when the user's
+/// settings indicate that bridge mode is on, but the transport protocol is set to auto. Note that
+/// it is only valid to use TCP with bridges. Trying to use UDP over bridges is not allowed, and
+/// the relay selector should fail to select a relay in these cases.
+#[test]
+fn openvpn_bridge_with_automatic_transport_protocol() {
+    // Enable bridge mode.
+    let config = SelectorConfig {
+        bridge_state: BridgeState::On,
+        ..SelectorConfig::default()
+    };
+    let relay_selector = RelaySelector::from_list(config, RELAYS.clone());
+
+    // First, construct a query to choose an OpenVPN relay and bridge.
+    let mut query = RelayQueryBuilder::new().openvpn().bridge().build();
+    // Forcefully modify the transport protocol, as the builder will ensure that the transport
+    // protocol is set to TCP.
+    query.openvpn_constraints.port = Constraint::Any;
+
+    for _ in 0..100 {
+        let relay = relay_selector.get_relay_by_query(query.clone()).unwrap();
+        // Assert that the relay selector is able to cope with the transport protocol being set to
+        // auto.
+        match relay {
+            GetRelay::OpenVpn { endpoint, .. } => {
+                assert_eq!(endpoint.unwrap_openvpn().protocol, TCP);
+            }
+            wrong_relay => panic!(
+                "Relay selector should have picked an OpenVPN relay, instead chose {wrong_relay:?}"
+            ),
+        }
+    }
+
+    // Modify the query slightly to forcefully use UDP. This should not be allowed!
+    let query = RelayQueryBuilder::new()
+        .openvpn()
+        .bridge()
+        .transport_protocol(UDP)
+        .build();
+    for _ in 0..100 {
+        let relay = relay_selector.get_relay_by_query(query.clone());
+        assert!(relay.is_err())
+    }
+}