From 247b25ea3268ebce72209b4fdecab391fe91b4d1 Mon Sep 17 00:00:00 2001 From: Sebastian Holmin Date: Wed, 17 Apr 2024 14:21:16 +0200 Subject: [PATCH 1/4] Remove unused fn `would_return` --- .../src/relay_selector/mod.rs | 24 ------------------- 1 file changed, 24 deletions(-) diff --git a/mullvad-relay-selector/src/relay_selector/mod.rs b/mullvad-relay-selector/src/relay_selector/mod.rs index 3f92bd344bf8..758e580b0e76 100644 --- a/mullvad-relay-selector/src/relay_selector/mod.rs +++ b/mullvad-relay-selector/src/relay_selector/mod.rs @@ -527,30 +527,6 @@ impl RelaySelector { self.get_relay_with_custom_params(retry_attempt, &RETRY_ORDER, runtime_params) } - /// Peek at which [`TunnelType`] that would be returned for a certain connection attempt for a - /// given [`SelectorConfig`]. Returns [`Option::None`] if the given config would return a - /// custom tunnel endpoint. - /// - /// # Note - /// This function is only really useful for testing-purposes. It is exposed to ease testing of - /// other mullvad crates which depend on the retry behaviour of [`RelaySelector`]. - pub fn would_return(connection_attempt: usize, config: &SelectorConfig) -> Option { - match SpecializedSelectorConfig::from(config) { - // This case is not really interesting - SpecializedSelectorConfig::Custom(_) => None, - SpecializedSelectorConfig::Normal(config) => Some( - Self::pick_and_merge_query( - connection_attempt, - &RETRY_ORDER, - RuntimeParameters::default(), - RelayQuery::from(config), - ) - .tunnel_protocol - .unwrap_or(TunnelType::Wireguard), - ), - } - } - /// Returns a random relay and relay endpoint matching the current constraints defined by /// `retry_order` corresponding to `retry_attempt`. pub fn get_relay_with_custom_params( From c531b5708d4de9af9416e9f5fd2f0367f185e278 Mon Sep 17 00:00:00 2001 From: Sebastian Holmin Date: Wed, 17 Apr 2024 14:22:14 +0200 Subject: [PATCH 2/4] Filter queries with no matching relays --- .../src/relay_selector/mod.rs | 51 +++++++++++-------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/mullvad-relay-selector/src/relay_selector/mod.rs b/mullvad-relay-selector/src/relay_selector/mod.rs index 758e580b0e76..35c0e8a3e740 100644 --- a/mullvad-relay-selector/src/relay_selector/mod.rs +++ b/mullvad-relay-selector/src/relay_selector/mod.rs @@ -510,7 +510,7 @@ impl RelaySelector { } SpecializedSelectorConfig::Normal(pure_config) => { let parsed_relays = &self.parsed_relays.lock().unwrap(); - Self::get_relay_inner(query, parsed_relays, &pure_config) + Self::get_relay_inner(&query, parsed_relays, &pure_config) } } } @@ -547,14 +547,14 @@ impl RelaySelector { SpecializedSelectorConfig::Normal(normal_config) => { let parsed_relays = &self.parsed_relays.lock().unwrap(); // Merge user preferences with the relay selector's default preferences. - let user_preferences = RelayQuery::from(normal_config.clone()); let query = Self::pick_and_merge_query( retry_attempt, retry_order, runtime_params, - user_preferences, - ); - Self::get_relay_inner(query, parsed_relays, &normal_config) + &normal_config, + parsed_relays, + )?; + Self::get_relay_inner(&query, parsed_relays, &normal_config) } } } @@ -569,22 +569,29 @@ impl RelaySelector { /// Runtime parameters may affect which of the default queries that are considered. For example, /// queries which rely on IPv6 will not be considered if working IPv6 is not available at /// runtime. + /// + /// Returns an error iff the intersection between the user's preferences and every default retry + /// attempt-query yields queries with no matching relays. I.e., no retry attempt could ever + /// resolve to a relay. fn pick_and_merge_query( retry_attempt: usize, retry_order: &[RelayQuery], runtime_params: RuntimeParameters, - user_preferences: RelayQuery, - ) -> RelayQuery { - log::trace!("Merging user preferences {user_preferences:?} with default retry strategy"); + user_config: &NormalSelectorConfig<'_>, + parsed_relays: &ParsedRelays, + ) -> Result { + let user_query = RelayQuery::from(user_config.clone()); + log::trace!("Merging user preferences {user_query:?} with default retry strategy"); retry_order .iter() // Remove candidate queries based on runtime parameters before trying to merge user // settings .filter(|query| runtime_params.compatible(query)) - .cycle() - .filter_map(|query| query.clone().intersection(user_preferences.clone())) + .filter_map(|query| query.clone().intersection(user_query.clone())) + .filter(|query| Self::get_relay_inner(query, parsed_relays, user_config).is_ok()) + .cycle() // If the above filters remove all relays, cycle will also return an empty iterator .nth(retry_attempt) - .unwrap_or(user_preferences) + .ok_or(Error::NoRelay) } /// "Execute" the given query, yielding a final set of relays and/or bridges which the VPN @@ -605,7 +612,7 @@ impl RelaySelector { /// * An `Err` if no suitable bridge is found #[cfg(not(target_os = "android"))] fn get_relay_inner( - query: RelayQuery, + query: &RelayQuery, parsed_relays: &ParsedRelays, config: &NormalSelectorConfig<'_>, ) -> Result { @@ -622,7 +629,7 @@ impl RelaySelector { let mut new_query = query.clone(); new_query.tunnel_protocol = Constraint::Only(tunnel_type); // If a suitable relay is found, short-circuit and return it - if let Ok(relay) = Self::get_relay_inner(new_query, parsed_relays, config) { + if let Ok(relay) = Self::get_relay_inner(&new_query, parsed_relays, config) { return Ok(relay); } } @@ -659,7 +666,7 @@ impl RelaySelector { /// /// [`MullvadEndpoint`]: mullvad_types::endpoint::MullvadEndpoint fn get_wireguard_relay( - query: RelayQuery, + query: &RelayQuery, config: &NormalSelectorConfig<'_>, parsed_relays: &ParsedRelays, ) -> Result { @@ -668,13 +675,13 @@ impl RelaySelector { Constraint::Only(TunnelType::Wireguard) ); let inner = if !query.wireguard_constraints.multihop() { - Self::get_wireguard_singlehop_config(&query, config, parsed_relays)? + Self::get_wireguard_singlehop_config(query, config, parsed_relays)? } else { - Self::get_wireguard_multihop_config(&query, config, parsed_relays)? + Self::get_wireguard_multihop_config(query, config, parsed_relays)? }; - let endpoint = Self::get_wireguard_endpoint(&query, parsed_relays, &inner)?; + let endpoint = Self::get_wireguard_endpoint(query, parsed_relays, &inner)?; let obfuscator = - Self::get_wireguard_obfuscator(&query, inner.clone(), &endpoint, parsed_relays)?; + Self::get_wireguard_obfuscator(query, inner.clone(), &endpoint, parsed_relays)?; Ok(GetRelay::Wireguard { endpoint, @@ -822,16 +829,16 @@ impl RelaySelector { /// [`MullvadEndpoint`]: mullvad_types::endpoint::MullvadEndpoint #[cfg(not(target_os = "android"))] fn get_openvpn_relay( - query: RelayQuery, + query: &RelayQuery, config: &NormalSelectorConfig<'_>, parsed_relays: &ParsedRelays, ) -> Result { assert_eq!(query.tunnel_protocol, Constraint::Only(TunnelType::OpenVpn)); let exit = - Self::choose_openvpn_relay(&query, config, parsed_relays).ok_or(Error::NoRelay)?; - let endpoint = Self::get_openvpn_endpoint(&query, &exit, parsed_relays)?; + Self::choose_openvpn_relay(query, config, parsed_relays).ok_or(Error::NoRelay)?; + let endpoint = Self::get_openvpn_endpoint(query, &exit, parsed_relays)?; let bridge = - Self::get_openvpn_bridge(&query, &exit, &endpoint.protocol, parsed_relays, config)?; + Self::get_openvpn_bridge(query, &exit, &endpoint.protocol, parsed_relays, config)?; // FIXME: This assert would be better to encode at the type level. assert!(matches!(exit.endpoint_data, RelayEndpointData::Openvpn)); From 1e0a5831e7d206e4c4b05cf7a0c2d1c69757563b Mon Sep 17 00:00:00 2001 From: Sebastian Holmin Date: Wed, 17 Apr 2024 14:33:34 +0200 Subject: [PATCH 3/4] Replace refs to config with just custom list The entire config was passed around just for the custom lists. --- .../src/relay_selector/mod.rs | 79 ++++++++++--------- 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/mullvad-relay-selector/src/relay_selector/mod.rs b/mullvad-relay-selector/src/relay_selector/mod.rs index 35c0e8a3e740..843eea9a204a 100644 --- a/mullvad-relay-selector/src/relay_selector/mod.rs +++ b/mullvad-relay-selector/src/relay_selector/mod.rs @@ -472,7 +472,7 @@ impl RelaySelector { let near_location = match specialized_config { SpecializedSelectorConfig::Normal(config) => { let user_preferences = RelayQuery::from(config.clone()); - Self::get_relay_midpoint(&user_preferences, parsed_relays, &config) + Self::get_relay_midpoint(&user_preferences, parsed_relays, config.custom_lists) } SpecializedSelectorConfig::Custom(_) => None, }; @@ -508,9 +508,9 @@ impl RelaySelector { SpecializedSelectorConfig::Custom(custom_config) => { Ok(GetRelay::Custom(custom_config.clone())) } - SpecializedSelectorConfig::Normal(pure_config) => { + SpecializedSelectorConfig::Normal(normal_config) => { let parsed_relays = &self.parsed_relays.lock().unwrap(); - Self::get_relay_inner(&query, parsed_relays, &pure_config) + Self::get_relay_inner(&query, parsed_relays, normal_config.custom_lists) } } } @@ -554,7 +554,7 @@ impl RelaySelector { &normal_config, parsed_relays, )?; - Self::get_relay_inner(&query, parsed_relays, &normal_config) + Self::get_relay_inner(&query, parsed_relays, normal_config.custom_lists) } } } @@ -588,7 +588,7 @@ impl RelaySelector { // settings .filter(|query| runtime_params.compatible(query)) .filter_map(|query| query.clone().intersection(user_query.clone())) - .filter(|query| Self::get_relay_inner(query, parsed_relays, user_config).is_ok()) + .filter(|query| Self::get_relay_inner(query, parsed_relays, user_config.custom_lists).is_ok()) .cycle() // If the above filters remove all relays, cycle will also return an empty iterator .nth(retry_attempt) .ok_or(Error::NoRelay) @@ -614,14 +614,14 @@ impl RelaySelector { fn get_relay_inner( query: &RelayQuery, parsed_relays: &ParsedRelays, - config: &NormalSelectorConfig<'_>, + custom_lists: &CustomListsSettings, ) -> Result { match query.tunnel_protocol { Constraint::Only(TunnelType::Wireguard) => { - Self::get_wireguard_relay(query, config, parsed_relays) + Self::get_wireguard_relay(query, custom_lists, parsed_relays) } Constraint::Only(TunnelType::OpenVpn) => { - Self::get_openvpn_relay(query, config, parsed_relays) + Self::get_openvpn_relay(query, custom_lists, parsed_relays) } Constraint::Any => { // Try Wireguard, then OpenVPN, then fail @@ -629,7 +629,9 @@ impl RelaySelector { let mut new_query = query.clone(); new_query.tunnel_protocol = Constraint::Only(tunnel_type); // If a suitable relay is found, short-circuit and return it - if let Ok(relay) = Self::get_relay_inner(&new_query, parsed_relays, config) { + if let Ok(relay) = + Self::get_relay_inner(&new_query, parsed_relays, custom_lists) + { return Ok(relay); } } @@ -640,16 +642,17 @@ impl RelaySelector { #[cfg(target_os = "android")] fn get_relay_inner( - mut query: RelayQuery, + query: &RelayQuery, parsed_relays: &ParsedRelays, - config: &NormalSelectorConfig<'_>, + custom_lists: &CustomListsSettings, ) -> Result { // FIXME: A bit of defensive programming - calling `get_wiregurad_relay` with a query that // doesn't specify Wireguard as the desired tunnel type is not valid and will lead // to unwanted behavior. This should be seen as a workaround, and it would be nicer // to lift this invariant to be checked by the type system instead. + let mut query = query.clone(); query.tunnel_protocol = Constraint::Only(TunnelType::Wireguard); - Self::get_wireguard_relay(query, config, parsed_relays) + Self::get_wireguard_relay(&query, custom_lists, parsed_relays) } /// Derive a valid Wireguard relay configuration from `query`. @@ -667,7 +670,7 @@ impl RelaySelector { /// [`MullvadEndpoint`]: mullvad_types::endpoint::MullvadEndpoint fn get_wireguard_relay( query: &RelayQuery, - config: &NormalSelectorConfig<'_>, + custom_lists: &CustomListsSettings, parsed_relays: &ParsedRelays, ) -> Result { assert_eq!( @@ -675,9 +678,9 @@ impl RelaySelector { Constraint::Only(TunnelType::Wireguard) ); let inner = if !query.wireguard_constraints.multihop() { - Self::get_wireguard_singlehop_config(query, config, parsed_relays)? + Self::get_wireguard_singlehop_config(query, custom_lists, parsed_relays)? } else { - Self::get_wireguard_multihop_config(query, config, parsed_relays)? + Self::get_wireguard_multihop_config(query, custom_lists, parsed_relays)? }; let endpoint = Self::get_wireguard_endpoint(query, parsed_relays, &inner)?; let obfuscator = @@ -697,11 +700,10 @@ impl RelaySelector { /// * `Ok(WireguardInner::Singlehop)` otherwise fn get_wireguard_singlehop_config( query: &RelayQuery, - config: &NormalSelectorConfig<'_>, + custom_lists: &CustomListsSettings, parsed_relays: &ParsedRelays, ) -> Result { - let candidates = - filter_matching_relay_list(query, parsed_relays.relays(), config.custom_lists); + let candidates = filter_matching_relay_list(query, parsed_relays.relays(), custom_lists); helpers::pick_random_relay(&candidates) .cloned() .map(WireguardConfig::singlehop) @@ -717,7 +719,7 @@ impl RelaySelector { /// * `Ok(WireguardInner::Multihop)` otherwise fn get_wireguard_multihop_config( query: &RelayQuery, - config: &NormalSelectorConfig<'_>, + custom_lists: &CustomListsSettings, parsed_relays: &ParsedRelays, ) -> Result { // Here, we modify the original query just a bit. @@ -732,16 +734,10 @@ impl RelaySelector { let mut exit_relay_query = query.clone(); // DAITA should only be enabled for the entry relay exit_relay_query.wireguard_constraints.daita = Constraint::Only(false); - let exit_candidates = filter_matching_relay_list( - &exit_relay_query, - parsed_relays.relays(), - config.custom_lists, - ); - let entry_candidates = filter_matching_relay_list( - &entry_relay_query, - parsed_relays.relays(), - config.custom_lists, - ); + let exit_candidates = + filter_matching_relay_list(&exit_relay_query, parsed_relays.relays(), custom_lists); + let entry_candidates = + filter_matching_relay_list(&entry_relay_query, parsed_relays.relays(), custom_lists); fn pick_random_excluding<'a>(list: &'a [Relay], exclude: &'a Relay) -> Option<&'a Relay> { list.iter() @@ -830,15 +826,20 @@ impl RelaySelector { #[cfg(not(target_os = "android"))] fn get_openvpn_relay( query: &RelayQuery, - config: &NormalSelectorConfig<'_>, + custom_lists: &CustomListsSettings, parsed_relays: &ParsedRelays, ) -> Result { assert_eq!(query.tunnel_protocol, Constraint::Only(TunnelType::OpenVpn)); let exit = - Self::choose_openvpn_relay(query, config, parsed_relays).ok_or(Error::NoRelay)?; + Self::choose_openvpn_relay(query, custom_lists, parsed_relays).ok_or(Error::NoRelay)?; let endpoint = Self::get_openvpn_endpoint(query, &exit, parsed_relays)?; - let bridge = - Self::get_openvpn_bridge(query, &exit, &endpoint.protocol, parsed_relays, config)?; + let bridge = Self::get_openvpn_bridge( + query, + &exit, + &endpoint.protocol, + parsed_relays, + custom_lists, + )?; // FIXME: This assert would be better to encode at the type level. assert!(matches!(exit.endpoint_data, RelayEndpointData::Openvpn)); @@ -891,13 +892,13 @@ impl RelaySelector { relay: &Relay, protocol: &TransportProtocol, parsed_relays: &ParsedRelays, - config: &NormalSelectorConfig<'_>, + custom_lists: &CustomListsSettings, ) -> Result, Error> { if !BridgeQuery::should_use_bridge(&query.openvpn_constraints.bridge_settings) { Ok(None) } else { let bridge_query = &query.openvpn_constraints.bridge_settings.clone().unwrap(); - let custom_lists = &config.custom_lists; + let custom_lists = &custom_lists; match protocol { TransportProtocol::Udp => { log::error!("Can not use OpenVPN bridges over UDP"); @@ -1015,7 +1016,7 @@ impl RelaySelector { fn get_relay_midpoint( query: &RelayQuery, parsed_relays: &ParsedRelays, - config: &NormalSelectorConfig<'_>, + custom_lists: &CustomListsSettings, ) -> Option { use std::ops::Not; if query.location.is_any() { @@ -1023,7 +1024,7 @@ impl RelaySelector { } let matching_locations: Vec = - filter_matching_relay_list(query, parsed_relays.relays(), config.custom_lists) + filter_matching_relay_list(query, parsed_relays.relays(), custom_lists) .into_iter() .filter_map(|relay| relay.location) .unique_by(|location| location.city.clone()) @@ -1041,12 +1042,12 @@ impl RelaySelector { #[cfg(not(target_os = "android"))] fn choose_openvpn_relay( query: &RelayQuery, - config: &NormalSelectorConfig<'_>, + custom_lists: &CustomListsSettings, parsed_relays: &ParsedRelays, ) -> Option { // Filter among all valid relays let relays = parsed_relays.relays(); - let candidates = filter_matching_relay_list(query, relays, config.custom_lists); + let candidates = filter_matching_relay_list(query, relays, custom_lists); // Pick one of the valid relays. helpers::pick_random_relay(&candidates).cloned() } From ce59eb20d2cde53a829f891e5e0aca0938e654a8 Mon Sep 17 00:00:00 2001 From: Sebastian Holmin Date: Wed, 17 Apr 2024 16:01:26 +0200 Subject: [PATCH 4/4] Add test --- .../tests/relay_selector.rs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/mullvad-relay-selector/tests/relay_selector.rs b/mullvad-relay-selector/tests/relay_selector.rs index d92e7a72a9cb..7cf2329e9ae8 100644 --- a/mullvad-relay-selector/tests/relay_selector.rs +++ b/mullvad-relay-selector/tests/relay_selector.rs @@ -1296,3 +1296,30 @@ fn test_daita() { ), } } + +/// Check that if the original user query would yield a relay, the result of running the query +/// which is the intersection between the user query and any of the default queries shall never +/// fail. +#[test] +fn valid_user_setting_should_yield_relay() { + // Make a valid user relay constraint + let location = GeographicLocationConstraint::hostname("se", "got", "se9-wireguard"); + let user_query = RelayQueryBuilder::new().location(location.clone()).build(); + let user_constraints = RelayQueryBuilder::new() + .location(location.clone()) + .into_constraint(); + + let config = SelectorConfig { + relay_settings: user_constraints.into(), + ..SelectorConfig::default() + }; + let relay_selector = RelaySelector::from_list(config, RELAYS.clone()); + let user_result = relay_selector.get_relay_by_query(user_query.clone()); + for retry_attempt in 0..RETRY_ORDER.len() { + let post_unification_result = + relay_selector.get_relay(retry_attempt, RuntimeParameters::default()); + if user_result.is_ok() { + assert!(post_unification_result.is_ok(), "Expected Post-unification query to be valid because original query {:#?} yielded a connection configuration", user_query) + } + } +}