firewall: improve error handling of firewall

Error handling of the firewall binary should now be much more robust
on configuration errors. Instead of panicking in some cases it should
now log an error.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>

proxmox-firewall/src/bin/proxmox-firewall.rs

@@ -5,6 +5,7 @@ use std::time::{Duration, Instant};
 
 use anyhow::{Context, Error};
 
+use proxmox_firewall::config::{FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader};
 use proxmox_firewall::firewall::Firewall;
 use proxmox_nftables::{client::NftError, NftClient};
 
@@ -24,7 +25,9 @@ fn remove_firewall() -> Result<(), std::io::Error> {
 }
 
 fn handle_firewall() -> Result<(), Error> {
-    let firewall = Firewall::new();
+    let config = FirewallConfig::new(&PveFirewallConfigLoader::new(), &PveNftConfigLoader::new())?;
+
+    let firewall = Firewall::new(config);
 
     if !firewall.is_enabled() {
         return remove_firewall().with_context(|| "could not remove firewall tables".to_string());
@@ -84,7 +87,7 @@ fn main() -> Result<(), std::io::Error> {
         let start = Instant::now();
 
         if let Err(error) = handle_firewall() {
-            log::error!("error creating firewall rules: {error}");
+            log::error!("error updating firewall rules: {error}");
         }
 
         let duration = start.elapsed();

proxmox-firewall/src/config.rs

@@ -2,9 +2,8 @@ use std::collections::BTreeMap;
 use std::default::Default;
 use std::fs::File;
 use std::io::{self, BufReader};
-use std::sync::OnceLock;
 
-use anyhow::Error;
+use anyhow::{format_err, Context, Error};
 
 use proxmox_ve_config::firewall::cluster::Config as ClusterConfig;
 use proxmox_ve_config::firewall::guest::Config as GuestConfig;
@@ -19,15 +18,19 @@ use proxmox_nftables::types::ListChain;
 use proxmox_nftables::NftClient;
 
 pub trait FirewallConfigLoader {
-    fn cluster(&self) -> Option<Box<dyn io::BufRead>>;
-    fn host(&self) -> Option<Box<dyn io::BufRead>>;
-    fn guest_list(&self) -> GuestMap;
-    fn guest_config(&self, vmid: &Vmid, guest: &GuestEntry) -> Option<Box<dyn io::BufRead>>;
-    fn guest_firewall_config(&self, vmid: &Vmid) -> Option<Box<dyn io::BufRead>>;
+    fn cluster(&self) -> Result<Option<Box<dyn io::BufRead>>, Error>;
+    fn host(&self) -> Result<Option<Box<dyn io::BufRead>>, Error>;
+    fn guest_list(&self) -> Result<GuestMap, Error>;
+    fn guest_config(
+        &self,
+        vmid: &Vmid,
+        guest: &GuestEntry,
+    ) -> Result<Option<Box<dyn io::BufRead>>, Error>;
+    fn guest_firewall_config(&self, vmid: &Vmid) -> Result<Option<Box<dyn io::BufRead>>, Error>;
 }
 
 #[derive(Default)]
-struct PveFirewallConfigLoader {}
+pub struct PveFirewallConfigLoader {}
 
 impl PveFirewallConfigLoader {
     pub fn new() -> Self {
@@ -56,69 +59,70 @@ const CLUSTER_CONFIG_PATH: &str = "/etc/pve/firewall/cluster.fw";
 const HOST_CONFIG_PATH: &str = "/etc/pve/local/host.fw";
 
 impl FirewallConfigLoader for PveFirewallConfigLoader {
-    fn cluster(&self) -> Option<Box<dyn io::BufRead>> {
+    fn cluster(&self) -> Result<Option<Box<dyn io::BufRead>>, Error> {
         log::info!("loading cluster config");
 
-        let fd =
-            open_config_file(CLUSTER_CONFIG_PATH).expect("able to read cluster firewall config");
+        let fd = open_config_file(CLUSTER_CONFIG_PATH)?;
 
         if let Some(file) = fd {
             let buf_reader = Box::new(BufReader::new(file)) as Box<dyn io::BufRead>;
-            return Some(buf_reader);
+            return Ok(Some(buf_reader));
         }
 
-        None
+        Ok(None)
     }
 
-    fn host(&self) -> Option<Box<dyn io::BufRead>> {
+    fn host(&self) -> Result<Option<Box<dyn io::BufRead>>, Error> {
         log::info!("loading host config");
 
-        let fd = open_config_file(HOST_CONFIG_PATH).expect("able to read host firewall config");
+        let fd = open_config_file(HOST_CONFIG_PATH)?;
 
         if let Some(file) = fd {
             let buf_reader = Box::new(BufReader::new(file)) as Box<dyn io::BufRead>;
-            return Some(buf_reader);
+            return Ok(Some(buf_reader));
         }
 
-        None
+        Ok(None)
     }
 
-    fn guest_list(&self) -> GuestMap {
+    fn guest_list(&self) -> Result<GuestMap, Error> {
         log::info!("loading vmlist");
-        GuestMap::new().expect("able to read vmlist")
+        GuestMap::new()
     }
 
-    fn guest_config(&self, vmid: &Vmid, entry: &GuestEntry) -> Option<Box<dyn io::BufRead>> {
+    fn guest_config(
+        &self,
+        vmid: &Vmid,
+        entry: &GuestEntry,
+    ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
         log::info!("loading guest #{vmid} config");
 
-        let fd = open_config_file(&GuestMap::config_path(vmid, entry))
-            .expect("able to read guest config");
+        let fd = open_config_file(&GuestMap::config_path(vmid, entry))?;
 
         if let Some(file) = fd {
             let buf_reader = Box::new(BufReader::new(file)) as Box<dyn io::BufRead>;
-            return Some(buf_reader);
+            return Ok(Some(buf_reader));
         }
 
-        None
+        Ok(None)
     }
 
-    fn guest_firewall_config(&self, vmid: &Vmid) -> Option<Box<dyn io::BufRead>> {
+    fn guest_firewall_config(&self, vmid: &Vmid) -> Result<Option<Box<dyn io::BufRead>>, Error> {
         log::info!("loading guest #{vmid} firewall config");
 
-        let fd = open_config_file(&GuestMap::firewall_config_path(vmid))
-            .expect("able to read guest firewall config");
+        let fd = open_config_file(&GuestMap::firewall_config_path(vmid))?;
 
         if let Some(file) = fd {
             let buf_reader = Box::new(BufReader::new(file)) as Box<dyn io::BufRead>;
-            return Some(buf_reader);
+            return Ok(Some(buf_reader));
         }
 
-        None
+        Ok(None)
     }
 }
 
 pub trait NftConfigLoader {
-    fn chains(&self) -> CommandOutput;
+    fn chains(&self) -> Result<Option<CommandOutput>, Error>;
 }
 
 #[derive(Debug, Default)]
@@ -131,131 +135,122 @@ impl PveNftConfigLoader {
 }
 
 impl NftConfigLoader for PveNftConfigLoader {
-    fn chains(&self) -> CommandOutput {
+    fn chains(&self) -> Result<Option<CommandOutput>, Error> {
         log::info!("querying nftables config for chains");
 
         let commands = Commands::new(vec![List::chains()]);
 
         NftClient::run_json_commands(&commands)
-            .expect("can query chains in nftables")
-            .expect("nft returned output")
+            .with_context(|| "unable to query nft chains".to_string())
     }
 }
 
 pub struct FirewallConfig {
-    firewall_loader: Box<dyn FirewallConfigLoader>,
-    nft_loader: Box<dyn NftConfigLoader>,
-    cluster_config: OnceLock<ClusterConfig>,
-    host_config: OnceLock<HostConfig>,
-    guest_config: OnceLock<BTreeMap<Vmid, GuestConfig>>,
-    nft_config: OnceLock<BTreeMap<String, ListChain>>,
-}
-
-impl Default for FirewallConfig {
-    fn default() -> Self {
-        Self {
-            firewall_loader: Box::new(PveFirewallConfigLoader::new()),
-            nft_loader: Box::new(PveNftConfigLoader::new()),
-            cluster_config: OnceLock::new(),
-            host_config: OnceLock::new(),
-            guest_config: OnceLock::new(),
-            nft_config: OnceLock::new(),
-        }
-    }
+    cluster_config: ClusterConfig,
+    host_config: HostConfig,
+    guest_config: BTreeMap<Vmid, GuestConfig>,
+    nft_config: BTreeMap<String, ListChain>,
 }
 
 impl FirewallConfig {
-    pub fn new(
-        firewall_loader: Box<dyn FirewallConfigLoader>,
-        nft_loader: Box<dyn NftConfigLoader>,
-    ) -> Self {
-        Self {
-            firewall_loader,
-            nft_loader,
-            cluster_config: OnceLock::new(),
-            host_config: OnceLock::new(),
-            guest_config: OnceLock::new(),
-            nft_config: OnceLock::new(),
+    fn parse_cluster(firewall_loader: &dyn FirewallConfigLoader) -> Result<ClusterConfig, Error> {
+        match firewall_loader.cluster()? {
+            Some(data) => ClusterConfig::parse(data),
+            None => {
+                log::info!("no cluster config found, falling back to default");
+                Ok(ClusterConfig::default())
+            }
         }
     }
 
-    pub fn cluster(&self) -> &ClusterConfig {
-        self.cluster_config.get_or_init(|| {
-            let raw_config = self.firewall_loader.cluster();
-
-            match raw_config {
-                Some(data) => ClusterConfig::parse(data).expect("cluster firewall config is valid"),
-                None => {
-                    log::info!("no cluster config found, falling back to default");
-                    ClusterConfig::default()
-                }
+    fn parse_host(firewall_loader: &dyn FirewallConfigLoader) -> Result<HostConfig, Error> {
+        match firewall_loader.host()? {
+            Some(data) => HostConfig::parse(data),
+            None => {
+                log::info!("no host config found, falling back to default");
+                Ok(HostConfig::default())
             }
+        }
+    }
+
+    pub fn parse_guests(
+        firewall_loader: &dyn FirewallConfigLoader,
+    ) -> Result<BTreeMap<Vmid, GuestConfig>, Error> {
+        let mut guests = BTreeMap::new();
+
+        for (vmid, entry) in firewall_loader.guest_list()?.iter() {
+            if !entry.is_local() {
+                log::debug!("guest #{vmid} is not local, skipping");
+                continue;
+            }
+
+            let raw_firewall_config = firewall_loader.guest_firewall_config(vmid)?;
+
+            if let Some(raw_firewall_config) = raw_firewall_config {
+                log::debug!("found firewall config for #{vmid}, loading guest config");
+
+                let raw_config = firewall_loader
+                    .guest_config(vmid, entry)?
+                    .ok_or_else(|| format_err!("could not load guest config for #{vmid}"))?;
+
+                let config = GuestConfig::parse(
+                    vmid,
+                    entry.ty().iface_prefix(),
+                    raw_firewall_config,
+                    raw_config,
+                )?;
+
+                guests.insert(*vmid, config);
+            }
+        }
+
+        Ok(guests)
+    }
+
+    pub fn parse_nft(
+        nft_loader: &dyn NftConfigLoader,
+    ) -> Result<BTreeMap<String, ListChain>, Error> {
+        let output = nft_loader
+            .chains()?
+            .ok_or_else(|| format_err!("no command output from nft query"))?;
+
+        let mut chains = BTreeMap::new();
+
+        for element in &output.nftables {
+            if let ListOutput::Chain(chain) = element {
+                chains.insert(chain.name().to_owned(), chain.clone());
+            }
+        }
+
+        Ok(chains)
+    }
+
+    pub fn new(
+        firewall_loader: &dyn FirewallConfigLoader,
+        nft_loader: &dyn NftConfigLoader,
+    ) -> Result<Self, Error> {
+        Ok(Self {
+            cluster_config: Self::parse_cluster(firewall_loader)?,
+            host_config: Self::parse_host(firewall_loader)?,
+            guest_config: Self::parse_guests(firewall_loader)?,
+            nft_config: Self::parse_nft(nft_loader)?,
         })
     }
 
+    pub fn cluster(&self) -> &ClusterConfig {
+        &self.cluster_config
+    }
+
     pub fn host(&self) -> &HostConfig {
-        self.host_config.get_or_init(|| {
-            let raw_config = self.firewall_loader.host();
-
-            match raw_config {
-                Some(data) => HostConfig::parse(data).expect("host firewall config is valid"),
-                None => {
-                    log::info!("no host config found, falling back to default");
-                    HostConfig::default()
-                }
-            }
-        })
+        &self.host_config
     }
 
     pub fn guests(&self) -> &BTreeMap<Vmid, GuestConfig> {
-        self.guest_config.get_or_init(|| {
-            let mut guests = BTreeMap::new();
-
-            for (vmid, entry) in self.firewall_loader.guest_list().iter() {
-                if !entry.is_local() {
-                    log::debug!("guest #{vmid} is not local, skipping");
-                    continue;
-                }
-
-                let raw_firewall_config = self.firewall_loader.guest_firewall_config(vmid);
-
-                if let Some(raw_firewall_config) = raw_firewall_config {
-                    log::debug!("found firewall config for #{vmid}, loading guest config");
-
-                    let raw_config = self
-                        .firewall_loader
-                        .guest_config(vmid, entry)
-                        .expect("guest config exists if firewall config exists");
-
-                    let config = GuestConfig::parse(
-                        vmid,
-                        entry.ty().iface_prefix(),
-                        raw_firewall_config,
-                        raw_config,
-                    )
-                    .expect("guest config is valid");
-
-                    guests.insert(*vmid, config);
-                }
-            }
-
-            guests
-        })
+        &self.guest_config
     }
 
     pub fn nft_chains(&self) -> &BTreeMap<String, ListChain> {
-        self.nft_config.get_or_init(|| {
-            let output = self.nft_loader.chains();
-            let mut chains = BTreeMap::new();
-
-            for element in &output.nftables {
-                if let ListOutput::Chain(chain) = element {
-                    chains.insert(chain.name().to_owned(), chain.clone());
-                }
-            }
-
-            chains
-        })
+        &self.nft_config
     }
 
     pub fn is_enabled(&self) -> bool {

proxmox-firewall/src/firewall.rs

@@ -41,7 +41,6 @@ static NF_CONNTRACK_TCP_TIMEOUT_SYN_RECV: &str =
     "/proc/sys/net/netfilter/nf_conntrack_tcp_timeout_syn_recv";
 static LOG_CONNTRACK_FILE: &str = "/var/lib/pve-firewall/log_nf_conntrack";
 
-#[derive(Default)]
 pub struct Firewall {
     config: FirewallConfig,
 }
@@ -53,10 +52,8 @@ impl From<FirewallConfig> for Firewall {
 }
 
 impl Firewall {
-    pub fn new() -> Self {
-        Self {
-            ..Default::default()
-        }
+    pub fn new(config: FirewallConfig) -> Self {
+        Self { config }
     }
 
     pub fn is_enabled(&self) -> bool {

proxmox-firewall/tests/integration_tests.rs

@@ -1,3 +1,4 @@
+use anyhow::{Context, Error};
 use std::collections::HashMap;
 
 use proxmox_firewall::config::{FirewallConfig, FirewallConfigLoader, NftConfigLoader};
@@ -16,15 +17,15 @@ impl MockFirewallConfigLoader {
 }
 
 impl FirewallConfigLoader for MockFirewallConfigLoader {
-    fn cluster(&self) -> Option<Box<dyn std::io::BufRead>> {
-        Some(Box::new(include_str!("input/cluster.fw").as_bytes()))
+    fn cluster(&self) -> Result<Option<Box<dyn std::io::BufRead>>, Error> {
+        Ok(Some(Box::new(include_str!("input/cluster.fw").as_bytes())))
     }
 
-    fn host(&self) -> Option<Box<dyn std::io::BufRead>> {
-        Some(Box::new(include_str!("input/host.fw").as_bytes()))
+    fn host(&self) -> Result<Option<Box<dyn std::io::BufRead>>, Error> {
+        Ok(Some(Box::new(include_str!("input/host.fw").as_bytes())))
     }
 
-    fn guest_list(&self) -> GuestMap {
+    fn guest_list(&self) -> Result<GuestMap, Error> {
         let hostname = nodename().to_string();
 
         let mut map = HashMap::new();
@@ -35,31 +36,38 @@ impl FirewallConfigLoader for MockFirewallConfigLoader {
         let entry = GuestEntry::new(hostname, GuestType::Ct);
         map.insert(100.into(), entry);
 
-        GuestMap::from(map)
+        Ok(GuestMap::from(map))
     }
 
-    fn guest_config(&self, vmid: &Vmid, _guest: &GuestEntry) -> Option<Box<dyn std::io::BufRead>> {
+    fn guest_config(
+        &self,
+        vmid: &Vmid,
+        _guest: &GuestEntry,
+    ) -> Result<Option<Box<dyn std::io::BufRead>>, Error> {
         if *vmid == Vmid::new(101) {
-            return Some(Box::new(include_str!("input/101.conf").as_bytes()));
+            return Ok(Some(Box::new(include_str!("input/101.conf").as_bytes())));
         }
 
         if *vmid == Vmid::new(100) {
-            return Some(Box::new(include_str!("input/100.conf").as_bytes()));
+            return Ok(Some(Box::new(include_str!("input/100.conf").as_bytes())));
         }
 
-        None
+        Ok(None)
     }
 
-    fn guest_firewall_config(&self, vmid: &Vmid) -> Option<Box<dyn std::io::BufRead>> {
+    fn guest_firewall_config(
+        &self,
+        vmid: &Vmid,
+    ) -> Result<Option<Box<dyn std::io::BufRead>>, Error> {
         if *vmid == Vmid::new(101) {
-            return Some(Box::new(include_str!("input/101.fw").as_bytes()));
+            return Ok(Some(Box::new(include_str!("input/101.fw").as_bytes())));
         }
 
         if *vmid == Vmid::new(100) {
-            return Some(Box::new(include_str!("input/100.fw").as_bytes()));
+            return Ok(Some(Box::new(include_str!("input/100.fw").as_bytes())));
         }
 
-        None
+        Ok(None)
     }
 }
 
@@ -72,19 +80,22 @@ impl MockNftConfigLoader {
 }
 
 impl NftConfigLoader for MockNftConfigLoader {
-    fn chains(&self) -> CommandOutput {
-        serde_json::from_str(include_str!("input/chains.json")).expect("valid chains.json")
+    fn chains(&self) -> Result<Option<CommandOutput>, Error> {
+        serde_json::from_str::<CommandOutput>(include_str!("input/chains.json"))
+            .map(Some)
+            .with_context(|| "invalid chains.json".to_string())
     }
 }
 
 #[test]
 fn test_firewall() {
     let firewall_config = FirewallConfig::new(
-        Box::new(MockFirewallConfigLoader::new()),
-        Box::new(MockNftConfigLoader::new()),
-    );
+        &MockFirewallConfigLoader::new(),
+        &MockNftConfigLoader::new(),
+    )
+    .expect("valid mock configuration");
 
-    let firewall = Firewall::from(firewall_config);
+    let firewall = Firewall::new(firewall_config);
 
     insta::assert_json_snapshot!(firewall.full_host_fw().expect("firewall can be generated"));
 }