Re: [PATCH proxmox-firewall v2 11/12] firewall: add restore command

[Stefan Hanreich <s.hanreich@proxmox.com>]

> Add a 'restore' command that recompiles and applies the firewall from
> the config dumped to /var/lib/pve/firewall, to bridge the boot window
> before pmxcfs (and thus the real config) is available.
> 
> It reuses handle_firewall() along with a local firewall config loader.
> The config files are loaded from the local dump instead of pmxcfs and
> nothing is dumped back. A missing or unreadable SDN dump is treated as
> empty and rules referencing an unavailable IPSet are skipped, so a
> partial dump still restores the rest on a best-effort basis.
> 
> For the host config, a host.fw.new written by the
> pve-network-interface-pinning tool takes precedence over the plain dump
> when present. No .new file is ever created for the cluster config, so
> that is only loaded from cluster.fw.
> 
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> Reviewed-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> Tested-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
>
> diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
> index 1ac7c6c..423dfc7 100644
> --- a/proxmox-firewall/src/bin/proxmox-firewall.rs
> +++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
> @@ -9,7 +9,8 @@ use anyhow::{Context, Error, bail, format_err};
>  use pico_args::Arguments;
>  
>  use proxmox_firewall::config::{
> -    DUMP_DIR, FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader,
> +    DUMP_DIR, FirewallConfig, LocalFirewallConfigLoader, PveFirewallConfigLoader,
> +    PveNftConfigLoader,
>  };
>  use proxmox_firewall::firewall::Firewall;
>  use proxmox_log as log;
> @@ -29,6 +30,7 @@ COMMANDS:
>    compile           Compile and print firewall rules as accepted by 'nft -j -f -'
>    start             Execute proxmox-firewall service in foreground
>    localnet          Print the contents of the management ipset
> +  restore           Attempt to restore the last remembered ruleset
>  "#;
>  
>  const RULE_BASE: &str = include_str!("../../resources/proxmox-firewall.nft");
> @@ -87,6 +89,14 @@ fn dump_config(cfg: &FirewallConfig, cache: &mut HashMap<&str, Vec<u8>>) {
>      }
>  }
>  
> +fn create_local_firewall_instance() -> Result<Firewall, Error> {
> +    let config = FirewallConfig::new(
> +        &LocalFirewallConfigLoader::new(),
> +        &PveNftConfigLoader::new(),
> +    )?;
> +    Ok(Firewall::new(config))
> +}
> +
>  fn create_firewall_instance() -> Result<Firewall, Error> {
>      let config = FirewallConfig::new(&PveFirewallConfigLoader::new(), &PveNftConfigLoader::new())?;
>      Ok(Firewall::new(config))
> @@ -194,6 +204,17 @@ fn run_firewall() -> Result<(), Error> {
>      }
>  }
>  
> +fn restore() -> Result<(), Error> {
> +    let fw = create_local_firewall_instance()?;
> +    match fw.is_enabled() {
> +        true => handle_firewall(&fw),
> +        false => {
> +            log::info!("nftables firewall is not enabled, not restoring rules");
> +            Ok(())
> +        }
> +    }
> +}
> +
>  #[derive(Debug, Clone, Copy, PartialEq, Eq)]
>  pub enum Command {
>      Compile,
> @@ -201,6 +222,7 @@ pub enum Command {
>      Skeleton,
>      Start,
>      Localnet,
> +    Restore,
>  }
>  
>  impl std::str::FromStr for Command {
> @@ -213,6 +235,7 @@ impl std::str::FromStr for Command {
>              "skeleton" => Command::Skeleton,
>              "start" => Command::Start,
>              "localnet" => Command::Localnet,
> +            "restore" => Command::Restore,
>              cmd => {
>                  bail!("{cmd} is not a valid command")
>              }
> @@ -245,6 +268,7 @@ fn run_command(command: Command) -> Result<(), Error> {
>                  println!("{ip}");
>              }
>          }
> +        Command::Restore => restore()?,
>      };
>  
>      Ok(())
> diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs
> index eb8dd21..d6fe166 100644
> --- a/proxmox-firewall/src/config.rs
> +++ b/proxmox-firewall/src/config.rs
> @@ -1,7 +1,8 @@
> -use std::collections::BTreeMap;
> +use std::collections::{BTreeMap, HashMap};
>  use std::default::Default;
>  use std::fs::{self, DirEntry, File, ReadDir};
>  use std::io::{self, BufRead, BufReader};
> +use std::path::{Path, PathBuf};
>  
>  use anyhow::{Context, Error, bail, format_err};
>  
> @@ -60,15 +61,16 @@ impl PveFirewallConfigLoader {
>  /// opens a configuration file
>  ///
>  /// It returns a file handle to the file or [`None`] if it doesn't exist.
> -fn open_config_file(path: &str) -> Result<Option<File>, Error> {
> -    match File::open(path) {
> +fn open_config_file<P: Into<PathBuf>>(path: P) -> Result<Option<File>, Error> {

nit: if we want to do this generically, then it'd make more sense to
use AsRef<Path> as otherwise passing an argument here requires
ownership.

> +    let path: PathBuf = path.into();
> +    match File::open(&path) {
>          Ok(data) => Ok(Some(data)),
>          Err(err) if err.kind() == io::ErrorKind::NotFound => {
> -            log::info!("config file does not exist: {path}");
> +            log::info!("config file does not exist: {path:?}");
>              Ok(None)
>          }
>          Err(err) => {
> -            let context = format!("unable to open configuration file at {path}");
> +            let context = format!("unable to open configuration file at {path:?}");
>              Err(anyhow::Error::new(err).context(context))
>          }
>      }
> @@ -155,7 +157,7 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
>      ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
>          log::info!("loading guest #{vmid} config");
>  
> -        let fd = open_config_file(&GuestMap::config_path(vmid, entry))?;
> +        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>;
> @@ -168,7 +170,7 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
>      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))?;
> +        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>;
> @@ -230,7 +232,7 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
>      ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
>          log::info!("loading firewall config for bridge {bridge_name}");
>  
> -        let fd = open_config_file(&format!("/etc/pve/sdn/firewall/{bridge_name}.fw"))?;
> +        let fd = open_config_file(format!("/etc/pve/sdn/firewall/{bridge_name}.fw"))?;
>  
>          if let Some(file) = fd {
>              let buf_reader = Box::new(BufReader::new(file)) as Box<dyn io::BufRead>;
> @@ -247,6 +249,81 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
>      }
>  }
>  
> +#[derive(Debug, Default)]
> +pub struct LocalFirewallConfigLoader {}

we could make the dir configurable here and use DUMP_DIR as default?

> +
> +impl LocalFirewallConfigLoader {
> +    pub fn new() -> Self {
> +        Self::default()
> +    }
> +}
> +
> +fn load_first_of(candidates: &[&str], what: &str) -> Result<Option<Box<dyn BufRead>>, Error> {

nit: could be 

fn load_first_of<P: AsRef<Path>>(
    candidates: &[P],
    what: &str,
) -> Result<Option<Box<dyn BufRead>>, Error> {

then as well

> +    for p in candidates.iter().map(|p| Path::new(DUMP_DIR).join(p)) {

nit: could predefine Path like

let root_path = Path::new(DUMP_DIR);

and then only join it here to the existing root path:

for p in candidates.iter().map(|p| root_path.join(p))

> +        if let Some(fd) = open_config_file(&p)? {
> +            log::info!("loaded {what} config from {p:?}");
> +            let reader = Box::new(BufReader::new(fd)) as Box<dyn BufRead>;
> +            return Ok(Some(reader));
> +        }
> +    }
> +    Ok(None)
> +}
> +
> +impl FirewallConfigLoader for LocalFirewallConfigLoader {
> +    fn cluster(&self) -> Result<Option<Box<dyn BufRead>>, Error> {
> +        load_first_of(&["cluster.fw"], "cluster")
> +    }
> +
> +    fn host(&self) -> Result<Option<Box<dyn BufRead>>, Error> {
> +        load_first_of(&["host.fw.new", "host.fw"], "host")
> +    }
> +
> +    fn sdn_running_config(&self) -> Result<Option<Box<dyn BufRead>>, Error> {
> +        let path = Path::new(DUMP_DIR).join("sdn.json");
> +        if let Some(fd) = open_config_file(&path)? {
> +            let reader = Box::new(BufReader::new(fd)) as Box<dyn BufRead>;
> +            return Ok(Some(reader));
> +        }
> +
> +        Ok(None)
> +    }
> +
> +    fn guest_config(
> +        &self,
> +        _: &Vmid,
> +        _: &GuestEntry,
> +    ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
> +        Ok(None)
> +    }
> +
> +    fn guest_firewall_config(&self, _: &Vmid) -> Result<Option<Box<dyn io::BufRead>>, Error> {
> +        Ok(None)
> +    }
> +
> +    fn guest_list(&self) -> Result<GuestMap, Error> {
> +        Ok(GuestMap::from(HashMap::new()))
> +    }
> +
> +    fn bridge_firewall_config(
> +        &self,
> +        _: &BridgeName,
> +    ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
> +        Ok(None)
> +    }
> +
> +    fn bridge_list(&self) -> Result<Vec<BridgeName>, Error> {
> +        Ok(vec![])
> +    }
> +
> +    fn ipam(&self) -> Result<Option<Box<dyn io::BufRead>>, Error> {
> +        Ok(None)
> +    }
> +
> +    fn interface_mapping(&self) -> Result<AltnameMapping, Error> {
> +        Ok(AltnameMapping::from_iter([]))

Don't we actually need to get this, otherwise rules referencing
altnames won't work on boot?

-- 
Stefan Hanreich <s.hanreich@proxmox.com>