From 9d20e554bdf4dfeca6e059d4713fe9fd9534d3b4 Mon Sep 17 00:00:00 2001
From: James McMurray <jamesmcm03@gmail.com>
Date: Sun, 3 Mar 2024 12:59:54 +0100
Subject: [PATCH] Improve OpenVPN logging when verbose

---
 src/exec.rs                        |  7 ++++---
 src/main.rs                        |  2 +-
 vopono_core/Cargo.toml             |  2 +-
 vopono_core/src/network/netns.rs   |  2 ++
 vopono_core/src/network/openvpn.rs | 22 +++++++++++++++++++---
 5 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/src/exec.rs b/src/exec.rs
index f818859..fa24ae7 100644
--- a/src/exec.rs
+++ b/src/exec.rs
@@ -25,7 +25,7 @@ use vopono_core::network::sysctl::SysCtl;
 use vopono_core::util::vopono_dir;
 use vopono_core::util::{get_config_from_alias, get_existing_namespaces, get_target_subnet};
 
-pub fn exec(command: ExecCommand, uiclient: &dyn UiClient) -> anyhow::Result<()> {
+pub fn exec(command: ExecCommand, uiclient: &dyn UiClient, verbose: bool) -> anyhow::Result<()> {
     // this captures all sigint signals
     // ignore for now, they are automatically passed on to the child
     let signals = Signals::new([SIGINT])?;
@@ -146,8 +146,7 @@ pub fn exec(command: ExecCommand, uiclient: &dyn UiClient) -> anyhow::Result<()>
         )?;
         _sysctl = SysCtl::enable_ipv4_forwarding();
 
-        // TODO: Skip this if netns config only
-        let config_file = run_protocol_in_netns(&parsed_command, &mut ns, uiclient)?;
+        let config_file = run_protocol_in_netns(&parsed_command, &mut ns, uiclient, verbose)?;
         ns.set_config_file(config_file);
 
         if let Some(ref hosts) = parsed_command.open_hosts {
@@ -279,6 +278,7 @@ fn run_protocol_in_netns(
     parsed_command: &ArgsConfig,
     ns: &mut NetworkNamespace,
     uiclient: &dyn UiClient,
+    verbose: bool,
 ) -> anyhow::Result<Option<PathBuf>> {
     if parsed_command.provider == VpnProvider::None {
         log::warn!(
@@ -386,6 +386,7 @@ fn run_protocol_in_netns(
                 parsed_command.forward.as_ref(),
                 parsed_command.firewall,
                 parsed_command.disable_ipv6,
+                verbose,
             )?;
             debug!(
                 "Checking that OpenVPN is running in namespace: {}",
diff --git a/src/main.rs b/src/main.rs
index e3e9d36..839381d 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -53,7 +53,7 @@ fn main() -> anyhow::Result<()> {
             }
             elevate_privileges(app.askpass)?;
             clean_dead_namespaces()?;
-            exec::exec(cmd, &uiclient)?
+            exec::exec(cmd, &uiclient, app.verbose)?
         }
         args::Command::List(listcmd) => {
             clean_dead_locks()?;
diff --git a/vopono_core/Cargo.toml b/vopono_core/Cargo.toml
index 07cbaa4..7abe156 100644
--- a/vopono_core/Cargo.toml
+++ b/vopono_core/Cargo.toml
@@ -31,7 +31,7 @@ reqwest = { default-features = false, version = "0.11", features = [
     "rustls-tls",
 ] } # TODO: Can we remove Tokio dependency?
 sysinfo = "0.30"
-base64 = "0.21"
+base64 = "0.22"
 x25519-dalek = { version = "2", features = ["static_secrets"] }
 strum = "0.26"
 strum_macros = "0.26"
diff --git a/vopono_core/src/network/netns.rs b/vopono_core/src/network/netns.rs
index 0e5343a..bdad951 100644
--- a/vopono_core/src/network/netns.rs
+++ b/vopono_core/src/network/netns.rs
@@ -324,6 +324,7 @@ impl NetworkNamespace {
         forward_ports: Option<&Vec<u16>>,
         firewall: Firewall,
         disable_ipv6: bool,
+        verbose: bool,
     ) -> anyhow::Result<()> {
         self.openvpn = Some(OpenVpn::run(
             self,
@@ -335,6 +336,7 @@ impl NetworkNamespace {
             forward_ports,
             firewall,
             disable_ipv6,
+            verbose,
         )?);
         Ok(())
     }
diff --git a/vopono_core/src/network/openvpn.rs b/vopono_core/src/network/openvpn.rs
index 1d67df8..2772a24 100644
--- a/vopono_core/src/network/openvpn.rs
+++ b/vopono_core/src/network/openvpn.rs
@@ -31,6 +31,7 @@ impl OpenVpn {
         forward_ports: Option<&Vec<u16>>,
         firewall: Firewall,
         disable_ipv6: bool,
+        verbose: bool,
     ) -> anyhow::Result<Self> {
         // TODO: Refactor this to separate functions
         // TODO: --status flag
@@ -50,10 +51,12 @@ impl OpenVpn {
             File::create(&log_file_str)?;
         }
 
-        // TODO: Check config file for up and down script entries and warn on their presence
-
         let config_file_path = config_file.canonicalize().context("Invalid path given")?;
         set_config_permissions()?;
+
+        // Check config file for up and down script entries and warn on their presence
+        warn_on_scripts_config(&config_file_path)?;
+
         info!("Launching OpenVPN...");
         let mut command_vec = ([
             "openvpn",
@@ -107,7 +110,7 @@ impl OpenVpn {
             &command_vec,
             None,
             None,
-            true,
+            !verbose,
             false,
             false,
             Some(working_dir),
@@ -576,6 +579,19 @@ pub fn killswitch(
     Ok(())
 }
 
+pub fn warn_on_scripts_config(path: &Path) -> anyhow::Result<bool> {
+    let mut out = false;
+    let file_string =
+        std::fs::read_to_string(path).context(format!("Reading OpenVPN config file: {path:?}"))?;
+    for line in file_string.lines() {
+        if line.trim().starts_with("up ") || line.trim().starts_with("down ") {
+            log::error!("up / down scripts detected in OpenVPN config file - remove these or OpenVPN will likely hang in the network namespace. Line: {}", line);
+            out = true;
+        }
+    }
+    Ok(out)
+}
+
 pub fn get_remotes_from_config(path: &Path) -> anyhow::Result<Vec<Remote>> {
     let file_string =
         std::fs::read_to_string(path).context(format!("Reading OpenVPN config file: {path:?}"))?;