From 7c34fa4e1ac211f04f403b1dd90e8085cb711a52 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Thu, 26 Oct 2023 11:20:09 +0200 Subject: [PATCH 1/2] Prefix remote `ip` and `port` with `remote_` in Socks5Local contexts --- mullvad-api/src/https_client_with_sni.rs | 5 ++- mullvad-api/src/proxy.rs | 8 +++-- mullvad-cli/src/cmds/api_access.rs | 21 +++++++----- .../proto/management_interface.proto | 4 +-- .../src/types/conversions/access_method.rs | 33 +++++++++++-------- mullvad-types/src/access_method.rs | 24 ++++++++------ 6 files changed, 57 insertions(+), 38 deletions(-) diff --git a/mullvad-api/src/https_client_with_sni.rs b/mullvad-api/src/https_client_with_sni.rs index 17d9f7f0d850..7d559faa7a7d 100644 --- a/mullvad-api/src/https_client_with_sni.rs +++ b/mullvad-api/src/https_client_with_sni.rs @@ -255,7 +255,10 @@ impl TryFrom for InnerConnectionMode { ProxyConfig::Socks(config) => match config { access_method::Socks5::Local(config) => { InnerConnectionMode::Socks5(SocksConfig { - peer: SocketAddr::new(IpAddr::from(Ipv4Addr::LOCALHOST), config.port), + peer: SocketAddr::new( + IpAddr::from(Ipv4Addr::LOCALHOST), + config.local_port, + ), authentication: SocksAuth::None, }) } diff --git a/mullvad-api/src/proxy.rs b/mullvad-api/src/proxy.rs index 44a2309587e5..3193fdc19b53 100644 --- a/mullvad-api/src/proxy.rs +++ b/mullvad-api/src/proxy.rs @@ -46,7 +46,7 @@ impl ProxyConfig { match self { ProxyConfig::Shadowsocks(ss) => ss.peer, ProxyConfig::Socks(socks) => match socks { - access_method::Socks5::Local(s) => s.peer, + access_method::Socks5::Local(s) => s.remote_peer, access_method::Socks5::Remote(s) => s.peer, }, } @@ -60,7 +60,11 @@ impl fmt::Display for ProxyConfig { ProxyConfig::Shadowsocks(ss) => write!(f, "Shadowsocks {}/TCP", ss.peer), ProxyConfig::Socks(socks) => match socks { access_method::Socks5::Local(s) => { - write!(f, "Socks5 {}/TCP via localhost:{}", s.peer, s.port) + write!( + f, + "Socks5 {}/TCP via localhost:{}", + s.remote_peer, s.local_port + ) } access_method::Socks5::Remote(s) => write!(f, "Socks5 {}/TCP", s.peer), }, diff --git a/mullvad-cli/src/cmds/api_access.rs b/mullvad-cli/src/cmds/api_access.rs index dc736c71c6d4..c5707a727638 100644 --- a/mullvad-cli/src/cmds/api_access.rs +++ b/mullvad-cli/src/cmds/api_access.rs @@ -116,11 +116,15 @@ impl ApiAccess { } CustomAccessMethod::Socks5(socks) => match socks { mullvad_types::access_method::Socks5::Local(local) => { - let ip = cmd.params.ip.unwrap_or(local.peer.ip()).to_string(); - let port = cmd.params.port.unwrap_or(local.peer.port()); - let local_port = cmd.params.local_port.unwrap_or(local.port); - mullvad_types::access_method::Socks5Local::from_args(ip, port, local_port) - .map(AccessMethod::from) + let remote_ip = cmd.params.ip.unwrap_or(local.remote_peer.ip()).to_string(); + let remote_port = cmd.params.port.unwrap_or(local.remote_peer.port()); + let local_port = cmd.params.local_port.unwrap_or(local.local_port); + mullvad_types::access_method::Socks5Local::from_args( + remote_ip, + remote_port, + local_port, + ) + .map(AccessMethod::from) } mullvad_types::access_method::Socks5::Remote(remote) => { let ip = cmd.params.ip.unwrap_or(remote.peer.ip()).to_string(); @@ -223,9 +227,8 @@ impl ApiAccess { 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 \"{}\"", + "Could not reach the Mullvad API using access method \"{}\"", new_access_method.get_name(), - previous_access_method.get_name() )); } }; @@ -586,8 +589,8 @@ mod pp { } writeln!(f)?; print_option!("Protocol", "Socks5 (local)"); - print_option!("Peer", local.peer); - print_option!("Local port", local.port); + print_option!("Peer", local.remote_peer); + print_option!("Local port", local.local_port); Ok(()) } }, diff --git a/mullvad-management-interface/proto/management_interface.proto b/mullvad-management-interface/proto/management_interface.proto index 35417584b313..057718bdb137 100644 --- a/mullvad-management-interface/proto/management_interface.proto +++ b/mullvad-management-interface/proto/management_interface.proto @@ -333,8 +333,8 @@ message AccessMethod { message Direct {} message Bridges {} message Socks5Local { - string ip = 1; - uint32 port = 2; + string remote_ip = 1; + uint32 remote_port = 2; uint32 local_port = 3; } message SocksAuth { diff --git a/mullvad-management-interface/src/types/conversions/access_method.rs b/mullvad-management-interface/src/types/conversions/access_method.rs index 8907c4da2947..40a96290703c 100644 --- a/mullvad-management-interface/src/types/conversions/access_method.rs +++ b/mullvad-management-interface/src/types/conversions/access_method.rs @@ -142,11 +142,15 @@ mod data { type Error = FromProtobufTypeError; fn try_from(value: proto::access_method::Socks5Local) -> Result { - Socks5Local::from_args(value.ip, value.port as u16, value.local_port as u16) - .ok_or(FromProtobufTypeError::InvalidArgument( - "Could not parse Socks5 (local) message from protobuf", - )) - .map(AccessMethod::from) + Socks5Local::from_args( + value.remote_ip, + value.remote_port as u16, + value.local_port as u16, + ) + .ok_or(FromProtobufTypeError::InvalidArgument( + "Could not parse Socks5 (local) message from protobuf", + )) + .map(AccessMethod::from) } } @@ -216,15 +220,16 @@ mod data { }, ) } - CustomAccessMethod::Socks5(Socks5::Local(Socks5Local { peer, port })) => { - proto::access_method::AccessMethod::Socks5local( - proto::access_method::Socks5Local { - ip: peer.ip().to_string(), - port: peer.port() as u32, - local_port: port as u32, - }, - ) - } + CustomAccessMethod::Socks5(Socks5::Local(Socks5Local { + remote_peer: peer, + local_port: port, + })) => proto::access_method::AccessMethod::Socks5local( + proto::access_method::Socks5Local { + remote_ip: peer.ip().to_string(), + remote_port: peer.port() as u32, + local_port: port as u32, + }, + ), CustomAccessMethod::Socks5(Socks5::Remote(Socks5Remote { peer, authentication, diff --git a/mullvad-types/src/access_method.rs b/mullvad-types/src/access_method.rs index 249c097ba12e..5473837bb245 100644 --- a/mullvad-types/src/access_method.rs +++ b/mullvad-types/src/access_method.rs @@ -205,9 +205,9 @@ pub struct Shadowsocks { #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)] pub struct Socks5Local { - pub peer: SocketAddr, + pub remote_peer: SocketAddr, /// Port on localhost where the SOCKS5-proxy listens to. - pub port: u16, + pub local_port: u16, } #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)] @@ -258,16 +258,20 @@ impl Shadowsocks { } impl Socks5Local { - pub fn new(peer: SocketAddr, port: u16) -> Self { - Self { peer, port } + pub fn new(remote_peer: SocketAddr, local_port: u16) -> Self { + Self { + remote_peer, + local_port, + } } - /// Like [new()], but tries to parse `ip` and `port` into a [`std::net::SocketAddr`] for you. - /// If `ip` or `port` are valid [`Some(Socks5Local)`] is returned, otherwise [`None`]. - pub fn from_args(ip: String, port: u16, localport: u16) -> Option { - let peer_ip = IpAddr::V4(Ipv4Addr::from_str(&ip).ok()?); - let peer = SocketAddr::new(peer_ip, port); - Some(Self::new(peer, localport)) + /// Like [new()], but tries to parse `remote_ip` and `remote_port` into a + /// [`std::net::SocketAddr`] for you. If `remote_ip` or `remote_port` are + /// valid [`Some(Socks5Local)`] is returned, otherwise [`None`]. + pub fn from_args(remote_ip: String, remote_port: u16, local_port: u16) -> Option { + let peer_ip = IpAddr::V4(Ipv4Addr::from_str(&remote_ip).ok()?); + let remote_peer = SocketAddr::new(peer_ip, remote_port); + Some(Self::new(remote_peer, local_port)) } } From d1ac9a5cb37f59b4cb08999cbe5b75bd9e422cda Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Fri, 27 Oct 2023 15:01:58 +0200 Subject: [PATCH 2/2] Remove `from_args` constructors for `access_methods` datatypes In particular, `access_methods::Socks5Local`, `access_methods::Socks5Remote` & `access_methods::Shadowsocks` have got new constructors which are all infallible. --- mullvad-cli/src/cmds/api_access.rs | 102 +++++++----------- .../src/types/conversions/access_method.rs | 54 ++++++---- mullvad-types/src/access_method.rs | 56 +++------- 3 files changed, 82 insertions(+), 130 deletions(-) diff --git a/mullvad-cli/src/cmds/api_access.rs b/mullvad-cli/src/cmds/api_access.rs index c5707a727638..55c4a9b51626 100644 --- a/mullvad-cli/src/cmds/api_access.rs +++ b/mullvad-cli/src/cmds/api_access.rs @@ -83,7 +83,7 @@ impl ApiAccess { let mut rpc = MullvadProxyClient::new().await?; let name = cmd.name().to_string(); let enabled = cmd.enabled(); - let access_method = AccessMethod::try_from(cmd)?; + let access_method = AccessMethod::from(cmd); rpc.add_access_method(name, enabled, access_method).await?; Ok(()) } @@ -99,6 +99,9 @@ impl ApiAccess { /// Edit the data of an API access method. async fn edit(cmd: EditCustomCommands) -> Result<()> { + use mullvad_types::access_method::{ + Shadowsocks, Socks5, Socks5Local, Socks5Remote, SocksAuth, + }; let mut rpc = MullvadProxyClient::new().await?; let mut api_access_method = Self::get_access_method(&mut rpc, &cmd.item).await?; @@ -107,42 +110,31 @@ impl ApiAccess { None => return Err(anyhow!("Can not edit built-in access method")), Some(x) => match x.clone() { CustomAccessMethod::Shadowsocks(shadowsocks) => { - let ip = cmd.params.ip.unwrap_or(shadowsocks.peer.ip()).to_string(); + let ip = cmd.params.ip.unwrap_or(shadowsocks.peer.ip()); let port = cmd.params.port.unwrap_or(shadowsocks.peer.port()); let password = cmd.params.password.unwrap_or(shadowsocks.password); let cipher = cmd.params.cipher.unwrap_or(shadowsocks.cipher); - mullvad_types::access_method::Shadowsocks::from_args(ip, port, cipher, password) - .map(AccessMethod::from) + AccessMethod::from(Shadowsocks::new((ip, port), cipher, password)) } CustomAccessMethod::Socks5(socks) => match socks { - mullvad_types::access_method::Socks5::Local(local) => { - let remote_ip = cmd.params.ip.unwrap_or(local.remote_peer.ip()).to_string(); + Socks5::Local(local) => { + let remote_ip = cmd.params.ip.unwrap_or(local.remote_peer.ip()); let remote_port = cmd.params.port.unwrap_or(local.remote_peer.port()); let local_port = cmd.params.local_port.unwrap_or(local.local_port); - mullvad_types::access_method::Socks5Local::from_args( - remote_ip, - remote_port, - local_port, - ) - .map(AccessMethod::from) + AccessMethod::from(Socks5Local::new((remote_ip, remote_port), local_port)) } - mullvad_types::access_method::Socks5::Remote(remote) => { - let ip = cmd.params.ip.unwrap_or(remote.peer.ip()).to_string(); + Socks5::Remote(remote) => { + let ip = cmd.params.ip.unwrap_or(remote.peer.ip()); let port = cmd.params.port.unwrap_or(remote.peer.port()); - match remote.authentication { - None => mullvad_types::access_method::Socks5Remote::from_args(ip, port), - Some(mullvad_types::access_method::SocksAuth { - username, - password, - }) => { + AccessMethod::from(match remote.authentication { + None => Socks5Remote::new((ip, port)), + Some(SocksAuth { username, password }) => { let username = cmd.params.username.unwrap_or(username); let password = cmd.params.password.unwrap_or(password); - mullvad_types::access_method::Socks5Remote::from_args_with_password( - ip, port, username, password, - ) + let auth = SocksAuth { username, password }; + Socks5Remote::new_with_authentication((ip, port), auth) } - } - .map(AccessMethod::from) + }) } }, }, @@ -151,9 +143,7 @@ impl ApiAccess { if let Some(name) = cmd.params.name { api_access_method.name = name; }; - if let Some(access_method) = access_method { - api_access_method.access_method = access_method; - } + api_access_method.access_method = access_method; rpc.update_access_method(api_access_method).await?; @@ -415,16 +405,12 @@ pub struct EditParams { /// Since these are not supposed to be used outside of the CLI, /// we define them in a hidden-away module. mod conversions { - use anyhow::{anyhow, Error}; - use mullvad_types::access_method as daemon_types; - use super::{AddCustomCommands, AddSocks5Commands, SocksAuthentication}; + use mullvad_types::access_method as daemon_types; - impl TryFrom for daemon_types::AccessMethod { - type Error = Error; - - fn try_from(value: AddCustomCommands) -> Result { - Ok(match value { + impl From for daemon_types::AccessMethod { + fn from(value: AddCustomCommands) -> Self { + match value { AddCustomCommands::Socks5(socks) => match socks { AddSocks5Commands::Local { local_port, @@ -434,14 +420,7 @@ mod conversions { disabled: _, } => { 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"))? + daemon_types::Socks5Local::new((remote_ip, remote_port), local_port).into() } AddSocks5Commands::Remote { remote_ip, @@ -449,29 +428,23 @@ mod conversions { authentication, name: _, disabled: _, - } => { + } => daemon_types::AccessMethod::from(daemon_types::Socks5::Remote( match authentication { Some(SocksAuthentication { username, password }) => { println!("Adding SOCKS5-proxy: {username}:{password}@{remote_ip}:{remote_port}"); - daemon_types::Socks5Remote::from_args_with_password( - remote_ip.to_string(), - remote_port, - username, - password + let auth = + mullvad_types::access_method::SocksAuth { username, password }; + daemon_types::Socks5Remote::new_with_authentication( + (remote_ip, remote_port), + auth, ) } None => { println!("Adding SOCKS5-proxy: {remote_ip}:{remote_port}"); - daemon_types::Socks5Remote::from_args( - remote_ip.to_string(), - remote_port, - ) + daemon_types::Socks5Remote::new((remote_ip, remote_port)) } - } - .map(daemon_types::Socks5::Remote) - .map(daemon_types::AccessMethod::from) - .ok_or(anyhow!("Could not create a remote Socks5 access method"))? - } + }, + )), }, AddCustomCommands::Shadowsocks { remote_ip, @@ -484,16 +457,13 @@ mod conversions { println!( "Adding Shadowsocks-proxy: {password} @ {remote_ip}:{remote_port} using {cipher}" ); - daemon_types::Shadowsocks::from_args( - remote_ip.to_string(), - remote_port, + daemon_types::AccessMethod::from(daemon_types::Shadowsocks::new( + (remote_ip, remote_port), cipher, password, - ) - .map(daemon_types::AccessMethod::from) - .ok_or(anyhow!("Could not create a Shadowsocks access method"))? + )) } - }) + } } } } diff --git a/mullvad-management-interface/src/types/conversions/access_method.rs b/mullvad-management-interface/src/types/conversions/access_method.rs index 40a96290703c..b4a4547fdf93 100644 --- a/mullvad-management-interface/src/types/conversions/access_method.rs +++ b/mullvad-management-interface/src/types/conversions/access_method.rs @@ -42,6 +42,8 @@ mod settings { /// [`crate::types::proto::ApiAccessMethod`] type to the internal /// [`mullvad_types::access_method::AccessMethodSetting`] data type. mod data { + use std::net::Ipv4Addr; + use crate::types::{proto, FromProtobufTypeError}; use mullvad_types::access_method::{ AccessMethod, AccessMethodSetting, BuiltInAccessMethod, CustomAccessMethod, Id, @@ -142,15 +144,15 @@ mod data { type Error = FromProtobufTypeError; fn try_from(value: proto::access_method::Socks5Local) -> Result { - Socks5Local::from_args( - value.remote_ip, - value.remote_port as u16, + let remote_ip = value.remote_ip.parse::().map_err(|_| { + FromProtobufTypeError::InvalidArgument( + "Could not parse Socks5 (local) message from protobuf", + ) + })?; + Ok(AccessMethod::from(Socks5Local::new( + (remote_ip, value.remote_port as u16), value.local_port as u16, - ) - .ok_or(FromProtobufTypeError::InvalidArgument( - "Could not parse Socks5 (local) message from protobuf", - )) - .map(AccessMethod::from) + ))) } } @@ -163,19 +165,19 @@ mod data { port, authentication, } = value; - let port = port as u16; - match authentication.map(SocksAuth::from) { - Some(SocksAuth { username, password }) => { - Socks5Remote::from_args_with_password(ip, port, username, password) - } - None => Socks5Remote::from_args(ip, port), - } - .ok_or({ + let ip = ip.parse::().map_err(|_| { FromProtobufTypeError::InvalidArgument( "Could not parse Socks5 (remote) message from protobuf", ) - }) - .map(AccessMethod::from) + })?; + let port = port as u16; + + Ok(AccessMethod::from( + match authentication.map(SocksAuth::from) { + Some(auth) => Socks5Remote::new_with_authentication((ip, port), auth), + None => Socks5Remote::new((ip, port)), + }, + )) } } @@ -183,11 +185,17 @@ mod data { type Error = FromProtobufTypeError; fn try_from(value: proto::access_method::Shadowsocks) -> Result { - Shadowsocks::from_args(value.ip, value.port as u16, value.cipher, value.password) - .ok_or(FromProtobufTypeError::InvalidArgument( - "Could not parse Shadowsocks message from protobuf", - )) - .map(AccessMethod::from) + let ip = value.ip.parse::().map_err(|_| { + FromProtobufTypeError::InvalidArgument( + "Could not parse Socks5 (remote) message from protobuf", + ) + })?; + + Ok(AccessMethod::from(Shadowsocks::new( + (ip, value.port as u16), + value.cipher, + value.password, + ))) } } diff --git a/mullvad-types/src/access_method.rs b/mullvad-types/src/access_method.rs index 5473837bb245..f4a76392fa3c 100644 --- a/mullvad-types/src/access_method.rs +++ b/mullvad-types/src/access_method.rs @@ -1,7 +1,7 @@ use std::str::FromStr; use serde::{Deserialize, Serialize}; -use std::net::{IpAddr, Ipv4Addr, SocketAddr, SocketAddrV4}; +use std::net::SocketAddr; /// Daemon settings for API access methods. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] @@ -241,66 +241,40 @@ impl BuiltInAccessMethod { } impl Shadowsocks { - pub fn new(peer: SocketAddr, cipher: String, password: String) -> Self { + pub fn new>(peer: I, cipher: String, password: String) -> Self { Shadowsocks { - peer, + peer: peer.into(), password, cipher, } } - - /// Like [new()], but tries to parse `ip` and `port` into a [`std::net::SocketAddr`] for you. - /// If `ip` or `port` are valid [`Some(Socks5Local)`] is returned, otherwise [`None`]. - pub fn from_args(ip: String, port: u16, cipher: String, password: String) -> Option { - let peer = SocketAddrV4::new(Ipv4Addr::from_str(&ip).ok()?, port).into(); - Some(Self::new(peer, cipher, password)) - } } impl Socks5Local { - pub fn new(remote_peer: SocketAddr, local_port: u16) -> Self { + pub fn new>(remote_peer: I, local_port: u16) -> Self { Self { - remote_peer, + remote_peer: remote_peer.into(), local_port, } } - - /// Like [new()], but tries to parse `remote_ip` and `remote_port` into a - /// [`std::net::SocketAddr`] for you. If `remote_ip` or `remote_port` are - /// valid [`Some(Socks5Local)`] is returned, otherwise [`None`]. - pub fn from_args(remote_ip: String, remote_port: u16, local_port: u16) -> Option { - let peer_ip = IpAddr::V4(Ipv4Addr::from_str(&remote_ip).ok()?); - let remote_peer = SocketAddr::new(peer_ip, remote_port); - Some(Self::new(remote_peer, local_port)) - } } impl Socks5Remote { - pub fn new(peer: SocketAddr) -> Self { + pub fn new>(peer: I) -> Self { Self { - peer, + peer: peer.into(), authentication: None, } } - /// Like [new()], but tries to parse `ip` and `port` into a [`std::net::SocketAddr`] for you. - /// If `ip` or `port` are valid [`Some(Socks5Remote)`] is returned, otherwise [`None`]. - pub fn from_args(ip: String, port: u16) -> Option { - let peer_ip = IpAddr::V4(Ipv4Addr::from_str(&ip).ok()?); - let peer = SocketAddr::new(peer_ip, port); - Some(Self::new(peer)) - } - - /// Like [from_args()], but with authentication. - pub fn from_args_with_password( - ip: String, - port: u16, - username: String, - password: String, - ) -> Option { - let mut socks = Self::from_args(ip, port)?; - socks.authentication = Some(SocksAuth { username, password }); - Some(socks) + pub fn new_with_authentication>( + peer: I, + authentication: SocksAuth, + ) -> Self { + Self { + peer: peer.into(), + authentication: Some(authentication), + } } }