From: Stefan Hanreich <s.hanreich@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH proxmox-firewall 1/1] fix #6107: add support for legacy ipset / alias names
Date: Fri, 12 Sep 2025 18:11:18 +0200 [thread overview]
Message-ID: <20250912161121.229819-4-s.hanreich@proxmox.com> (raw)
In-Reply-To: <20250912161121.229819-1-s.hanreich@proxmox.com>
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 <s.hanreich@proxmox.com>
---
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<String, Ipset>,
+}
+
+impl FirewallSdnConfig {
+ pub fn ipsets(&self) -> &BTreeMap<String, Ipset> {
+ &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<Vmid, GuestConfig>,
bridge_config: BTreeMap<BridgeName, BridgeConfig>,
nft_config: BTreeMap<String, ListChain>,
- sdn_config: Option<SdnConfig>,
+ sdn_config: Option<FirewallSdnConfig>,
ipam_config: Option<Ipam>,
interface_mapping: AltnameMapping,
}
@@ -325,11 +342,21 @@ impl FirewallConfig {
pub fn parse_sdn(
firewall_loader: &dyn FirewallConfigLoader,
- ) -> Result<Option<SdnConfig>, Error> {
+ ) -> Result<Option<FirewallSdnConfig>, 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<Vmid>) -> 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<Vmid>) -> 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<Vmid>) -> 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<NftRule>,
- 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
prev parent reply other threads:[~2025-09-12 16:11 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-12 16:11 [pve-devel] [PATCH proxmox{-ve-rs, -firewall} 0/3] Add " Stefan Hanreich
2025-09-12 16:11 ` [pve-devel] [PATCH proxmox-ve-rs 1/2] config: firewall: add support for legacy " Stefan Hanreich
2025-09-12 16:11 ` [pve-devel] [PATCH proxmox-ve-rs 2/2] config: firewall: add support for legacy ipset names Stefan Hanreich
2025-09-12 16:11 ` Stefan Hanreich [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250912161121.229819-4-s.hanreich@proxmox.com \
--to=s.hanreich@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox