[pve-devel] [PATCH proxmox-firewall 1/1] fix #6107: add support for legacy ipset / alias names
Stefan Hanreich
s.hanreich at proxmox.com
Fri Sep 12 18:11:18 CEST 2025
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 at 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
More information about the pve-devel
mailing list