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 5A7021FF16B for ; Fri, 12 Sep 2025 18:11:43 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E3664588; Fri, 12 Sep 2025 18:11:27 +0200 (CEST) From: Stefan Hanreich To: pve-devel@lists.proxmox.com Date: Fri, 12 Sep 2025 18:11:18 +0200 Message-ID: <20250912161121.229819-4-s.hanreich@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20250912161121.229819-1-s.hanreich@proxmox.com> References: <20250912161121.229819-1-s.hanreich@proxmox.com> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.183 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [object.rs, rule.rs, firewall.rs, config.rs] Subject: [pve-devel] [PATCH proxmox-firewall 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 | 15 +- proxmox-firewall/src/object.rs | 4 +- proxmox-firewall/src/rule.rs | 28 ++- proxmox-firewall/tests/input/cluster.fw | 2 + .../integration_tests__firewall.snap | 172 ++++++++++++++++++ 6 files changed, 276 insertions(+), 38 deletions(-) diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs index d6a4df5..2f06c8d 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().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 alias {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 8cac190..0f8772b 100644 --- a/proxmox-firewall/src/firewall.rs +++ b/proxmox-firewall/src/firewall.rs @@ -25,17 +25,17 @@ use proxmox_ve_config::firewall::guest::Config as GuestConfig; use proxmox_ve_config::firewall::host::Config as HostConfig; use proxmox_network_types::ip_address::{Cidr, Ipv6Cidr}; -use proxmox_ve_config::firewall::types::Group; use proxmox_ve_config::firewall::types::ipset::{ Ipfilter, Ipset, IpsetEntry, IpsetName, IpsetScope, }; use proxmox_ve_config::firewall::types::log::{LogLevel as ConfigLogLevel, LogRateLimit}; use proxmox_ve_config::firewall::types::rule::{Direction, Verdict as ConfigVerdict}; +use proxmox_ve_config::firewall::types::Group; use proxmox_ve_config::guest::types::Vmid; use crate::config::FirewallConfig; use crate::object::{NftObjectEnv, ToNftObjects}; -use crate::rule::{NftRule, NftRuleEnv, generate_verdict}; +use crate::rule::{generate_verdict, NftRule, NftRuleEnv}; static CLUSTER_TABLE_NAME: &str = "proxmox-firewall"; static HOST_TABLE_NAME: &str = "proxmox-firewall"; @@ -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 a7575bb..da2c589 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, Ipset, alias::AliasName, ipset::IpsetAddress}, + 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 77bc6ea..62499b8 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::{FwMacro, get_macro}, types::{ - Alias, Rule, - alias::AliasName, - ipset::{Ipfilter, IpsetName}, + alias::RuleAliasName, + ipset::{Ipfilter, RuleIpsetName}, log::LogRateLimit, rule::{Direction, Kind, RuleGroup, Verdict as ConfigVerdict}, rule_match::{ Icmp, Icmpv6, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Sctp, Tcp, Udp, }, + 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) } @@ -285,11 +289,15 @@ impl ToNftRules for RuleMatch { fn handle_set( rules: &mut Vec, - name: &IpsetName, + name: &RuleIpsetName, field_name: &str, env: &NftRuleEnv, contains: bool, ) -> Result<(), Error> { + let Some(ipset) = env.ipset(name) else { + bail!("could not find ipset {name}"); + }; + let mut new_rules = rules .drain(..) .flat_map(|rule| { @@ -307,7 +315,7 @@ fn handle_set( field.clone(), Expression::set_name(&SetName::ipset_name( Family::V4, - name, + ipset.name(), env.vmid, false, )), @@ -318,7 +326,7 @@ fn handle_set( field, Expression::set_name(&SetName::ipset_name( Family::V4, - name, + ipset.name(), env.vmid, true, )), @@ -341,7 +349,7 @@ fn handle_set( field.clone(), Expression::set_name(&SetName::ipset_name( Family::V6, - name, + ipset.name(), env.vmid, false, )), @@ -352,7 +360,7 @@ fn handle_set( field, Expression::set_name(&SetName::ipset_name( Family::V6, - name, + ipset.name(), env.vmid, true, )), @@ -681,7 +689,7 @@ impl ToNftRules for Ipfilter<'_> { let mut ipfilter_rules = vec![base_rule.clone()]; handle_set( &mut ipfilter_rules, - self.ipset().name(), + &RuleIpsetName::Scoped(self.ipset().name().clone()), "saddr", env, false, 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 e3db8ae..4f9a970 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