Re: [PATCH proxmox-firewall v2 10/12] firewall: dump config to local directory after apply

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

It feels as though it might make sense to move the cache handling into
its own struct instead of using a HashMap here that gets passed
around?

> The firewall configuration files are stored on pmxcfs, which is only
> available when pve-cluster is up. To be able to restore firewall rules
> before the network comes up and close the boot-time window in which the
> PVE host is unprotected, dump the required configuration files to a
> local directory after every apply.
> 
> The last dumped version is cached for each file, the daemon only
> actually writes to disk if there has been a change since the last write
> or the dump file does not exist.
> 
> Ownership of the dumps is keyed on the nftables option in the host
> config. When the firewall is disabled, the dumps are only removed if
> nftables is enabled - otherwise that responsibility is on pve-firewall.
> 
> The dumps are also removed when the daemon is stopped, to prevent a
> ruleset restored at boot from living forever if no daemon starts
> afterwards.
> 
> 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/debian/dirs b/debian/dirs
> new file mode 100644
> index 0000000..34355af
> --- /dev/null
> +++ b/debian/dirs
> @@ -0,0 +1 @@
> +/var/lib/pve/firewall
> diff --git a/proxmox-firewall/Cargo.toml b/proxmox-firewall/Cargo.toml
> index fa7fd34..77ca285 100644
> --- a/proxmox-firewall/Cargo.toml
> +++ b/proxmox-firewall/Cargo.toml
> @@ -22,9 +22,9 @@ proxmox-log.workspace = true
>  proxmox-network-types.workspace = true
>  proxmox-network-api = { workspace = true, features = [ "impl" ] }
>  proxmox-nftables = { workspace = true, features = [ "config-ext" ] }
> +proxmox-sys.workspace = true
>  proxmox-systemd.workspace = true
>  proxmox-ve-config.workspace = true
>  
>  [dev-dependencies]
>  insta = { workspace = true, features = [ "json" ] }
> -proxmox-sys.workspace = true
> diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
> index 27fd67d..1ac7c6c 100644
> --- a/proxmox-firewall/src/bin/proxmox-firewall.rs
> +++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
> @@ -1,3 +1,6 @@
> +use std::collections::HashMap;
> +use std::os::unix::fs::PermissionsExt;
> +use std::path::Path;
>  use std::sync::Arc;
>  use std::sync::atomic::{AtomicBool, Ordering};
>  use std::time::{Duration, Instant};
> @@ -5,11 +8,14 @@ use std::time::{Duration, Instant};
>  use anyhow::{Context, Error, bail, format_err};
>  use pico_args::Arguments;
>  
> -use proxmox_firewall::config::{FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader};
> +use proxmox_firewall::config::{
> +    DUMP_DIR, FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader,
> +};
>  use proxmox_firewall::firewall::Firewall;
>  use proxmox_log as log;
>  use proxmox_log::{LevelFilter, Logger};
>  use proxmox_nftables::{NftClient, client::NftError};
> +use proxmox_sys::fs;
>  use proxmox_systemd::systemctl;
>  use proxmox_ve_config::firewall::host::Config as HostConfig;
>  
> @@ -42,14 +48,58 @@ fn remove_firewall() -> Result<(), std::io::Error> {
>      Ok(())
>  }
>  
> +fn dump_config(cfg: &FirewallConfig, cache: &mut HashMap<&str, Vec<u8>>) {
> +    let dump_dir = Path::new(DUMP_DIR);
> +    if !dump_dir.exists()
> +        && let Err(e) = fs::create_path(dump_dir, None, None)
> +    {
> +        log::error!("could not create {DUMP_DIR}: {e} - config will not be dumped");
> +        return;
> +    }
> +
> +    if let Err(e) = std::fs::set_permissions(dump_dir, std::fs::Permissions::from_mode(0o700)) {
> +        log::warn!("could not set permissions on {DUMP_DIR}: {e}");
> +    }

Would make sense to set the permissions on creation here as well
instead of doing it every update cycle.

> +
> +    for (bytes, name) in [
> +        (cfg.cluster_raw(), "cluster.fw"),
> +        (cfg.host_raw(), "host.fw"),
> +        (cfg.sdn_raw(), "sdn.json"),
> +    ] {
> +        let out = dump_dir.join(name);
> +        match bytes {
> +            Some(b) if !out.exists() || b != cache.get(name).map(Vec::as_slice).unwrap_or(&[]) => {
> +                match fs::replace_file(&out, b, fs::CreateOptions::new(), true) {
> +                    Err(e) => log::error!("could not dump {name} to {out:?}: {e}"),
> +                    Ok(()) => {
> +                        log::info!("successfully dumped {out:?}");
> +                        cache.insert(name, b.to_vec());
> +                    }
> +                }
> +            }
> +            Some(_) => log::debug!("no changes to {name} since last dump to {out:?}"),
> +            None => {
> +                log::debug!("nothing to dump for {name}");
> +                cache.remove(name);
> +                _ = std::fs::remove_file(&out);

at least log the error here if unsuccessful?

> +            }
> +        }
> +    }
> +}
> +
>  fn create_firewall_instance() -> Result<Firewall, Error> {
>      let config = FirewallConfig::new(&PveFirewallConfigLoader::new(), &PveNftConfigLoader::new())?;
>      Ok(Firewall::new(config))
>  }
>  
> -fn handle_firewall() -> Result<(), Error> {
> -    let firewall = create_firewall_instance()?;
> +fn delete_dumps(cache: &mut HashMap<&str, Vec<u8>>) {
> +    cache.clear();
> +    for f in ["cluster.fw", "host.fw", "sdn.json"] {
> +        _ = std::fs::remove_file(Path::new(DUMP_DIR).join(f));

same w.r.t. logging errors here as well...

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