* [pve-devel] [PATCH proxmox-firewall] firewall: improve error handling of firewall
@ 2024-04-25 17:23 Stefan Hanreich
2024-04-25 17:32 ` [pve-devel] applied: " Thomas Lamprecht
0 siblings, 1 reply; 2+ messages in thread
From: Stefan Hanreich @ 2024-04-25 17:23 UTC (permalink / raw)
To: 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 <s.hanreich@proxmox.com>
---
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<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 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<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;
}
- })
- }
- pub fn guests(&self) -> &BTreeMap<Vmid, GuestConfig> {
- 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<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)
+ }
- guests
+ 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 nft_chains(&self) -> &BTreeMap<String, ListChain> {
- 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<Vmid, GuestConfig> {
+ &self.guest_config
+ }
+
+ pub fn nft_chains(&self) -> &BTreeMap<String, ListChain> {
+ &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<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 {
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<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"));
}
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
* [pve-devel] applied: [PATCH proxmox-firewall] firewall: improve error handling of firewall
2024-04-25 17:23 [pve-devel] [PATCH proxmox-firewall] firewall: improve error handling of firewall Stefan Hanreich
@ 2024-04-25 17:32 ` Thomas Lamprecht
0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2024-04-25 17:32 UTC (permalink / raw)
To: Proxmox VE development discussion, Stefan Hanreich
Am 25/04/2024 um 19:23 schrieb Stefan Hanreich:
> 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 | 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(-)
>
>
applied, thanks!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-04-25 17:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-25 17:23 [pve-devel] [PATCH proxmox-firewall] firewall: improve error handling of firewall Stefan Hanreich
2024-04-25 17:32 ` [pve-devel] applied: " Thomas Lamprecht
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.