From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 024961FF165 for ; Thu, 25 Sep 2025 14:24:09 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D97F31FF99; Thu, 25 Sep 2025 14:24:37 +0200 (CEST) From: Stefan Hanreich To: pve-devel@lists.proxmox.com Date: Thu, 25 Sep 2025 14:24:01 +0200 Message-ID: <20250925122403.230867-4-s.hanreich@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20250925122403.230867-1-s.hanreich@proxmox.com> References: <20250925122403.230867-1-s.hanreich@proxmox.com> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.181 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 v2 1/1] fix #6107: add support for legacy ipset / alias names 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" Change the existing firewall rule generation logic to accomodate the changes in the config parser, so it can deal with the legacy ipset / alias names. The lookup logic is the same as in pve-firewall: In guest context, check if it is contained in the guest config, otherwise check the cluster config. In cluster context, only check the cluster configuration. Because the rule generation logic now has to look up ipsets instead of blindly inserting rules with the ipset name, store the generated SDN ipsets in the firewall config as well, to avoid re-generating them for every lookup. This should also allow for better error reporting, in case ipsets or aliases are referenced that do not exist in the configuration. Signed-off-by: Stefan Hanreich --- proxmox-firewall/src/config.rs | 93 ++++++++-- proxmox-firewall/src/firewall.rs | 11 +- proxmox-firewall/src/object.rs | 4 +- proxmox-firewall/src/rule.rs | 26 ++- proxmox-firewall/tests/input/cluster.fw | 2 + .../integration_tests__firewall.snap | 172 ++++++++++++++++++ 6 files changed, 273 insertions(+), 35 deletions(-) diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs index 8bd9f2a..aa8829f 100644 --- a/proxmox-firewall/src/config.rs +++ b/proxmox-firewall/src/config.rs @@ -11,8 +11,10 @@ use proxmox_ve_config::firewall::bridge::Config as BridgeConfig; use proxmox_ve_config::firewall::cluster::Config as ClusterConfig; use proxmox_ve_config::firewall::guest::Config as GuestConfig; use proxmox_ve_config::firewall::host::Config as HostConfig; -use proxmox_ve_config::firewall::types::alias::{Alias, AliasName, AliasScope}; +use proxmox_ve_config::firewall::types::alias::{Alias, AliasScope, RuleAliasName}; +use proxmox_ve_config::firewall::types::ipset::{IpsetScope, RuleIpsetName}; +use proxmox_ve_config::firewall::types::Ipset; use proxmox_ve_config::guest::types::Vmid; use proxmox_ve_config::guest::{GuestEntry, GuestMap}; use proxmox_ve_config::host::types::BridgeName; @@ -257,13 +259,28 @@ impl NftConfigLoader for PveNftConfigLoader { } } +pub struct FirewallSdnConfig { + _config: SdnConfig, + ipsets: BTreeMap, +} + +impl FirewallSdnConfig { + pub fn ipsets(&self) -> &BTreeMap { + &self.ipsets + } + + pub fn ipset(&self, name: &str) -> Option<&Ipset> { + self.ipsets.get(name) + } +} + pub struct FirewallConfig { cluster_config: ClusterConfig, host_config: HostConfig, guest_config: BTreeMap, bridge_config: BTreeMap, nft_config: BTreeMap, - sdn_config: Option, + sdn_config: Option, ipam_config: Option, interface_mapping: AltnameMapping, } @@ -325,11 +342,21 @@ impl FirewallConfig { pub fn parse_sdn( firewall_loader: &dyn FirewallConfigLoader, - ) -> Result, Error> { + ) -> Result, Error> { Ok(match firewall_loader.sdn_running_config()? { Some(data) => { let running_config: RunningConfig = serde_json::from_reader(data)?; - Some(SdnConfig::try_from(running_config)?) + let config = SdnConfig::try_from(running_config)?; + + let ipsets = config + .ipsets(None) + .map(|ipset| (ipset.name().name().to_string(), ipset)) + .collect(); + + Some(FirewallSdnConfig { + _config: config, + ipsets, + }) } _ => None, }) @@ -415,7 +442,7 @@ impl FirewallConfig { &self.nft_config } - pub fn sdn(&self) -> Option<&SdnConfig> { + pub fn sdn(&self) -> Option<&FirewallSdnConfig> { self.sdn_config.as_ref() } @@ -431,22 +458,54 @@ impl FirewallConfig { self.interface_mapping.get(iface_name).map(|x| x.as_str()) } - pub fn alias(&self, name: &AliasName, vmid: Option) -> Option<&Alias> { - log::trace!("getting alias {name:?}"); + fn guest_alias(&self, name: &str, vmid: Vmid) -> Option<&Alias> { + if let Some(guest_config) = self.guests().get(&vmid) { + return guest_config.alias(name); + } - match name.scope() { - AliasScope::Datacenter => self.cluster().alias(name.name()), - AliasScope::Guest => { - if let Some(vmid) = vmid { - if let Some(entry) = self.guests().get(&vmid) { - return entry.alias(name); - } + log::warn!("trying to get alias {name} for non-existing guest: #{vmid}"); + None + } - log::warn!("trying to get alias {name} for non-existing guest: #{vmid}"); + pub fn alias(&self, name: &RuleAliasName, vmid: Option) -> Option<&Alias> { + log::trace!("getting alias {name:?}"); + + match name { + RuleAliasName::Scoped(alias_name) => match alias_name.scope() { + AliasScope::Datacenter => self.cluster().alias(alias_name.name()), + AliasScope::Guest => { + vmid.and_then(|vmid| self.guest_alias(alias_name.name(), vmid)) } + }, + RuleAliasName::Legacy(legacy_alias_name) => vmid + .and_then(|vmid| self.guest_alias(legacy_alias_name.as_ref(), vmid)) + .or_else(|| self.cluster().alias(legacy_alias_name.as_ref())), + } + } - None - } + fn guest_ipset(&self, name: &str, vmid: Vmid) -> Option<&Ipset> { + if let Some(guest_config) = self.guests().get(&vmid) { + return guest_config.ipset(name); + } + + log::warn!("trying to get ipset {name} for non-existing guest: #{vmid}"); + None + } + + pub fn ipset(&self, name: &RuleIpsetName, vmid: Option) -> Option<&Ipset> { + log::trace!("getting ipset {name:?}"); + + match name { + RuleIpsetName::Scoped(ipset_name) => match ipset_name.scope() { + IpsetScope::Sdn => self.sdn()?.ipset(ipset_name.name()), + IpsetScope::Datacenter => self.cluster().ipset(ipset_name.name()), + IpsetScope::Guest => { + vmid.and_then(|vmid| self.guest_ipset(ipset_name.name(), vmid)) + } + }, + RuleIpsetName::Legacy(legacy_ipset_name) => vmid + .and_then(|vmid| self.guest_ipset(legacy_ipset_name.as_ref(), vmid)) + .or_else(|| self.cluster().ipset(legacy_ipset_name.as_ref())), } } } diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs index ddf839b..30bc642 100644 --- a/proxmox-firewall/src/firewall.rs +++ b/proxmox-firewall/src/firewall.rs @@ -196,7 +196,7 @@ impl Firewall { let management_ips = HostConfig::management_ips()?; - let mut ipset = Ipset::from_parts(IpsetScope::Datacenter, "management"); + let mut ipset = Ipset::new(IpsetName::new(IpsetScope::Datacenter, "management")); ipset.reserve(management_ips.len()); let entries = management_ips.into_iter().map(IpsetEntry::from); @@ -229,13 +229,10 @@ impl Firewall { let guest_table = Self::guest_table(); if let Some(sdn_config) = self.config.sdn() { - let ipsets = sdn_config - .ipsets(None) - .map(|ipset| (ipset.name().to_string(), ipset)) - .collect(); + let ipsets = sdn_config.ipsets(); - self.create_ipsets(&mut commands, &ipsets, &cluster_host_table, None)?; - self.create_ipsets(&mut commands, &ipsets, &guest_table, None)?; + self.create_ipsets(&mut commands, ipsets, &cluster_host_table, None)?; + self.create_ipsets(&mut commands, ipsets, &guest_table, None)?; } if let Some(ipam_config) = self.config.ipam() { diff --git a/proxmox-firewall/src/object.rs b/proxmox-firewall/src/object.rs index 42423b9..5c18708 100644 --- a/proxmox-firewall/src/object.rs +++ b/proxmox-firewall/src/object.rs @@ -13,7 +13,7 @@ use proxmox_nftables::{ use proxmox_ve_config::{ firewall::{ ct_helper::CtHelperMacro, - types::{alias::AliasName, ipset::IpsetAddress, Alias, Ipset}, + types::{alias::RuleAliasName, ipset::IpsetAddress, Alias, Ipset}, }, guest::types::Vmid, }; @@ -29,7 +29,7 @@ pub(crate) struct NftObjectEnv<'a, 'b> { } impl NftObjectEnv<'_, '_> { - pub(crate) fn alias(&self, name: &AliasName) -> Option<&Alias> { + pub(crate) fn alias(&self, name: &RuleAliasName) -> Option<&Alias> { self.firewall_config.alias(name, self.vmid) } } diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs index 23e87a5..23bdfb4 100644 --- a/proxmox-firewall/src/rule.rs +++ b/proxmox-firewall/src/rule.rs @@ -14,14 +14,14 @@ use proxmox_ve_config::{ ct_helper::CtHelperMacro, fw_macros::{get_macro, FwMacro}, types::{ - alias::AliasName, - ipset::{Ipfilter, IpsetName}, + alias::RuleAliasName, + ipset::{Ipfilter, IpsetName, RuleIpsetName}, log::LogRateLimit, rule::{Direction, Kind, RuleGroup, Verdict as ConfigVerdict}, rule_match::{ Icmp, Icmpv6, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Sctp, Tcp, Udp, }, - Alias, Rule, + Alias, Ipset, Rule, }, }, guest::types::Vmid, @@ -121,7 +121,11 @@ pub(crate) struct NftRuleEnv<'a> { } impl NftRuleEnv<'_> { - fn alias(&self, name: &AliasName) -> Option<&Alias> { + fn ipset(&self, name: &RuleIpsetName) -> Option<&Ipset> { + self.firewall_config.ipset(name, self.vmid) + } + + fn alias(&self, name: &RuleAliasName) -> Option<&Alias> { self.firewall_config.alias(name, self.vmid) } @@ -289,10 +293,14 @@ impl ToNftRules for RuleMatch { /// `name`. It matches the IPs contained in the ipset with the field given in `field_name`. fn handle_set( rules: &mut Vec, - name: &IpsetName, + name: &RuleIpsetName, field_name: &str, env: &NftRuleEnv, ) -> Result<(), Error> { + let Some(ipset) = env.ipset(name) else { + bail!("could not find ipset {name}"); + }; + let mut new_rules = rules .drain(..) .flat_map(|rule| { @@ -310,7 +318,7 @@ fn handle_set( field.clone(), Expression::set_name(&SetName::ipset_name( Family::V4, - name, + ipset.name(), env.vmid, false, )), @@ -321,7 +329,7 @@ fn handle_set( field, Expression::set_name(&SetName::ipset_name( Family::V4, - name, + ipset.name(), env.vmid, true, )), @@ -344,7 +352,7 @@ fn handle_set( field.clone(), Expression::set_name(&SetName::ipset_name( Family::V6, - name, + ipset.name(), env.vmid, false, )), @@ -355,7 +363,7 @@ fn handle_set( field, Expression::set_name(&SetName::ipset_name( Family::V6, - name, + ipset.name(), env.vmid, true, )), diff --git a/proxmox-firewall/tests/input/cluster.fw b/proxmox-firewall/tests/input/cluster.fw index 3be7a72..376d2f1 100644 --- a/proxmox-firewall/tests/input/cluster.fw +++ b/proxmox-firewall/tests/input/cluster.fw @@ -20,8 +20,10 @@ dc/network1 GROUP network1 -i eth0 IN ACCEPT -log nolog +IN ACCEPT -dest +network1 [group network1] IN ACCEPT -source dc/network1 -dest dc/network1 -log nolog +IN ACCEPT -source network1 -dest network1 -log nolog diff --git a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap index 71285af..1a19ea7 100644 --- a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap +++ b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap @@ -1820,6 +1820,54 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" } } }, + { + "add": { + "rule": { + "family": "inet", + "table": "proxmox-firewall", + "chain": "group-network1-in", + "expr": [ + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "saddr" + } + }, + "right": { + "prefix": { + "addr": "172.16.100.0", + "len": 24 + } + } + } + }, + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "daddr" + } + }, + "right": { + "prefix": { + "addr": "172.16.100.0", + "len": 24 + } + } + } + }, + { + "accept": null + } + ] + } + } + }, { "add": { "chain": { @@ -1897,6 +1945,82 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" } } }, + { + "add": { + "rule": { + "family": "inet", + "table": "proxmox-firewall", + "chain": "cluster-in", + "expr": [ + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "daddr" + } + }, + "right": "@v4-dc/network1" + } + }, + { + "match": { + "op": "!=", + "left": { + "payload": { + "protocol": "ip", + "field": "daddr" + } + }, + "right": "@v4-dc/network1-nomatch" + } + }, + { + "accept": null + } + ] + } + } + }, + { + "add": { + "rule": { + "family": "inet", + "table": "proxmox-firewall", + "chain": "cluster-in", + "expr": [ + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip6", + "field": "daddr" + } + }, + "right": "@v6-dc/network1" + } + }, + { + "match": { + "op": "!=", + "left": { + "payload": { + "protocol": "ip6", + "field": "daddr" + } + }, + "right": "@v6-dc/network1-nomatch" + } + }, + { + "accept": null + } + ] + } + } + }, { "add": { "rule": { @@ -4003,6 +4127,54 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" } } }, + { + "add": { + "rule": { + "family": "bridge", + "table": "proxmox-firewall-guests", + "chain": "group-network1-in", + "expr": [ + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "saddr" + } + }, + "right": { + "prefix": { + "addr": "172.16.100.0", + "len": 24 + } + } + } + }, + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "daddr" + } + }, + "right": { + "prefix": { + "addr": "172.16.100.0", + "len": 24 + } + } + } + }, + { + "accept": null + } + ] + } + } + }, { "add": { "chain": { -- 2.47.3 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel