From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 46DB520EC88 for ; Thu, 25 Apr 2024 19:23:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 571D31FDBE; Thu, 25 Apr 2024 19:23:11 +0200 (CEST) From: Stefan Hanreich To: pve-devel@lists.proxmox.com Date: Thu, 25 Apr 2024 19:23:07 +0200 Message-Id: <20240425172307.654151-1-s.hanreich@proxmox.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.268 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods RDNS_NONE 0.793 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an SPF Record Subject: [pve-devel] [PATCH proxmox-firewall] firewall: improve error handling of firewall X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 --- proxmox-firewall/src/bin/proxmox-firewall.rs | 7 +- proxmox-firewall/src/config.rs | 239 +++++++++---------- proxmox-firewall/src/firewall.rs | 7 +- proxmox-firewall/tests/integration_tests.rs | 51 ++-- 4 files changed, 155 insertions(+), 149 deletions(-) diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs index 4e07993..b61007d 100644 --- a/proxmox-firewall/src/bin/proxmox-firewall.rs +++ b/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(); diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs index 2cf3e39..5bd2512 100644 --- a/proxmox-firewall/src/config.rs +++ b/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>; - fn host(&self) -> Option>; - fn guest_list(&self) -> GuestMap; - fn guest_config(&self, vmid: &Vmid, guest: &GuestEntry) -> Option>; - fn guest_firewall_config(&self, vmid: &Vmid) -> Option>; + fn cluster(&self) -> Result>, Error>; + fn host(&self) -> Result>, Error>; + fn guest_list(&self) -> Result; + fn guest_config( + &self, + vmid: &Vmid, + guest: &GuestEntry, + ) -> Result>, Error>; + fn guest_firewall_config(&self, vmid: &Vmid) -> Result>, 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> { + fn cluster(&self) -> Result>, 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; - return Some(buf_reader); + return Ok(Some(buf_reader)); } - None + Ok(None) } - fn host(&self) -> Option> { + fn host(&self) -> Result>, 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; - return Some(buf_reader); + return Ok(Some(buf_reader)); } - None + Ok(None) } - fn guest_list(&self) -> GuestMap { + fn guest_list(&self) -> Result { log::info!("loading vmlist"); - GuestMap::new().expect("able to read vmlist") + GuestMap::new() } - fn guest_config(&self, vmid: &Vmid, entry: &GuestEntry) -> Option> { + fn guest_config( + &self, + vmid: &Vmid, + entry: &GuestEntry, + ) -> Result>, 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; - return Some(buf_reader); + return Ok(Some(buf_reader)); } - None + Ok(None) } - fn guest_firewall_config(&self, vmid: &Vmid) -> Option> { + 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)) - .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; - return Some(buf_reader); + return Ok(Some(buf_reader)); } - None + Ok(None) } } pub trait NftConfigLoader { - fn chains(&self) -> CommandOutput; + fn chains(&self) -> Result, Error>; } #[derive(Debug, Default)] @@ -131,131 +135,122 @@ impl PveNftConfigLoader { } impl NftConfigLoader for PveNftConfigLoader { - fn chains(&self) -> CommandOutput { + fn chains(&self) -> Result, 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, - nft_loader: Box, - cluster_config: OnceLock, - host_config: OnceLock, - guest_config: OnceLock>, - nft_config: OnceLock>, -} - -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, + nft_config: BTreeMap, } impl FirewallConfig { - pub fn new( - firewall_loader: Box, - nft_loader: Box, - ) -> 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 { + 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 { + 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 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() - } + pub fn parse_guests( + firewall_loader: &dyn FirewallConfigLoader, + ) -> Result, 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; } - }) - } - pub fn guests(&self) -> &BTreeMap { - self.guest_config.get_or_init(|| { - let mut guests = BTreeMap::new(); + let raw_firewall_config = firewall_loader.guest_firewall_config(vmid)?; - for (vmid, entry) in self.firewall_loader.guest_list().iter() { - if !entry.is_local() { - log::debug!("guest #{vmid} is not local, skipping"); - continue; - } + if let Some(raw_firewall_config) = raw_firewall_config { + log::debug!("found firewall config for #{vmid}, loading guest config"); - let raw_firewall_config = self.firewall_loader.guest_firewall_config(vmid); + let raw_config = firewall_loader + .guest_config(vmid, entry)? + .ok_or_else(|| format_err!("could not load guest config for #{vmid}"))?; - if let Some(raw_firewall_config) = raw_firewall_config { - log::debug!("found firewall config for #{vmid}, loading guest config"); + let config = GuestConfig::parse( + vmid, + entry.ty().iface_prefix(), + raw_firewall_config, + raw_config, + )?; - let raw_config = self - .firewall_loader - .guest_config(vmid, entry) - .expect("guest config exists if firewall config exists"); + guests.insert(*vmid, config); + } + } - let config = GuestConfig::parse( - vmid, - entry.ty().iface_prefix(), - raw_firewall_config, - raw_config, - ) - .expect("guest config is valid"); + Ok(guests) + } - guests.insert(*vmid, config); - } + pub fn parse_nft( + nft_loader: &dyn NftConfigLoader, + ) -> Result, 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) + } - guests + pub fn new( + firewall_loader: &dyn FirewallConfigLoader, + nft_loader: &dyn NftConfigLoader, + ) -> Result { + 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 nft_chains(&self) -> &BTreeMap { - self.nft_config.get_or_init(|| { - let output = self.nft_loader.chains(); - let mut chains = BTreeMap::new(); + pub fn cluster(&self) -> &ClusterConfig { + &self.cluster_config + } - for element in &output.nftables { - if let ListOutput::Chain(chain) = element { - chains.insert(chain.name().to_owned(), chain.clone()); - } - } + pub fn host(&self) -> &HostConfig { + &self.host_config + } - chains - }) + pub fn guests(&self) -> &BTreeMap { + &self.guest_config + } + + pub fn nft_chains(&self) -> &BTreeMap { + &self.nft_config } pub fn is_enabled(&self) -> bool { diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs index 509e295..41b1df2 100644 --- a/proxmox-firewall/src/firewall.rs +++ b/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 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 { diff --git a/proxmox-firewall/tests/integration_tests.rs b/proxmox-firewall/tests/integration_tests.rs index 860c78d..e9baffe 100644 --- a/proxmox-firewall/tests/integration_tests.rs +++ b/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> { - Some(Box::new(include_str!("input/cluster.fw").as_bytes())) + fn cluster(&self) -> Result>, Error> { + Ok(Some(Box::new(include_str!("input/cluster.fw").as_bytes()))) } - fn host(&self) -> Option> { - Some(Box::new(include_str!("input/host.fw").as_bytes())) + fn host(&self) -> Result>, Error> { + Ok(Some(Box::new(include_str!("input/host.fw").as_bytes()))) } - fn guest_list(&self) -> GuestMap { + fn guest_list(&self) -> Result { 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> { + fn guest_config( + &self, + vmid: &Vmid, + _guest: &GuestEntry, + ) -> Result>, 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> { + fn guest_firewall_config( + &self, + vmid: &Vmid, + ) -> Result>, 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, Error> { + serde_json::from_str::(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")); } -- 2.39.2 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel