From 189d900a08b4330c7cf613d92fa1c5755387e7af Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Thu, 28 Sep 2023 12:51:26 +0200 Subject: [PATCH] UX improvements for `mullvad api-access` - Re-phrase help texts for a lot of `mullvad api-access` commands - Add to help texts for some `mullvad api-access` commands - Compact the output of `mullvad api-access test` - `mullvad api-access status` is changed to `mullvad api-access get` to align with other `mullvad` commands. - `mullvad api-access get` does not print the enabled/disabled status of the shown access method - Rotate access method if the currently active one is updated or removed - Fix reset access method after `mullvad api-access test` After running `mullvad api-access test`, the previously used access method should be used, which was not the case previously. - Fix `mullvad api-access use` API connectivity check - `mullvad api-access use` now runs a test-routine to check that the new access method will function before comitting to it. If this check fails, the previously used access method will be used instead - guarantee that `set_api_access_method` has finished upon returning. Make `mullvad_api::rest::next_api_endpoint` `async` and send a message upon completion. This is propagated to the caller of `next_api_endpoint` which can `await` the result --- mullvad-api/src/rest.rs | 24 +++- mullvad-cli/src/cmds/api_access.rs | 177 +++++++++++++++++++--------- mullvad-cli/src/main.rs | 17 ++- mullvad-daemon/src/access_method.rs | 137 ++++++++++++++++----- mullvad-daemon/src/lib.rs | 7 +- 5 files changed, 269 insertions(+), 93 deletions(-) diff --git a/mullvad-api/src/rest.rs b/mullvad-api/src/rest.rs index 0fc31353a7fa..674bcf8c4eb0 100644 --- a/mullvad-api/src/rest.rs +++ b/mullvad-api/src/rest.rs @@ -77,6 +77,10 @@ pub enum Error { /// The string given was not a valid URI. #[error(display = "Not a valid URI")] UriError(#[error(source)] http::uri::InvalidUri), + + /// A new API config was requested, but the request could not be completed. + #[error(display = "Failed to rotate API config")] + NextApiConfigError, } impl Error { @@ -207,7 +211,9 @@ impl< if err.is_network_error() && !api_availability.get_state().is_offline() { log::error!("{}", err.display_chain_with_msg("HTTP request failed")); if let Some(tx) = tx { - let _ = tx.unbounded_send(RequestCommand::NextApiConfig); + let (completion_tx, _completion_rx) = oneshot::channel(); + let _ = + tx.unbounded_send(RequestCommand::NextApiConfig(completion_tx)); } } } @@ -223,10 +229,11 @@ impl< RequestCommand::Reset => { self.connector_handle.reset(); } - RequestCommand::NextApiConfig => { + RequestCommand::NextApiConfig(completion_tx) => { #[cfg(feature = "api-override")] if API.force_direct_connection { log::debug!("Ignoring API connection mode"); + let _ = completion_tx.send(Ok(())); return; } @@ -240,6 +247,8 @@ impl< self.connector_handle.set_connection_mode(new_config); } } + + let _ = completion_tx.send(Ok(())); } } } @@ -274,10 +283,13 @@ impl RequestServiceHandle { } /// Forcibly update the connection mode. - pub fn next_api_endpoint(&self) -> Result<()> { + pub async fn next_api_endpoint(&self) -> Result<()> { + let (completion_tx, completion_rx) = oneshot::channel(); self.tx - .unbounded_send(RequestCommand::NextApiConfig) - .map_err(|_| Error::SendError) + .unbounded_send(RequestCommand::NextApiConfig(completion_tx)) + .map_err(|_| Error::SendError)?; + + completion_rx.await.map_err(|_| Error::NextApiConfigError)? } } @@ -288,7 +300,7 @@ pub(crate) enum RequestCommand { oneshot::Sender>, ), Reset, - NextApiConfig, + NextApiConfig(oneshot::Sender>), } /// A REST request that is sent to the RequestService to be executed. diff --git a/mullvad-cli/src/cmds/api_access.rs b/mullvad-cli/src/cmds/api_access.rs index 75fe7c44bf21..a03d3ba11fbb 100644 --- a/mullvad-cli/src/cmds/api_access.rs +++ b/mullvad-cli/src/cmds/api_access.rs @@ -8,27 +8,31 @@ use talpid_types::net::openvpn::SHADOWSOCKS_CIPHERS; #[derive(Subcommand, Debug, Clone)] pub enum ApiAccess { - /// List the configured API access methods - List, + /// Display the current API access method. + Get, /// Add a custom API access method #[clap(subcommand)] Add(AddCustomCommands), - /// Edit an API access method + /// Lists all API access methods + /// + /// * = Enabled + List, + /// Edit a custom API access method Edit(EditCustomCommands), - /// Remove an API access method + /// Remove a custom API access method Remove(SelectItem), /// Enable an API access method Enable(SelectItem), /// Disable an API access method Disable(SelectItem), - /// Test an API access method - Test(SelectItem), - /// Force the use of a specific API access method. + /// Try to use a specific API access method (If the API is unreachable, reverts back to the previous access method) + /// + /// Selecting "Direct" will connect to the Mullvad API without going through any proxy. This connection use https and is therefore encrypted. /// - /// Selecting "Mullvad Bridges" respects your current bridge settings. + /// Selecting "Mullvad Bridges" respects your current bridge settings Use(SelectItem), - /// Show which access method is currently used to access the Mullvad API. - Status, + /// Try to reach the Mullvad API using a specific access method + Test(SelectItem), } impl ApiAccess { @@ -54,8 +58,8 @@ impl ApiAccess { ApiAccess::Use(cmd) => { Self::set(cmd).await?; } - ApiAccess::Status => { - Self::status().await?; + ApiAccess::Get => { + Self::get().await?; } }; Ok(()) @@ -173,34 +177,75 @@ impl ApiAccess { /// Test an access method to see if it successfully reaches the Mullvad API. async fn test(item: SelectItem) -> Result<()> { let mut rpc = MullvadProxyClient::new().await?; + // Retrieve the currently used access method. We will reset to this + // after we are done testing. + let previous_access_method = rpc.get_current_api_access_method().await?; let access_method = Self::get_access_method(&mut rpc, &item).await?; + + println!("Testing access method \"{}\"", access_method.name); rpc.set_access_method(access_method.get_id()).await?; // Make the daemon perform an network request which involves talking to the Mullvad API. - match rpc.get_api_addresses().await { - Ok(_) => println!("Connected to the Mullvad API!"), - Err(_) => println!( - "Could *not* connect to the Mullvad API using access method \"{}\"", - access_method.name - ), - } - - Ok(()) + let result = match rpc.get_api_addresses().await { + Ok(_) => { + println!("Success!"); + Ok(()) + } + Err(_) => Err(anyhow!("Could not reach the Mullvad API")), + }; + // In any case, switch back to the previous access method. + rpc.set_access_method(previous_access_method.get_id()) + .await?; + result } - /// Force the use of a specific access method when trying to reach the - /// Mullvad API. If this method fails, the daemon will resume the automatic - /// roll-over behavior (which is the default). + /// Try to use of a specific [`AccessMethodSetting`] for subsequent calls to + /// the Mullvad API. + /// + /// First, a test will be performed to check that the new + /// [`AccessMethodSetting`] is able to reach the API. If it can, the daemon + /// will set this [`AccessMethodSetting`] to be used by the API runtime. + /// + /// If the new [`AccessMethodSetting`] fails, the daemon will perform a + /// roll-back to the previously used [`AccessMethodSetting`]. If that never + /// worked, or has recently stopped working, the daemon will start to + /// automatically try to find a working [`AccessMethodSetting`] among the + /// configured ones. async fn set(item: SelectItem) -> Result<()> { let mut rpc = MullvadProxyClient::new().await?; - let access_method = Self::get_access_method(&mut rpc, &item).await?; - rpc.set_access_method(access_method.get_id()).await?; + let previous_access_method = rpc.get_current_api_access_method().await?; + let mut new_access_method = Self::get_access_method(&mut rpc, &item).await?; + // Try to reach the API with the newly selected access method. + rpc.set_access_method(new_access_method.get_id()).await?; + match rpc.get_api_addresses().await { + Ok(_) => (), + Err(_) => { + // Roll-back to the previous access method + rpc.set_access_method(previous_access_method.get_id()) + .await?; + return Err(anyhow!( + "Could not reach the Mullvad API using access method \"{}\". Rolling back to \"{}\"", + new_access_method.get_name(), + previous_access_method.get_name() + )); + } + }; + // It worked! Let the daemon keep using this access method. + let display_name = new_access_method.get_name(); + // Toggle the enabled status if needed + if !new_access_method.enabled() { + new_access_method.enable(); + rpc.update_access_method(new_access_method).await?; + } + println!("Using access method \"{}\"", display_name); Ok(()) } - async fn status() -> Result<()> { + async fn get() -> Result<()> { let mut rpc = MullvadProxyClient::new().await?; let current = rpc.get_current_api_access_method().await?; - println!("{}", pp::ApiAccessMethodFormatter::new(¤t)); + let mut access_method_formatter = pp::ApiAccessMethodFormatter::new(¤t); + access_method_formatter.settings.write_enabled = false; + println!("{}", access_method_formatter); Ok(()) } @@ -218,16 +263,16 @@ impl ApiAccess { #[derive(Subcommand, Debug, Clone)] pub enum AddCustomCommands { - /// Configure a local SOCKS5 proxy + /// Configure a SOCKS5 proxy #[clap(subcommand)] Socks5(AddSocks5Commands), - /// Configure Shadowsocks proxy + /// Configure a custom Shadowsocks proxy to use as an API access method Shadowsocks { /// An easy to remember name for this custom proxy name: String, - /// The IP of the remote Shadowsocks server + /// The IP of the remote Shadowsocks-proxy remote_ip: IpAddr, - /// The port of the remote Shadowsocks server + /// Port on which the remote Shadowsocks-proxy listens for traffic #[arg(default_value = "443")] remote_port: u16, /// Password for authentication @@ -249,9 +294,9 @@ pub enum AddSocks5Commands { Remote { /// An easy to remember name for this custom proxy name: String, - /// The IP of the remote proxy server + /// IP of the remote SOCKS5-proxy remote_ip: IpAddr, - /// The port of the remote proxy server + /// Port on which the remote SOCKS5-proxy listens for traffic remote_port: u16, #[clap(flatten)] authentication: Option, @@ -386,16 +431,15 @@ mod conversions { name: _, disabled: _, } => { - println!("Adding Local SOCKS5-proxy: localhost:{local_port} => {remote_ip}:{remote_port}"); - let socks_proxy = daemon_types::Socks5::Local( - daemon_types::Socks5Local::from_args( - remote_ip.to_string(), - remote_port, - local_port, - ) - .ok_or(anyhow!("Could not create a local Socks5 api proxy"))?, - ); - daemon_types::AccessMethod::from(socks_proxy) + println!("Adding SOCKS5-proxy: localhost:{local_port} => {remote_ip}:{remote_port}"); + daemon_types::Socks5Local::from_args( + remote_ip.to_string(), + remote_port, + local_port, + ) + .map(daemon_types::Socks5::Local) + .map(daemon_types::AccessMethod::from) + .ok_or(anyhow!("Could not create a local Socks5 access method"))? } AddSocks5Commands::Remote { remote_ip, @@ -424,7 +468,7 @@ mod conversions { } .map(daemon_types::Socks5::Remote) .map(daemon_types::AccessMethod::from) - .ok_or(anyhow!("Could not create a remote Socks5 api proxy"))? + .ok_or(anyhow!("Could not create a remote Socks5 access method"))? } }, AddCustomCommands::Shadowsocks { @@ -438,14 +482,14 @@ mod conversions { println!( "Adding Shadowsocks-proxy: {password} @ {remote_ip}:{remote_port} using {cipher}" ); - let shadowsocks_proxy = daemon_types::Shadowsocks::from_args( + daemon_types::Shadowsocks::from_args( remote_ip.to_string(), remote_port, cipher, password, ) - .ok_or(anyhow!("Could not create a Shadowsocks api proxy"))?; - daemon_types::AccessMethod::from(shadowsocks_proxy) + .map(daemon_types::AccessMethod::from) + .ok_or(anyhow!("Could not create a Shadowsocks access method"))? } }) } @@ -460,11 +504,29 @@ mod pp { pub struct ApiAccessMethodFormatter<'a> { api_access_method: &'a AccessMethodSetting, + pub settings: FormatterSettings, + } + + pub struct FormatterSettings { + /// If the formatter should print the enabled status of an + /// [`AcessMethodSetting`] (*) next to its name. + pub write_enabled: bool, + } + + impl Default for FormatterSettings { + fn default() -> Self { + Self { + write_enabled: true, + } + } } impl<'a> ApiAccessMethodFormatter<'a> { pub fn new(api_access_method: &'a AccessMethodSetting) -> ApiAccessMethodFormatter<'a> { - ApiAccessMethodFormatter { api_access_method } + ApiAccessMethodFormatter { + api_access_method, + settings: Default::default(), + } } } @@ -483,12 +545,17 @@ mod pp { match &self.api_access_method.access_method { AccessMethod::BuiltIn(method) => { write!(f, "{}", method.canonical_name())?; - write_status(f, self.api_access_method.enabled()) + if self.settings.write_enabled { + write_status(f, self.api_access_method.enabled())?; + } + Ok(()) } AccessMethod::Custom(method) => match &method { CustomAccessMethod::Shadowsocks(shadowsocks) => { write!(f, "{}", self.api_access_method.get_name())?; - write_status(f, self.api_access_method.enabled())?; + if self.settings.write_enabled { + write_status(f, self.api_access_method.enabled())?; + } writeln!(f)?; print_option!("Protocol", format!("Shadowsocks [{}]", shadowsocks.cipher)); print_option!("Peer", shadowsocks.peer); @@ -498,7 +565,9 @@ mod pp { CustomAccessMethod::Socks5(socks) => match socks { Socks5::Remote(remote) => { write!(f, "{}", self.api_access_method.get_name())?; - write_status(f, self.api_access_method.enabled())?; + if self.settings.write_enabled { + write_status(f, self.api_access_method.enabled())?; + } writeln!(f)?; print_option!("Protocol", "Socks5"); print_option!("Peer", remote.peer); @@ -513,7 +582,9 @@ mod pp { } Socks5::Local(local) => { write!(f, "{}", self.api_access_method.get_name())?; - write_status(f, self.api_access_method.enabled())?; + if self.settings.write_enabled { + write_status(f, self.api_access_method.enabled())?; + } writeln!(f)?; print_option!("Protocol", "Socks5 (local)"); print_option!("Peer", local.peer); diff --git a/mullvad-cli/src/main.rs b/mullvad-cli/src/main.rs index a79465f9658d..7a09a4eebdf7 100644 --- a/mullvad-cli/src/main.rs +++ b/mullvad-cli/src/main.rs @@ -71,9 +71,20 @@ enum Cli { #[clap(subcommand)] Relay(relay::Relay), - /// Manage use of access methods for reaching the Mullvad API. - /// Can be used to connect to the the Mullvad API via one of the - /// Mullvad bridge servers or a custom proxy (SOCKS5 & Shadowsocks). + /// Manage Mullvad API access methods. + /// + /// Access methods are used to connect to the the Mullvad API via one of + /// Mullvad's bridge servers or a custom proxy (SOCKS5 & Shadowsocks) when + /// and where establishing a direct connection does not work. + /// + /// If the Mullvad daemon is unable to connect to the Mullvad API, it will + /// automatically try to use any other configured access method and re-try + /// the API call. If it succeeds, all subsequent API calls are made using + /// the new access method. Otherwise it will re-try using yet another access + /// method. + /// + /// The Mullvad API is used for logging in, accessing the relay list, + /// rotating Wireguard keys and more. #[clap(subcommand)] ApiAccess(api_access::ApiAccess), diff --git a/mullvad-daemon/src/access_method.rs b/mullvad-daemon/src/access_method.rs index ea096cf5ba64..013cf9d7ce51 100644 --- a/mullvad-daemon/src/access_method.rs +++ b/mullvad-daemon/src/access_method.rs @@ -2,7 +2,10 @@ use crate::{ settings::{self, MadeChanges}, Daemon, EventListener, }; -use mullvad_types::access_method::{self, AccessMethod, AccessMethodSetting}; +use mullvad_types::{ + access_method::{self, AccessMethod, AccessMethodSetting}, + settings::Settings, +}; #[derive(err_derive::Error, Debug)] pub enum Error { @@ -19,15 +22,35 @@ pub enum Error { /// the user should do a factory reset. #[error(display = "No access methods are configured")] NoMethodsExist, + /// Access method could not be rotate + #[error(display = "Access method could not be rotated")] + RotationError, /// Access methods settings error #[error(display = "Settings error")] Settings(#[error(source)] settings::Error), } +/// A tiny datastructure used for signaling whether the daemon should force a +/// rotation of the currently used [`AccessMethodSetting`] or not, and if so: +/// how it should do it. +pub enum Command { + /// There is no need to force a rotation of [`AccessMethodSetting`] + Nothing, + /// Select the next available [`AccessMethodSetting`], whichever that is + Rotate, + /// Select the [`AccessMethodSetting`] with a certain [`access_method::Id`] + Set(access_method::Id), +} + impl Daemon where L: EventListener + Clone + Send + 'static, { + /// Add a [`AccessMethod`] to the daemon's settings. + /// + /// If the daemon settings are successfully updated, the + /// [`access_method::Id`] of the newly created [`AccessMethodSetting`] + /// (which has been derived from the [`AccessMethod`]) is returned. pub async fn add_access_method( &mut self, name: String, @@ -44,71 +67,119 @@ where .map_err(Error::Settings) } + /// Remove a [`AccessMethodSetting`] from the daemon's saved settings. + /// + /// If the [`AccessMethodSetting`] which is currently in use happens to be + /// removed, the daemon should force a rotation of the active API endpoint. pub async fn remove_access_method( &mut self, access_method: access_method::Id, ) -> Result<(), Error> { // Make sure that we are not trying to remove a built-in API access // method - match self.settings.api_access_methods.find(&access_method) { - None => return Ok(()), + let command = match self.settings.api_access_methods.find(&access_method) { Some(api_access_method) => { if api_access_method.is_builtin() { - return Err(Error::RemoveBuiltIn); + Err(Error::RemoveBuiltIn) + } else if api_access_method.get_id() == self.get_current_access_method()?.get_id() { + Ok(Command::Rotate) + } else { + Ok(Command::Nothing) } } - }; + None => Ok(Command::Nothing), + }?; self.settings .update(|settings| settings.api_access_methods.remove(&access_method)) .await .map(|did_change| self.notify_on_change(did_change)) - .map_err(Error::Settings) + .map_err(Error::Settings)? + .process_command(command) + .await } - pub fn set_api_access_method(&mut self, access_method: access_method::Id) -> Result<(), Error> { - if let Some(access_method) = self.settings.api_access_methods.find(&access_method) { - { - let mut connection_modes = self.connection_modes.lock().unwrap(); - connection_modes.set_access_method(access_method.clone()); - } - // Force a rotation of Access Methods. - let _ = self.api_handle.service().next_api_endpoint(); - Ok(()) - } else { - Err(Error::NoSuchMethod(access_method)) + /// Set a [`AccessMethodSetting`] as the current API access method. + /// + /// If successful, the daemon will force a rotation of the active API access + /// method, which means that subsequent API calls will use the new + /// [`AccessMethodSetting`] to figure out the API endpoint. + pub async fn set_api_access_method( + &mut self, + access_method: access_method::Id, + ) -> Result<(), Error> { + let access_method = self + .settings + .api_access_methods + .find(&access_method) + .ok_or(Error::NoSuchMethod(access_method))?; + { + let mut connection_modes = self.connection_modes.lock().unwrap(); + connection_modes.set_access_method(access_method.clone()); } + // Force a rotation of Access Methods. + // + // This is not a call to `process_command` due to the restrictions on + // recursively calling async functions. + self.force_api_endpoint_rotation().await } /// "Updates" an [`AccessMethodSetting`] by replacing the existing entry /// with the argument `access_method_update` if an existing entry with - /// matching UUID is found. + /// matching [`access_method::Id`] is found. + /// + /// If the currently active [`AccessMethodSetting`] is updated, the daemon + /// will automatically use this updated [`AccessMethodSetting`] when + /// performing subsequent API calls. pub async fn update_access_method( &mut self, access_method_update: AccessMethodSetting, ) -> Result<(), Error> { - self.settings - .update(|settings| { - let access_methods = &mut settings.api_access_methods; - if let Some(access_method) = access_methods.find_mut(&access_method_update.get_id()) - { - *access_method = access_method_update + let current = self.get_current_access_method()?; + let mut command = Command::Nothing; + let settings_update = |settings: &mut Settings| { + if let Some(access_method) = settings + .api_access_methods + .find_mut(&access_method_update.get_id()) + { + *access_method = access_method_update; + if access_method.get_id() == current.get_id() { + command = Command::Set(access_method.get_id()) } - }) + } + }; + + self.settings + .update(settings_update) .await .map(|did_change| self.notify_on_change(did_change)) - .map_err(Error::Settings) + .map_err(Error::Settings)? + .process_command(command) + .await } /// Return the [`AccessMethodSetting`] which is currently used to access the /// Mullvad API. - pub fn get_current_access_method(&mut self) -> Result { + pub fn get_current_access_method(&self) -> Result { let connections_modes = self.connection_modes.lock().unwrap(); Ok(connections_modes.peek()) } + /// Change which [`AccessMethodSetting`] which will be used to figure out + /// the Mullvad API endpoint. + async fn force_api_endpoint_rotation(&self) -> Result<(), Error> { + self.api_handle + .service() + .next_api_endpoint() + .await + .map_err(|error| { + log::error!("Failed to rotate API endpoint: {}", error); + Error::RotationError + }) + } + /// If settings were changed due to an update, notify all listeners. - fn notify_on_change(&mut self, settings_changed: MadeChanges) { + fn notify_on_change(&mut self, settings_changed: MadeChanges) -> &mut Self { if settings_changed { self.event_listener .notify_settings(self.settings.to_settings()); @@ -124,5 +195,15 @@ where .collect(), ) }; + self + } + + /// The semantics of the [`Command`] datastructure. + async fn process_command(&mut self, command: Command) -> Result<(), Error> { + match command { + Command::Nothing => Ok(()), + Command::Rotate => self.force_api_endpoint_rotation().await, + Command::Set(id) => self.set_api_access_method(id).await, + } } } diff --git a/mullvad-daemon/src/lib.rs b/mullvad-daemon/src/lib.rs index 3def0855c207..f7343acc877a 100644 --- a/mullvad-daemon/src/lib.rs +++ b/mullvad-daemon/src/lib.rs @@ -1077,7 +1077,7 @@ where RemoveApiAccessMethod(tx, method) => self.on_remove_api_access_method(tx, method).await, UpdateApiAccessMethod(tx, method) => self.on_update_api_access_method(tx, method).await, GetCurrentAccessMethod(tx) => self.on_get_current_api_access_method(tx), - SetApiAccessMethod(tx, method) => self.on_set_api_access_method(tx, method), + SetApiAccessMethod(tx, method) => self.on_set_api_access_method(tx, method).await, GetApiAddresses(tx) => self.on_get_api_addresses(tx).await, IsPerformingPostUpgrade(tx) => self.on_is_performing_post_upgrade(tx), GetCurrentVersion(tx) => self.on_get_current_version(tx), @@ -1970,7 +1970,7 @@ where .notify_settings(self.settings.to_settings()); self.relay_selector .set_config(new_selector_config(&self.settings)); - if let Err(error) = self.api_handle.service().next_api_endpoint() { + if let Err(error) = self.api_handle.service().next_api_endpoint().await { log::error!("Failed to rotate API endpoint: {}", error); } self.reconnect_tunnel(); @@ -2284,13 +2284,14 @@ where Self::oneshot_send(tx, result, "remove_api_access_method response"); } - fn on_set_api_access_method( + async fn on_set_api_access_method( &mut self, tx: ResponseTx<(), Error>, access_method: mullvad_types::access_method::Id, ) { let result = self .set_api_access_method(access_method) + .await .map_err(Error::AccessMethodError); Self::oneshot_send(tx, result, "set_api_access_method response"); }