Merge pull request #92 from jamesmcm/natfix

Fix case where NAT rules undone prematurely
jamesmcm · Jun 5, 2021 · 5ae5210 · 5ae5210
2 parents 46775dd + cf19aa3
commit 5ae5210
Show file tree

Hide file tree

Showing 11 changed files with 81 additions and 61 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 name = "vopono"
 description = "Launch applications via VPN tunnels using temporary network namespaces"
-version = "0.8.1"
+version = "0.8.2"
 authors = ["James McMurray <jamesmcm03@gmail.com>"]
 edition = "2018"
 license = "GPL-3.0-or-later"
@@ -37,8 +37,8 @@ reqwest = {default-features = false, version = "0.11", features = ["blocking", "
 sysinfo = "0.18"
 base64 = "0.13"
 x25519-dalek = "1"
-strum = "0.20"
-strum_macros = "0.20"
+strum = "0.21"
+strum_macros = "0.21"
 zip = "0.5"
 maplit = "1"
 webbrowser = "0.5"

diff --git a/src/dns_config.rs b/src/dns_config.rs
@@ -4,7 +4,7 @@ use serde::{Deserialize, Serialize};
 use std::io::Write;
 use std::net::IpAddr;
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct DnsConfig {
     ns_name: String,
 }

diff --git a/src/host_masquerade.rs b/src/host_masquerade.rs
@@ -2,9 +2,10 @@ use super::firewall::Firewall;
 use super::network_interface::NetworkInterface;
 use super::util::sudo_command;
 use anyhow::Context;
+use log::debug;
 use serde::{Deserialize, Serialize};
 
-#[derive(Serialize, Deserialize, Clone)]
+#[derive(Serialize, Deserialize, Clone, Debug)]
 pub struct HostMasquerade {
     ip_mask: String,
     interface: NetworkInterface,
@@ -80,43 +81,48 @@ impl HostMasquerade {
 
 impl Drop for HostMasquerade {
     fn drop(&mut self) {
-        match self.firewall {
-            Firewall::IpTables => {
-                sudo_command(&[
-                    "iptables",
-                    "-t",
-                    "nat",
-                    "-D",
-                    "POSTROUTING",
-                    "-s",
-                    &self.ip_mask,
-                    "-o",
-                    &self.interface.name,
-                    "-j",
-                    "MASQUERADE",
-                ])
-                .unwrap_or_else(|_| {
-                    panic!(
-                        "Failed to delete iptables masquerade rule, ip_mask: {}, interface: {}",
-                        &self.ip_mask, &self.interface.name
-                    )
-                });
-            }
-            Firewall::NfTables => {
-                sudo_command(&["nft", "delete", "table", "inet", "vopono_nat"]).unwrap_or_else(
-                    |_| {
+        // Only drop these settings if there are no other active namespaces
+        let namespaces = crate::list::get_lock_namespaces();
+        debug!("Remaining namespaces: {:?}", namespaces);
+        if namespaces.is_ok() && namespaces.unwrap().is_empty() {
+            match self.firewall {
+                Firewall::IpTables => {
+                    sudo_command(&[
+                        "iptables",
+                        "-t",
+                        "nat",
+                        "-D",
+                        "POSTROUTING",
+                        "-s",
+                        &self.ip_mask,
+                        "-o",
+                        &self.interface.name,
+                        "-j",
+                        "MASQUERADE",
+                    ])
+                    .unwrap_or_else(|_| {
                         panic!(
+                            "Failed to delete iptables masquerade rule, ip_mask: {}, interface: {}",
+                            &self.ip_mask, &self.interface.name
+                        )
+                    });
+                }
+                Firewall::NfTables => {
+                    sudo_command(&["nft", "delete", "table", "inet", "vopono_nat"]).unwrap_or_else(
+                        |_| {
+                            panic!(
                             "Failed to delete nftables masquerade rule, ip_mask: {}, interface: {}",
                             &self.ip_mask, &self.interface.name
                         )
-                    },
-                );
+                        },
+                    );
+                }
             }
         }
     }
 }
 
-#[derive(Serialize, Deserialize, Clone)]
+#[derive(Serialize, Deserialize, Clone, Debug)]
 pub struct FirewallException {
     host_interface: NetworkInterface,
     ns_interface: NetworkInterface,
@@ -231,9 +237,13 @@ impl FirewallException {
 
 impl Drop for FirewallException {
     fn drop(&mut self) {
-        match self.firewall {
-            Firewall::IpTables => {
-                sudo_command(&[
+        // Only drop these settings if there are no other active namespaces
+        let namespaces = crate::list::get_lock_namespaces();
+        debug!("Remaining namespaces: {:?}", namespaces);
+        if namespaces.is_ok() && namespaces.unwrap().is_empty() {
+            match self.firewall {
+                Firewall::IpTables => {
+                    sudo_command(&[
                     "iptables",
                     "-D",
                     "FORWARD",
@@ -251,7 +261,7 @@ impl Drop for FirewallException {
                     )
                 });
 
-                sudo_command(&[
+                    sudo_command(&[
                     "iptables",
                     "-D",
                     "FORWARD",
@@ -268,16 +278,17 @@ impl Drop for FirewallException {
                         &self.host_interface.name, &self.ns_interface.name
                     )
                 });
-            }
-            Firewall::NfTables => {
-                sudo_command(&["nft", "delete", "table", "inet", "vopono_bridge"]).unwrap_or_else(
+                }
+                Firewall::NfTables => {
+                    sudo_command(&["nft", "delete", "table", "inet", "vopono_bridge"]).unwrap_or_else(
                     |_| {
                         panic!(
                             "Failed to delete nftables namespace bridge firewall rule, host interface: {}, namespace interface: {}",
                             &self.host_interface.name, &self.ns_interface.name
                         )
                     },
                 );
+                }
             }
         }
     }

diff --git a/src/netns.rs b/src/netns.rs
@@ -22,7 +22,7 @@ use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
 use std::time::{SystemTime, UNIX_EPOCH};
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct NetworkNamespace {
     pub name: String,
     pub veth_pair: Option<VethPair>,
@@ -444,6 +444,10 @@ impl Drop for NetworkNamespace {
                 .unwrap_or_else(|_| panic!("Failed to delete network namespace: {}", &self.name));
         } else {
             debug!("Skipping destructors since other vopono instance using this namespace!");
+            debug!(
+                "Existing lockfiles using this namespace: {:?}",
+                lockfile_path.read_dir().unwrap().collect::<Vec<_>>()
+            );
             std::mem::forget(self.openvpn.take());
             std::mem::forget(self.veth_pair.take());
             std::mem::forget(self.dns_config.take());
@@ -455,7 +459,7 @@ impl Drop for NetworkNamespace {
     }
 }
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct Lockfile {
     pub ns: NetworkNamespace,
     pub start: u64,

diff --git a/src/network_interface.rs b/src/network_interface.rs
@@ -4,7 +4,7 @@ use serde::{Deserialize, Serialize};
 use std::process::Command;
 use std::str::FromStr;
 
-#[derive(Clone, Serialize, Deserialize)]
+#[derive(Clone, Serialize, Deserialize, Debug)]
 pub struct NetworkInterface {
     pub name: String,
 }

diff --git a/src/openconnect.rs b/src/openconnect.rs
@@ -6,7 +6,7 @@ use log::{debug, error, info};
 use serde::{Deserialize, Serialize};
 use std::path::{Path, PathBuf};
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct OpenConnect {
     pid: u32,
 }

diff --git a/src/openfortivpn.rs b/src/openfortivpn.rs
@@ -9,7 +9,7 @@ use std::net::{IpAddr, Ipv4Addr};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct OpenFortiVpn {
     pid: u32,
 }

diff --git a/src/openvpn.rs b/src/openvpn.rs
@@ -12,7 +12,7 @@ use std::net::IpAddr;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct OpenVpn {
     pid: u32,
     pub openvpn_dns: Option<IpAddr>,

diff --git a/src/shadowsocks.rs b/src/shadowsocks.rs
@@ -22,7 +22,7 @@ use std::net::{IpAddr, Ipv4Addr};
 use std::path::Path;
 use std::str::FromStr;
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct Shadowsocks {
     pid: u32,
 }

diff --git a/src/veth_pair.rs b/src/veth_pair.rs
@@ -8,14 +8,14 @@ use std::io::Write;
 use std::path::PathBuf;
 use std::str::FromStr;
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct VethPair {
     pub source: String,
     pub dest: String,
     pub nm_unmanaged: Option<NetworkManagerUnmanaged>,
 }
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct NetworkManagerUnmanaged {
     pub backup_file: Option<PathBuf>,
 }
@@ -123,17 +123,22 @@ impl Drop for VethPair {
 
 impl Drop for NetworkManagerUnmanaged {
     fn drop(&mut self) {
-        let nm_path = PathBuf::from_str("/etc/NetworkManager/conf.d/unmanaged.conf")
-            .expect("Failed to build path");
-        if self.backup_file.is_some() {
-            std::fs::copy(self.backup_file.as_ref().unwrap(), &nm_path)
-                .expect("Failed to restore backup of NetworkManager unmanaged.conf");
-            std::fs::remove_file(self.backup_file.as_ref().unwrap())
-                .expect("Failed to delete backup of NetworkManager unmanaged.conf");
-        } else {
-            std::fs::remove_file(&nm_path).expect("Failed to delete NetworkManager unmanaged.conf");
+        // Only restore settings if there are no other active namespaces
+        let namespaces = crate::list::get_lock_namespaces();
+        if namespaces.is_ok() && namespaces.unwrap().is_empty() {
+            let nm_path = PathBuf::from_str("/etc/NetworkManager/conf.d/unmanaged.conf")
+                .expect("Failed to build path");
+            if self.backup_file.is_some() {
+                std::fs::copy(self.backup_file.as_ref().unwrap(), &nm_path)
+                    .expect("Failed to restore backup of NetworkManager unmanaged.conf");
+                std::fs::remove_file(self.backup_file.as_ref().unwrap())
+                    .expect("Failed to delete backup of NetworkManager unmanaged.conf");
+            } else {
+                std::fs::remove_file(&nm_path)
+                    .expect("Failed to delete NetworkManager unmanaged.conf");
+            }
+            sudo_command(&["nmcli", "connection", "reload"])
+                .expect("Failed to reload NetworkManager configuration");
         }
-        sudo_command(&["nmcli", "connection", "reload"])
-            .expect("Failed to reload NetworkManager configuration");
     }
 }
diff --git a/src/wireguard.rs b/src/wireguard.rs
@@ -12,7 +12,7 @@ use std::net::SocketAddr;
 use std::net::ToSocketAddrs;
 use std::path::PathBuf;
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Debug)]
 pub struct Wireguard {
     ns_name: String,
     config_file: PathBuf,