Merge pull request #248 from jamesmcm/protonvpn_customwg_portforwardi…

…ng_fix

Fix port forwarding for custom ProtonVPN WG config

.gitignore

@@ -1,3 +1,4 @@
 /target
 Cargo.lock
 dialoguer
+test.sh

Cargo.toml

@@ -26,11 +26,11 @@ signal-hook = "0.3"
 walkdir = "2"
 chrono = "0.4"
 bs58 = "0.5"
-nix = { version = "0.27", features = ["signal", "process"] }
-config = "0.13"
+nix = { version = "0.28", features = ["signal", "process"] }
+config = "0.14"
 basic_tcp_proxy = "0.3.2"
-strum = "0.25"
-strum_macros = "0.25"
+strum = "0.26"
+strum_macros = "0.26"
 
 [package.metadata.rpm]
 package = "vopono"

README.md

@@ -43,11 +43,20 @@ lynx all running through different VPN connections:
 
 \*\*\* For ProtonVPN you can generate and download specific Wireguard config
 files, and use them as a custom provider config. See the [User Guide](USERGUIDE.md)
-for details. [Port Forwarding](https://protonvpn.com/support/port-forwarding-manual-setup/) is supported with the `--port-forwarding` argument for both OpenVPN and Wireguard (with `--provider custom --custom xxx.conf --protocol wireguard` ). `natpmpc` must be installed. Note for OpenVPN you must generate the OpenVPN config files appending `+pmp` to your OpenVPN username, and you must choose servers which support this feature (e.g. at the time of writing, the Romania servers do). The assigned port is then printed to the terminal where vopono was launched - this should then be set in any applications that require it.
+for details. [Port Forwarding](https://protonvpn.com/support/port-forwarding-manual-setup/) is supported with the `--port-forwarding` argument for both OpenVPN and Wireguard.
+Note for using a custom config with Wireguard, the port forwarding implementation to be used should be specified with `--custom-port-forwarding`
+(i.e. with `--provider custom --custom xxx.conf --protocol wireguard --custom-port-forwarding protonvpn` ). `natpmpc` must be installed.
+Note for OpenVPN you must generate the OpenVPN config files appending `+pmp` to your OpenVPN username, and you must choose servers which support this feature
+(e.g. at the time of writing, the Romania servers do). The assigned port is then printed to the terminal where vopono was launched - this should then be set in any applications that require it.
+The port can also be passed to a custom script that will be executed
+within the network namespace via the `--port-forwarding-callback`
+argument.
 
 
 \*\*\*\* Cloudflare Warp uses its own protocol. Set both the provider and
-protocol to `warp`. Note you must first register with `sudo warp-cli register` and then run it once with `sudo warp-svc` and `sudo warp-cli connect` outside of vopono. Please verify this works first before trying it with vopono.
+protocol to `warp`. Note you must first register with `sudo warp-cli register` and then run it once with `sudo warp-svc` and `sudo warp-cli connect` outside of vopono.
+Please verify this works first before trying it with vopono. Note there
+may also be issues with Warp overriding the DNS settings.
 
 
 ## Usage
@@ -175,7 +184,7 @@ $ rustc --version
 - When launching a new application in an existing vopono namespace, any
   modifications to the firewall rules (i.e. forwarding and opening
   ports) will not be applied (they are only used when creating the
-  namespace).
+  namespace). The same applies for port forwarding.
 - OpenVPN credentials are always stored in plaintext in configuration - may add
   option to not store credentials, but it seems OpenVPN needs them
   provided in plaintext.

USERGUIDE.md

@@ -488,7 +488,7 @@ Due to the way Wireguard configuration generation is handled, this should be
 generated online and then used as a custom configuration, e.g.:
 
 ```bash
-$ vopono -v exec --provider custom --custom testwg-UK-17.conf --protocol wireguard --port-forwarding firefox-developer-edition
+$ vopono -v exec --provider custom --custom testwg-UK-17.conf --protocol wireguard --custom-port-forwarding protonvpn firefox-developer-edition
 ```
 
 #### Port Forwarding
@@ -508,10 +508,21 @@ The port you are allocated will then be printed to the console like:
 
 And that is the port you would then set up in applications that require it.
 
+For Wireguard custom configs mentioned above, you must set the
+`--custom-port-forwarding protonvpn` argument, so vopono knows which
+port forwarding implementation to use for the custom config file.
+
+The port can also be passed to a script (which will be executed within
+the network namespace every 45 seconds when the port is refreshed) by passing the script 
+as the `--port-forwarding-callback` argument - the port will be passed
+as the first argument (i.e. `$1`).
+
 ### PrivateInternetAccess
 
 Port forwaring supported with the `--port-forwarding` option, use the `--port-forwarding-callback` option to specify a command to run when the port is refreshed.
 
+Note the usual `-o` / `--open-ports` argument has no effect here as we only know the port number assigned after connecting to PIA.
+
 ### Cloudflare Warp
 
 Cloudflare Warp users must first register with Warp via the CLI client:

src/args.rs

@@ -144,7 +144,6 @@ pub struct ExecCommand {
     pub working_directory: Option<String>,
 
     /// Custom VPN Provider - OpenVPN or Wireguard config file (will override other settings)
-    // TODO: Check From OsStr part works
     #[clap(long = "custom")]
     pub custom_config: Option<PathBuf>,
 
@@ -217,6 +216,10 @@ pub struct ExecCommand {
     #[clap(long = "port-forwarding")]
     pub port_forwarding: bool,
 
+    /// Port forwarding implementation to use for custom config file with --custom-config
+    #[clap(long = "custom-port-forwarding", ignore_case = true)]
+    pub custom_port_forwarding: Option<WrappedArg<VpnProvider>>,
+
     /// Path or alias to executable script or binary to be called with the port as an argumnet
     /// when the port forwarding is refreshed (PIA only)
     #[clap(long = "port-forwarding-callback")]

src/exec.rs

@@ -46,6 +46,7 @@ pub fn exec(command: ExecCommand, uiclient: &dyn UiClient) -> anyhow::Result<()>
         std::fs::OpenOptions::new()
             .write(true)
             .create(true)
+            .truncate(false)
             .read(true)
             .open(&config_path)?;
     }
@@ -152,6 +153,21 @@ pub fn exec(command: ExecCommand, uiclient: &dyn UiClient) -> anyhow::Result<()>
         command.port_forwarding
     };
 
+    // Custom port forwarding (implementation to use for --custom-config)
+    // TODO: Allow fully custom handling separate callback script?
+    let custom_port_forwarding: Option<VpnProvider> = command
+        .custom_port_forwarding
+        .map(|x| x.to_variant())
+        .or_else(|| {
+            vopono_config_settings
+                .get("custom_port_forwarding")
+                .map_err(|_e| anyhow!("Failed to read config file"))
+                .ok()
+        });
+    if custom_port_forwarding.is_some() && custom_config.is_none() {
+        warn!("Custom port forwarding implementation is set, but not using custom provider config file. custom-port-forwarding setting will be ignored");
+    }
+
     // Create netns only
     let create_netns_only = if !command.create_netns_only {
         vopono_config_settings
@@ -338,18 +354,21 @@ pub fn exec(command: ExecCommand, uiclient: &dyn UiClient) -> anyhow::Result<()>
         }?;
         Some(get_config_from_alias(&cdir, &server_name)?)
     } else {
-        Some(custom_config.expect("No custom config provided"))
+        Some(custom_config.clone().expect("No custom config provided"))
     };
 
     // Better to check for lockfile exists?
+    let using_existing_netns;
     if get_existing_namespaces()?.contains(&ns_name) {
         // If namespace exists, read its lock config
         info!(
             "Using existing namespace: {}, will not modify firewall rules",
             &ns_name
         );
         ns = NetworkNamespace::from_existing(ns_name)?;
+        using_existing_netns = true;
     } else {
+        using_existing_netns = false;
         ns = NetworkNamespace::new(
             ns_name.clone(),
             provider.clone(),
@@ -556,15 +575,36 @@ pub fn exec(command: ExecCommand, uiclient: &dyn UiClient) -> anyhow::Result<()>
 
     let ns = ns.write_lockfile(&command.application)?;
 
-    let forwarder: Option<Box<dyn Forwarder>> = if port_forwarding {
+    //  Does not re-run if re-using existing namespace
+    if using_existing_netns && (port_forwarding || custom_port_forwarding.is_some()) {
+        warn!("Re-using existing network namespace {} - will not run port forwarder, should be run when netns first created", &ns.name);
+    }
+    let forwarder: Option<Box<dyn Forwarder>> = if (port_forwarding
+        || custom_port_forwarding.is_some())
+        && !using_existing_netns
+    {
+        let provider_or_custom = if custom_config.is_some() {
+            custom_port_forwarding
+        } else {
+            Some(provider)
+        };
+
+        if provider_or_custom.is_some() {
+            debug!(
+                "Will use {:?} as provider for port forwarding",
+                &provider_or_custom
+            );
+        }
+
         let callback = command.port_forwarding_callback.or_else(|| {
             vopono_config_settings
                 .get("port_forwarding_callback")
                 .map_err(|_e| anyhow!("Failed to read config file"))
                 .ok()
         });
-        match provider {
-            VpnProvider::PrivateInternetAccess => {
+
+        match provider_or_custom {
+            Some(VpnProvider::PrivateInternetAccess) => {
                 let conf_path = config_file.expect("No PIA config file provided");
                 let conf_name = conf_path
                     .file_name()
@@ -579,16 +619,21 @@ pub fn exec(command: ExecCommand, uiclient: &dyn UiClient) -> anyhow::Result<()>
                     callback.as_ref(),
                 )?))
             }
-            VpnProvider::ProtonVPN => {
+            Some(VpnProvider::ProtonVPN) => {
                 vopono_core::util::open_hosts(
                     &ns,
                     vec![vopono_core::network::natpmpc::PROTONVPN_GATEWAY],
                     firewall,
                 )?;
-                Some(Box::new(Natpmpc::new(&ns)?))
+                Some(Box::new(Natpmpc::new(&ns, callback.as_ref())?))
+            }
+            Some(p) => {
+                warn!("Port forwarding not supported for the selected provider: {} - ignoring --port-forwarding", p);
+                None
             }
-            _ => {
-                anyhow::bail!("Port forwarding not supported for the selected provider");
+            None => {
+                warn!("--port-forwarding set but --custom-port-forwarding provider not provided for --custom-config usage. Ignoring --port-forwarding");
+                None
             }
         }
     } else {

vopono_core/Cargo.toml

@@ -16,7 +16,7 @@ directories-next = "2"
 log = "0.4"
 which = "6"
 users = "0.11"
-nix = { version = "0.27", features = ["user", "signal", "fs", "process"] }
+nix = { version = "0.28", features = ["user", "signal", "fs", "process"] }
 serde = { version = "1", features = ["derive", "std"] }
 csv = "1"
 regex = "1"
@@ -33,8 +33,8 @@ reqwest = { default-features = false, version = "0.11", features = [
 sysinfo = "0.30"
 base64 = "0.21"
 x25519-dalek = { version = "2", features = ["static_secrets"] }
-strum = "0.25"
-strum_macros = "0.25"
+strum = "0.26"
+strum_macros = "0.26"
 zip = "0.6"
 maplit = "1"
 webbrowser = "0.8"

vopono_core/src/network/natpmpc.rs

@@ -20,8 +20,13 @@ pub struct Natpmpc {
     send_channel: Sender<bool>,
 }
 
+struct ThreadParams {
+    pub netns_name: String,
+    pub callback: Option<String>,
+}
+
 impl Natpmpc {
-    pub fn new(ns: &NetworkNamespace) -> anyhow::Result<Self> {
+    pub fn new(ns: &NetworkNamespace, callback: Option<&String>) -> anyhow::Result<Self> {
         let gateway_str = PROTONVPN_GATEWAY.to_string();
 
         if let Err(x) = which::which("natpmpc") {
@@ -44,12 +49,15 @@ impl Natpmpc {
             anyhow::bail!("natpmpc failed - likely that this server does not support port forwarding, please choose another server")
         }
 
-        let port = Self::refresh_port(&ns.name)?;
+        let params = ThreadParams {
+            netns_name: ns.name.clone(),
+            callback: callback.cloned(),
+        };
+        let port = Self::refresh_port(&params)?;
 
         let (send, recv) = mpsc::channel::<bool>();
 
-        let ns_name = ns.name.clone();
-        let handle = std::thread::spawn(move || Self::thread_loop(ns_name, recv));
+        let handle = std::thread::spawn(move || Self::thread_loop(params, recv));
 
         log::info!("ProtonVPN forwarded local port: {port}");
         Ok(Self {
@@ -59,13 +67,14 @@ impl Natpmpc {
         })
     }
 
-    fn refresh_port(ns_name: &str) -> anyhow::Result<u16> {
+    // TODO: Refactor these two methods into Trait shared with piapf.rs
+    fn refresh_port(params: &ThreadParams) -> anyhow::Result<u16> {
         let gateway_str = PROTONVPN_GATEWAY.to_string();
         // TODO: Cache regex
         let re = Regex::new(r"Mapped public port (?P<port>\d{1,5}) protocol").unwrap();
         // Read Mapped public port 61057 protocol UDP
         let udp_output = NetworkNamespace::exec_with_output(
-            ns_name,
+            &params.netns_name,
             &["natpmpc", "-a", "1", "0", "udp", "60", "-g", &gateway_str],
         )?;
         let udp_port: u16 = re
@@ -77,7 +86,7 @@ impl Natpmpc {
             .parse()?;
         // Mapped public port 61057 protocol TCP
         let tcp_output = NetworkNamespace::exec_with_output(
-            ns_name,
+            &params.netns_name,
             &["natpmpc", "-a", "1", "0", "tcp", "60", "-g", &gateway_str],
         )?;
         let tcp_port: u16 = re
@@ -94,18 +103,35 @@ impl Natpmpc {
             )
         }
 
+        if let Some(cb) = &params.callback {
+            let refresh_response = NetworkNamespace::exec_with_output(
+                &params.netns_name,
+                &[cb, &udp_port.to_string()],
+            )?;
+            if !refresh_response.status.success() {
+                log::error!(
+                    "Port forwarding callback script was unsuccessful!: stdout: {:?}, stderr: {:?}, exit code: {}",
+                    String::from_utf8(refresh_response.stdout),
+                    String::from_utf8(refresh_response.stderr),
+                    refresh_response.status
+                );
+            } else if let Ok(out) = String::from_utf8(refresh_response.stdout) {
+                println!("{}", out);
+            }
+        }
+
         Ok(udp_port)
     }
 
     // Spawn thread to repeat above every 45 seconds
-    fn thread_loop(netns_name: String, recv: Receiver<bool>) {
+    fn thread_loop(params: ThreadParams, recv: Receiver<bool>) {
         loop {
             let resp = recv.recv_timeout(std::time::Duration::from_secs(45));
             if resp.is_ok() {
                 log::debug!("Thread exiting...");
                 return;
             } else {
-                let port = Self::refresh_port(&netns_name);
+                let port = Self::refresh_port(&params);
                 match port {
                     Err(e) => {
                         log::error!("Thread failed to refresh port: {e:?}");

vopono_core/src/network/piapf.rs

@@ -169,6 +169,7 @@ impl Piapf {
         })
     }
 
+    // TODO: Refactor methods below into Trait
     fn refresh_port(params: &ThreadParams) -> anyhow::Result<u16> {
         let bind_response = NetworkNamespace::exec_with_output(
             &params.netns_name,
@@ -203,10 +204,17 @@ impl Piapf {
         if let Some(cb) = &params.callback {
             let refresh_response = NetworkNamespace::exec_with_output(
                 &params.netns_name,
-                &[&cb, &params.port.to_string()],
+                &[cb, &params.port.to_string()],
             )?;
             if !refresh_response.status.success() {
-                log::info!("Callback script was unsuccessful!");
+                log::error!(
+                    "Port forwarding callback script was unsuccessful!: stdout: {:?}, stderr: {:?}, exit code: {}",
+                    String::from_utf8(refresh_response.stdout),
+                    String::from_utf8(refresh_response.stderr),
+                    refresh_response.status
+                );
+            } else if let Ok(out) = String::from_utf8(refresh_response.stdout) {
+                println!("{}", out);
             }
         }