From a374f6a13229f2969d4c902b8d9d8922b40be078 Mon Sep 17 00:00:00 2001 From: Sebastian Holmin Date: Tue, 28 Nov 2023 18:29:42 +0100 Subject: [PATCH] Remove `GetCurrentLocation`. Make the daemon send two tunnel state updates, one with out IP being empty, and another with it being filled when am.i.mullvad.net responds. Update CLI for this change. Other front ends are left out. --- mullvad-cli/src/cmds/status.rs | 42 +++---- mullvad-cli/src/format.rs | 9 +- mullvad-daemon/src/geoip.rs | 13 +-- mullvad-daemon/src/lib.rs | 107 ++++++++---------- mullvad-daemon/src/management_interface.rs | 11 -- .../proto/management_interface.proto | 3 +- mullvad-management-interface/src/client.rs | 25 +--- .../src/types/conversions/states.rs | 14 ++- mullvad-types/src/states.rs | 4 +- 9 files changed, 94 insertions(+), 134 deletions(-) diff --git a/mullvad-cli/src/cmds/status.rs b/mullvad-cli/src/cmds/status.rs index a4a8add15e5d..aad6182df6ee 100644 --- a/mullvad-cli/src/cmds/status.rs +++ b/mullvad-cli/src/cmds/status.rs @@ -2,7 +2,7 @@ use anyhow::Result; use clap::{Args, Subcommand}; use futures::StreamExt; use mullvad_management_interface::{client::DaemonEvent, MullvadProxyClient}; -use mullvad_types::{device::DeviceState, states::TunnelState}; +use mullvad_types::{device::DeviceState, location::GeoIpLocation, states::TunnelState}; use crate::format; @@ -38,13 +38,21 @@ impl Status { format::print_state(&new_state, args.verbose); } - match new_state { - TunnelState::Connected { .. } | TunnelState::Disconnected => { - if args.location { - print_location(&mut rpc).await?; - } + let location = match new_state { + TunnelState::Connected { + location, + endpoint: _, + } => location, + TunnelState::Disconnected(location) => location, + _ => return Ok(()), /* No location data is available during + * connecting/disconnecting/error states */ + }; + if args.location { + if let Some(location) = location { + print_location(&location); + } else { + eprintln!("No location available") } - _ => {} } } DaemonEvent::Settings(settings) => { @@ -88,30 +96,17 @@ pub async fn handle(cmd: Option, args: StatusArgs) -> Result<()> { if args.debug { println!("Tunnel state: {state:#?}"); } else { + // TODO: respect location arg? format::print_state(&state, args.verbose); } - if args.location { - print_location(&mut rpc).await?; - } - if cmd == Some(Status::Listen) { Status::listen(rpc, args).await?; } Ok(()) } -async fn print_location(rpc: &mut MullvadProxyClient) -> Result<()> { - let location = match rpc.get_current_location().await { - Ok(location) => location, - Err(error) => match &error { - mullvad_management_interface::Error::NoLocationData => { - println!("Location data unavailable"); - return Ok(()); - } - _ => return Err(error.into()), - }, - }; +pub fn print_location(location: &GeoIpLocation) { if let Some(ipv4) = location.ipv4 { println!("IPv4: {ipv4}"); } @@ -123,7 +118,6 @@ async fn print_location(rpc: &mut MullvadProxyClient) -> Result<()> { "Position: {:.5}°N, {:.5}°W", location.latitude, location.longitude ); - Ok(()) } fn print_account_loggedout(state: &TunnelState, device: &DeviceState) { @@ -137,6 +131,6 @@ fn print_account_loggedout(state: &TunnelState, device: &DeviceState) { DeviceState::LoggedIn(_) => (), } } - TunnelState::Disconnected | TunnelState::Disconnecting(_) => (), + TunnelState::Disconnected(_) | TunnelState::Disconnecting(_) => (), } } diff --git a/mullvad-cli/src/format.rs b/mullvad-cli/src/format.rs index 2938842cefcf..68d9679d0522 100644 --- a/mullvad-cli/src/format.rs +++ b/mullvad-cli/src/format.rs @@ -4,6 +4,8 @@ use talpid_types::{ tunnel::ErrorState, }; +use crate::cmds::status::print_location; + #[macro_export] macro_rules! print_option { ($value:expr $(,)?) => {{ @@ -37,7 +39,12 @@ pub fn print_state(state: &TunnelState, verbose: bool) { format_relay_connection(endpoint, location.as_ref(), verbose) ); } - Disconnected => println!("Disconnected"), + Disconnected(disconnected_location) => { + println!("Disconnected"); + if let Some(disconnected_location) = disconnected_location { + print_location(disconnected_location); + } + } Disconnecting(_) => println!("Disconnecting..."), } } diff --git a/mullvad-daemon/src/geoip.rs b/mullvad-daemon/src/geoip.rs index 0af495e8bd83..b4ed662fa1e1 100644 --- a/mullvad-daemon/src/geoip.rs +++ b/mullvad-daemon/src/geoip.rs @@ -44,13 +44,13 @@ const LOCATION_RETRY_STRATEGY: Jittered = Jittered::jitter(ExponentialBackoff::new(Duration::from_secs(1), 4)); /// Fetch the current `GeoIpLocation` with retrys -pub async fn get_geo_location( +pub async fn get_geo_location_with_retry( rest_service: RequestServiceHandle, use_ipv6: bool, api_handle: ApiAvailabilityHandle, -) -> Option { +) -> Result { log::debug!("Fetching GeoIpLocation"); - match retry_future( + retry_future( move || send_location_request(rest_service.clone(), use_ipv6), move |result| match result { Err(error) if error.is_network_error() => !api_handle.get_state().is_offline(), @@ -59,13 +59,6 @@ pub async fn get_geo_location( LOCATION_RETRY_STRATEGY, ) .await - { - Ok(loc) => Some(loc), - Err(e) => { - log::warn!("Unable to fetch GeoIP location: {}", e.display_chain()); - None - } - } } async fn send_location_request( diff --git a/mullvad-daemon/src/lib.rs b/mullvad-daemon/src/lib.rs index 65f2c56654e2..b53975a2a777 100644 --- a/mullvad-daemon/src/lib.rs +++ b/mullvad-daemon/src/lib.rs @@ -27,7 +27,7 @@ mod tunnel; pub mod version; mod version_check; -use crate::{geoip::get_geo_location, target_state::PersistentTargetState}; +use crate::target_state::PersistentTargetState; use device::{AccountEvent, PrivateAccountAndDevice, PrivateDeviceEvent}; use futures::{ channel::{mpsc, oneshot}, @@ -200,8 +200,6 @@ pub enum DaemonCommand { Reconnect(oneshot::Sender), /// Request the current state. GetState(oneshot::Sender), - /// Get the current geographical location. - GetCurrentLocation(oneshot::Sender>), CreateNewAccount(ResponseTx), /// Request the metadata for an account. GetAccountData( @@ -417,7 +415,7 @@ impl DaemonExecutionState { match self { Running => { match tunnel_state { - TunnelState::Disconnected => mem::replace(self, Finished), + TunnelState::Disconnected(_) => mem::replace(self, Finished), _ => mem::replace(self, Exiting), }; } @@ -614,6 +612,7 @@ pub struct Daemon { tunnel_state_machine_handle: TunnelStateMachineHandle, #[cfg(target_os = "windows")] volume_update_tx: mpsc::UnboundedSender<()>, + location_abort_handle: Option, } impl Daemon @@ -847,7 +846,7 @@ where relay_list_updater.update().await; let daemon = Daemon { - tunnel_state: TunnelState::Disconnected, + tunnel_state: TunnelState::Disconnected(None), target_state, state: DaemonExecutionState::Running, #[cfg(target_os = "linux")] @@ -873,6 +872,7 @@ where tunnel_state_machine_handle, #[cfg(target_os = "windows")] volume_update_tx, + location_abort_handle: None, }; api_availability.unsuspend(); @@ -971,20 +971,33 @@ where &mut self, tunnel_state_transition: TunnelStateTransition, ) { + // Abort any ongoing calls to `self.notify_tunnel_state_when_ip_arrives` from + // previous tunnel state transitions to avoid sending an outdated information. + if let Some(handle) = self.location_abort_handle.take() { + handle.abort(); + } self.reset_rpc_sockets_on_tunnel_state_transition(&tunnel_state_transition); self.device_checker .handle_state_transition(&tunnel_state_transition); let tunnel_state = match tunnel_state_transition { - TunnelStateTransition::Disconnected => TunnelState::Disconnected, + TunnelStateTransition::Disconnected => { + self.notify_tunnel_state_when_ip_arrives(TunnelState::Disconnected) + .await; + TunnelState::Disconnected(self.parameters_generator.get_last_location().await) + } + TunnelStateTransition::Connecting(endpoint) => TunnelState::Connecting { endpoint, location: self.parameters_generator.get_last_location().await, }, - TunnelStateTransition::Connected(endpoint) => TunnelState::Connected { - endpoint, - location: self.parameters_generator.get_last_location().await, - }, + TunnelStateTransition::Connected(endpoint) => { + let make_tunnel_state = |location| TunnelState::Connected { endpoint, location }; + self.notify_tunnel_state_when_ip_arrives(make_tunnel_state.clone()) + .await; + + make_tunnel_state(self.parameters_generator.get_last_location().await) + } TunnelStateTransition::Disconnecting(after_disconnect) => { TunnelState::Disconnecting(after_disconnect) } @@ -1001,7 +1014,7 @@ where log::debug!("New tunnel state: {:?}", tunnel_state); match tunnel_state { - TunnelState::Disconnected => { + TunnelState::Disconnected(_) => { self.api_handle.availability.reset_inactivity_timer(); } _ => { @@ -1010,7 +1023,7 @@ where } match &tunnel_state { - TunnelState::Disconnected => self.state.disconnected(), + TunnelState::Disconnected(_) => self.state.disconnected(), TunnelState::Connecting { .. } => { log::debug!("Settings: {}", self.settings.summary()); } @@ -1040,6 +1053,29 @@ where self.event_listener.notify_new_state(tunnel_state); } + /// Get the out IP from am.i.mullvad.net. When it arrives, send another + /// TunnelState with the IP filled in. + async fn notify_tunnel_state_when_ip_arrives( + &mut self, + make_tunnel_state: impl FnOnce(Option) -> TunnelState + Send + 'static, + ) { + let rest_service = self.api_runtime.rest_handle().await; + let use_ipv6 = self.settings.tunnel_options.generic.enable_ipv6; + let api_handle = self.api_handle.availability.clone(); + let even_listener = self.event_listener.clone(); + + self.location_abort_handle = Some( + tokio::spawn(async move { + let location = + geoip::get_geo_location_with_retry(rest_service, use_ipv6, api_handle) + .await + .ok(); + even_listener.notify_new_state(make_tunnel_state(location)); + }) + .abort_handle(), + ); + } + fn reset_rpc_sockets_on_tunnel_state_transition( &mut self, tunnel_state_transition: &TunnelStateTransition, @@ -1091,7 +1127,6 @@ where SetTargetState(tx, state) => self.on_set_target_state(tx, state).await, Reconnect(tx) => self.on_reconnect(tx), GetState(tx) => self.on_get_state(tx), - GetCurrentLocation(tx) => self.on_get_current_location(tx).await, CreateNewAccount(tx) => self.on_create_new_account(tx), GetAccountData(tx, account_token) => self.on_get_account_data(tx, account_token), GetWwwAuthToken(tx) => self.on_get_www_auth_token(tx).await, @@ -1342,52 +1377,6 @@ where Self::oneshot_send(tx, performing_post_upgrade, "performing post upgrade"); } - async fn on_get_current_location(&mut self, tx: oneshot::Sender>) { - use self::TunnelState::*; - - match &self.tunnel_state { - Disconnected => self.update_and_send_geo_location(tx, None).await, - Connecting { location, .. } => { - Self::oneshot_send(tx, location.clone(), "current location") - } - Disconnecting(..) => Self::oneshot_send( - tx, - self.parameters_generator.get_last_location().await, - "current location", - ), - Connected { location, .. } => { - self.update_and_send_geo_location(tx, location.clone()) - .await - } - // We are not online at all at this stage so no location data is available. - Error(_) => Self::oneshot_send(tx, None, "current location"), - }; - } - - /// Fetch the current `GeoIpLocation` and send it on the return channel, - /// in a non-blocking fashion. Optionally give a chached previous location. - async fn update_and_send_geo_location( - &mut self, - tx: oneshot::Sender>, - current_relay_location: Option, - ) { - let rest_service = self.api_runtime.rest_handle().await; - let use_ipv6 = self.settings.tunnel_options.generic.enable_ipv6; - let api_handle = self.api_handle.availability.clone(); - tokio::spawn(async move { - let new_location = get_geo_location(rest_service, use_ipv6, api_handle).await; - Self::oneshot_send( - tx, - new_location.map(|fetched_location| GeoIpLocation { - ipv4: fetched_location.ipv4, - ipv6: fetched_location.ipv6, - ..current_relay_location.unwrap_or(fetched_location) - }), - "current location", - ); - }); - } - fn on_create_new_account(&mut self, tx: ResponseTx) { let account_manager = self.account_manager.clone(); tokio::spawn(async move { diff --git a/mullvad-daemon/src/management_interface.rs b/mullvad-daemon/src/management_interface.rs index f042a923e51e..75a9b0bf6fec 100644 --- a/mullvad-daemon/src/management_interface.rs +++ b/mullvad-daemon/src/management_interface.rs @@ -191,17 +191,6 @@ impl ManagementService for ManagementServiceImpl { .map(|relays| Response::new(types::RelayList::from(relays))) } - async fn get_current_location(&self, _: Request<()>) -> ServiceResult { - log::debug!("get_current_location"); - let (tx, rx) = oneshot::channel(); - self.send_command_to_daemon(DaemonCommand::GetCurrentLocation(tx))?; - let result = self.wait_for_result(rx).await?; - match result { - Some(geoip) => Ok(Response::new(types::GeoIpLocation::from(geoip))), - None => Err(Status::not_found("no location was found")), - } - } - async fn set_bridge_settings( &self, request: Request, diff --git a/mullvad-management-interface/proto/management_interface.proto b/mullvad-management-interface/proto/management_interface.proto index a27698f3176b..6f44c5f36ba8 100644 --- a/mullvad-management-interface/proto/management_interface.proto +++ b/mullvad-management-interface/proto/management_interface.proto @@ -29,7 +29,6 @@ service ManagementService { // Relays and tunnel constraints rpc UpdateRelayLocations(google.protobuf.Empty) returns (google.protobuf.Empty) {} rpc GetRelayLocations(google.protobuf.Empty) returns (RelayList) {} - rpc GetCurrentLocation(google.protobuf.Empty) returns (GeoIpLocation) {} rpc SetRelaySettings(RelaySettings) returns (google.protobuf.Empty) {} rpc SetBridgeSettings(BridgeSettings) returns (google.protobuf.Empty) {} rpc SetBridgeState(BridgeState) returns (google.protobuf.Empty) {} @@ -174,7 +173,7 @@ message ErrorState { } message TunnelState { - message Disconnected {} + message Disconnected { GeoIpLocation disconnected_location = 1; } message Connecting { TunnelStateRelayInfo relay_info = 1; } message Connected { TunnelStateRelayInfo relay_info = 1; } message Disconnecting { AfterDisconnect after_disconnect = 1; } diff --git a/mullvad-management-interface/src/client.rs b/mullvad-management-interface/src/client.rs index 64ee088d1877..e4071c8b084a 100644 --- a/mullvad-management-interface/src/client.rs +++ b/mullvad-management-interface/src/client.rs @@ -7,7 +7,6 @@ use mullvad_types::{ account::{AccountData, AccountToken, VoucherSubmission}, custom_list::{CustomList, Id}, device::{Device, DeviceEvent, DeviceId, DeviceState, RemoveDeviceEvent}, - location::GeoIpLocation, relay_constraints::{ BridgeSettings, BridgeState, ObfuscationSettings, RelayOverride, RelaySettings, }, @@ -236,16 +235,6 @@ impl MullvadProxyClient { Ok(()) } - pub async fn get_current_location(&mut self) -> Result { - let location = self - .0 - .get_current_location(()) - .await - .map_err(map_location_error)? - .into_inner(); - GeoIpLocation::try_from(location).map_err(Error::InvalidResponse) - } - pub async fn set_bridge_settings(&mut self, settings: BridgeSettings) -> Result<()> { let settings = types::BridgeSettings::from(settings); self.0 @@ -573,10 +562,9 @@ impl MullvadProxyClient { /// Set the [`AccessMethod`] which [`ApiConnectionModeProvider`] should /// pick. /// - /// - `access_method`: If `Some(access_method)`, [`ApiConnectionModeProvider`] will skip - /// ahead and return `access_method` when asked for a new access method. - /// If `None`, [`ApiConnectionModeProvider`] will pick the next access - /// method "randomly" + /// - `access_method`: If `Some(access_method)`, [`ApiConnectionModeProvider`] will skip ahead + /// and return `access_method` when asked for a new access method. If `None`, + /// [`ApiConnectionModeProvider`] will pick the next access method "randomly" /// /// [`ApiConnectionModeProvider`]: mullvad_daemon::api::ApiConnectionModeProvider pub async fn set_access_method(&mut self, api_access_method: access_method::Id) -> Result<()> { @@ -698,13 +686,6 @@ fn map_device_error(status: Status) -> Error { } } -fn map_location_error(status: Status) -> Error { - match status.code() { - Code::NotFound => Error::NoLocationData, - _other => Error::Rpc(status), - } -} - fn map_custom_list_error(status: Status) -> Error { match status.code() { Code::NotFound => { diff --git a/mullvad-management-interface/src/types/conversions/states.rs b/mullvad-management-interface/src/types/conversions/states.rs index b742aa7b004c..2541859e2745 100644 --- a/mullvad-management-interface/src/types/conversions/states.rs +++ b/mullvad-management-interface/src/types/conversions/states.rs @@ -32,8 +32,10 @@ impl From for proto::TunnelState { }; let state = match state { - MullvadTunnelState::Disconnected => { - proto::tunnel_state::State::Disconnected(proto::tunnel_state::Disconnected {}) + MullvadTunnelState::Disconnected(disconnected_location) => { + proto::tunnel_state::State::Disconnected(proto::tunnel_state::Disconnected { + disconnected_location: disconnected_location.map(proto::GeoIpLocation::from), + }) } MullvadTunnelState::Connecting { endpoint, location } => { proto::tunnel_state::State::Connecting(proto::tunnel_state::Connecting { @@ -189,7 +191,13 @@ impl TryFrom for mullvad_types::states::TunnelState { use talpid_types::{net as talpid_net, tunnel as talpid_tunnel}; let state = match state.state { - Some(proto::tunnel_state::State::Disconnected(_)) => MullvadState::Disconnected, + Some(proto::tunnel_state::State::Disconnected(proto::tunnel_state::Disconnected { + disconnected_location, + })) => MullvadState::Disconnected( + disconnected_location + .map(mullvad_types::location::GeoIpLocation::try_from) + .transpose()?, + ), Some(proto::tunnel_state::State::Connecting(proto::tunnel_state::Connecting { relay_info: Some(proto::TunnelStateRelayInfo { diff --git a/mullvad-types/src/states.rs b/mullvad-types/src/states.rs index f6131711cbcd..4a266d1d6e90 100644 --- a/mullvad-types/src/states.rs +++ b/mullvad-types/src/states.rs @@ -34,7 +34,7 @@ impl fmt::Display for TargetState { #[cfg_attr(target_os = "android", derive(IntoJava))] #[cfg_attr(target_os = "android", jnix(package = "net.mullvad.mullvadvpn.model"))] pub enum TunnelState { - Disconnected, + Disconnected(Option), Connecting { endpoint: TunnelEndpoint, location: Option, @@ -60,6 +60,6 @@ impl TunnelState { /// Returns true if the tunnel state is in the disconnected state. pub fn is_disconnected(&self) -> bool { - matches!(self, TunnelState::Disconnected) + matches!(self, TunnelState::Disconnected(_)) } }