From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 8448E1FF0BC for ; Thu, 15 Oct 2026 14:44:57 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3B189215F5; Thu, 15 Oct 2026 14:44:41 +0200 (CEST) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [PATCH proxmox-firewall v2 11/12] firewall: add restore command From: Stefan Hanreich To: Arthur Bied-Charreton In-Reply-To: <20261001122625.348730-12-a.bied-charreton@proxmox.com> References: <20261001122625.348730-1-a.bied-charreton@proxmox.com> <20261001122625.348730-12-a.bied-charreton@proxmox.com> Date: Thu, 15 Oct 2026 14:44:27 +0200 Message-Id: <179206826792.200327.6639899695911921043.b4-review@b4> X-Mailer: b4 0.16-dev-fbf84 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792068275674 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.533 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: CCMYNJIFBHG4A6SKPNSRDJHKNY5I3SCX X-Message-ID-Hash: CCMYNJIFBHG4A6SKPNSRDJHKNY5I3SCX X-MailFrom: s.hanreich@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: > 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 > Reviewed-by: Thomas Ellmenreich > Tested-by: Thomas Ellmenreich > > 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>) { > } > } > > +fn create_local_firewall_instance() -> Result { > + let config = FirewallConfig::new( > + &LocalFirewallConfigLoader::new(), > + &PveNftConfigLoader::new(), > + )?; > + Ok(Firewall::new(config)) > +} > + > fn create_firewall_instance() -> Result { > 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, Error> { > - match File::open(path) { > +fn open_config_file>(path: P) -> Result, Error> { nit: if we want to do this generically, then it'd make more sense to use AsRef 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>, 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; > @@ -168,7 +170,7 @@ impl FirewallConfigLoader for PveFirewallConfigLoader { > fn guest_firewall_config(&self, vmid: &Vmid) -> Result>, 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; > @@ -230,7 +232,7 @@ impl FirewallConfigLoader for PveFirewallConfigLoader { > ) -> Result>, 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; > @@ -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>, Error> { nit: could be fn load_first_of>( candidates: &[P], what: &str, ) -> Result>, 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; > + return Ok(Some(reader)); > + } > + } > + Ok(None) > +} > + > +impl FirewallConfigLoader for LocalFirewallConfigLoader { > + fn cluster(&self) -> Result>, Error> { > + load_first_of(&["cluster.fw"], "cluster") > + } > + > + fn host(&self) -> Result>, Error> { > + load_first_of(&["host.fw.new", "host.fw"], "host") > + } > + > + fn sdn_running_config(&self) -> Result>, 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; > + return Ok(Some(reader)); > + } > + > + Ok(None) > + } > + > + fn guest_config( > + &self, > + _: &Vmid, > + _: &GuestEntry, > + ) -> Result>, Error> { > + Ok(None) > + } > + > + fn guest_firewall_config(&self, _: &Vmid) -> Result>, Error> { > + Ok(None) > + } > + > + fn guest_list(&self) -> Result { > + Ok(GuestMap::from(HashMap::new())) > + } > + > + fn bridge_firewall_config( > + &self, > + _: &BridgeName, > + ) -> Result>, Error> { > + Ok(None) > + } > + > + fn bridge_list(&self) -> Result, Error> { > + Ok(vec![]) > + } > + > + fn ipam(&self) -> Result>, Error> { > + Ok(None) > + } > + > + fn interface_mapping(&self) -> Result { > + Ok(AltnameMapping::from_iter([])) Don't we actually need to get this, otherwise rules referencing altnames won't work on boot? -- Stefan Hanreich