* [pve-devel] [PATCH proxmox-ve-rs 1/2] config: firewall: add support for legacy alias names
2025-09-12 16:11 [pve-devel] [PATCH proxmox{-ve-rs, -firewall} 0/3] Add support for legacy ipset / alias names Stefan Hanreich
@ 2025-09-12 16:11 ` 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
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hanreich @ 2025-09-12 16:11 UTC (permalink / raw)
To: pve-devel
Proxmox VE 8.0 introduced scopes for ipsets and aliases in the
firewall configuration. Since existing firewall configuration was not
converted to this new syntax, there are still many systems that have
at least one alias / ipset name in their firewall ruleset that does
not contain any scope. proxmox-firewall failed to parse such rules and
in turn failed to create a new ruleset, which was a common occurence
when users switched from pve-firewall to proxmox-firewall. Since
aliases can also occur in ipset definitions, add support for parsing
non-scoped alias names there as well.
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
proxmox-ve-config/src/firewall/cluster.rs | 12 ++-
proxmox-ve-config/src/firewall/guest.rs | 6 +-
proxmox-ve-config/src/firewall/types/alias.rs | 98 +++++++++++++++++--
proxmox-ve-config/src/firewall/types/ipset.rs | 7 +-
proxmox-ve-config/src/firewall/types/rule.rs | 7 +-
.../src/firewall/types/rule_match.rs | 10 +-
6 files changed, 118 insertions(+), 22 deletions(-)
diff --git a/proxmox-ve-config/src/firewall/cluster.rs b/proxmox-ve-config/src/firewall/cluster.rs
index d588302..a477a53 100644
--- a/proxmox-ve-config/src/firewall/cluster.rs
+++ b/proxmox-ve-config/src/firewall/cluster.rs
@@ -138,7 +138,7 @@ mod tests {
use crate::firewall::types::{
address::IpList,
- alias::{AliasName, AliasScope},
+ alias::{AliasName, AliasScope, RuleAliasName},
ipset::{IpsetAddress, IpsetEntry},
log::{LogLevel, LogRateLimitTimescale},
rule::{Kind, RuleGroup},
@@ -250,12 +250,18 @@ IN BGP(REJECT) -log crit -source 1.2.3.4
},
IpsetEntry {
nomatch: false,
- address: IpsetAddress::Alias(AliasName::new(AliasScope::Datacenter, "analias")),
+ address: IpsetAddress::Alias(RuleAliasName::Scoped(AliasName::new(
+ AliasScope::Datacenter,
+ "analias",
+ ))),
comment: Some("a comment".to_string()),
},
IpsetEntry {
nomatch: false,
- address: IpsetAddress::Alias(AliasName::new(AliasScope::Datacenter, "wide")),
+ address: IpsetAddress::Alias(RuleAliasName::Scoped(AliasName::new(
+ AliasScope::Datacenter,
+ "wide",
+ ))),
comment: None,
},
IpsetEntry {
diff --git a/proxmox-ve-config/src/firewall/guest.rs b/proxmox-ve-config/src/firewall/guest.rs
index 4428a75..349fdb4 100644
--- a/proxmox-ve-config/src/firewall/guest.rs
+++ b/proxmox-ve-config/src/firewall/guest.rs
@@ -4,7 +4,7 @@ use std::io;
use crate::guest::types::Vmid;
use crate::guest::vm::NetworkConfig;
-use crate::firewall::types::alias::{Alias, AliasName};
+use crate::firewall::types::alias::Alias;
use crate::firewall::types::ipset::IpsetScope;
use crate::firewall::types::log::LogLevel;
use crate::firewall::types::rule::{Direction, Rule, Verdict};
@@ -108,8 +108,8 @@ impl Config {
self.vmid
}
- pub fn alias(&self, name: &AliasName) -> Option<&Alias> {
- self.config.alias(name.name())
+ pub fn alias(&self, name: &str) -> Option<&Alias> {
+ self.config.alias(name)
}
pub fn iface_name_by_key(&self, key: &str) -> Result<String, Error> {
diff --git a/proxmox-ve-config/src/firewall/types/alias.rs b/proxmox-ve-config/src/firewall/types/alias.rs
index c7a58c1..0d89bb9 100644
--- a/proxmox-ve-config/src/firewall/types/alias.rs
+++ b/proxmox-ve-config/src/firewall/types/alias.rs
@@ -6,8 +6,7 @@ use proxmox_network_types::ip_address::Cidr;
use crate::firewall::parse::{match_name, match_non_whitespace};
-#[derive(Debug, Clone)]
-#[cfg_attr(test, derive(Eq, PartialEq))]
+#[derive(Debug, Clone, Eq, PartialEq)]
pub enum AliasScope {
Datacenter,
Guest,
@@ -34,10 +33,51 @@ impl Display for AliasScope {
}
}
-/// Represents the name of an alias in a firewall rule in the RULES section of the firewall
-/// configuration.
-#[derive(Debug, Clone)]
-#[cfg_attr(test, derive(Eq, PartialEq))]
+/// An alias name in the firewall rules without any scope identifier.
+///
+/// It starts with an alphabetic character, followed by alphanumeric characters or
+/// hyphens/underscores.
+///
+/// When parsing the name, this will convert any ASCII characters contained in the name into
+/// lowercase. This is for maintaining backwards-compatibility with pve-firewall, where all aliases
+/// are lowercased when reading from the config.
+#[derive(Debug, Clone, Eq, PartialEq)]
+#[repr(transparent)]
+pub struct LegacyAliasName(String);
+
+impl AsRef<str> for LegacyAliasName {
+ fn as_ref(&self) -> &str {
+ &self.0
+ }
+}
+
+impl FromStr for LegacyAliasName {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ if let Some((name, "")) = match_name(s) {
+ return Ok(Self(name.to_lowercase()));
+ }
+
+ bail!("not a valid alias name: {s}");
+ }
+}
+
+impl Display for LegacyAliasName {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ self.0.fmt(f)
+ }
+}
+
+/// An alias name in the firewall rules with scope identifier
+///
+/// It starts with a scope ([`AliasScope`]), followed by a slash, and then has the same format as
+/// [`LegacyAliasName`]
+///
+/// When parsing the name, this will convert any ASCII characters contained in the name into
+/// lowercase. This is for maintaining backwards-compatibility with pve-firewall, where all aliases
+/// are lowercased when reading from the config.
+#[derive(Debug, Clone, Eq, PartialEq)]
pub struct AliasName {
scope: AliasScope,
name: String,
@@ -90,6 +130,40 @@ impl AliasName {
}
}
+/// The name of an alias in a firewall rule.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum RuleAliasName {
+ Scoped(AliasName),
+ Legacy(LegacyAliasName),
+}
+
+proxmox_serde::forward_deserialize_to_from_str!(RuleAliasName);
+
+impl FromStr for RuleAliasName {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ if let Ok(scoped_name) = s.parse() {
+ return Ok(Self::Scoped(scoped_name));
+ }
+
+ if let Ok(legacy_name) = s.parse() {
+ return Ok(Self::Legacy(legacy_name));
+ }
+
+ bail!("invalid alias name: {s}");
+ }
+}
+
+impl Display for RuleAliasName {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ Self::Scoped(alias_name) => alias_name.fmt(f),
+ Self::Legacy(legacy_name) => legacy_name.fmt(f),
+ }
+ }
+}
+
/// Represents an Alias stored in the ALIASES section of the firewall configuration.
///
/// Since they contain no scope in the firewall configuration itself, this struct also does not
@@ -189,6 +263,18 @@ mod tests {
assert_eq!(alias.comment(), Some("a comment"));
}
+ #[test]
+ fn test_parse_legacy_alias_name() {
+ for name in ["proxmox_123", "proxmox-123"] {
+ name.parse::<LegacyAliasName>().expect("valid alias name");
+ }
+
+ for name in ["1proxmox_123", "-proxmox-123"] {
+ name.parse::<LegacyAliasName>()
+ .expect_err("invalid alias name");
+ }
+ }
+
#[test]
fn test_parse_alias_name() {
for name in ["dc/proxmox_123", "guest/proxmox-123"] {
diff --git a/proxmox-ve-config/src/firewall/types/ipset.rs b/proxmox-ve-config/src/firewall/types/ipset.rs
index 8a51e38..2df6943 100644
--- a/proxmox-ve-config/src/firewall/types/ipset.rs
+++ b/proxmox-ve-config/src/firewall/types/ipset.rs
@@ -5,10 +5,11 @@ use std::str::FromStr;
use anyhow::{bail, format_err, Error};
use proxmox_network_types::ip_address::{Cidr, IpRange};
-use crate::firewall::parse::match_non_whitespace;
-use crate::firewall::types::alias::AliasName;
+use crate::firewall::parse::{match_name, match_non_whitespace};
use crate::guest::vm::NetworkConfig;
+use super::alias::RuleAliasName;
+
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
pub enum IpsetScope {
Datacenter,
@@ -92,7 +93,7 @@ impl Display for IpsetName {
#[derive(Debug, Clone)]
#[cfg_attr(test, derive(Eq, PartialEq))]
pub enum IpsetAddress {
- Alias(AliasName),
+ Alias(RuleAliasName),
Cidr(Cidr),
Range(IpRange),
}
diff --git a/proxmox-ve-config/src/firewall/types/rule.rs b/proxmox-ve-config/src/firewall/types/rule.rs
index 3ad8cf0..049f270 100644
--- a/proxmox-ve-config/src/firewall/types/rule.rs
+++ b/proxmox-ve-config/src/firewall/types/rule.rs
@@ -251,8 +251,8 @@ mod tests {
use crate::firewall::types::{
address::{IpEntry, IpList},
- alias::{AliasName, AliasScope},
ipset::{IpsetName, IpsetScope},
+ alias::{AliasName, AliasScope, RuleAliasName},
log::LogLevel,
rule_match::{Icmp, IcmpCode, IpAddrMatch, IpMatch, Ports, Protocol, Udp},
};
@@ -381,7 +381,10 @@ mod tests {
verdict: Verdict::Accept,
ip: Some(
IpMatch::new(
- IpAddrMatch::Alias(AliasName::new(AliasScope::Datacenter, "test")),
+ IpAddrMatch::Alias(RuleAliasName::Scoped(AliasName::new(
+ AliasScope::Datacenter,
+ "test"
+ ))),
IpAddrMatch::Set(IpsetName::new(IpsetScope::Datacenter, "test"),),
)
.unwrap()
diff --git a/proxmox-ve-config/src/firewall/types/rule_match.rs b/proxmox-ve-config/src/firewall/types/rule_match.rs
index 7fcd35c..2be84c5 100644
--- a/proxmox-ve-config/src/firewall/types/rule_match.rs
+++ b/proxmox-ve-config/src/firewall/types/rule_match.rs
@@ -12,7 +12,7 @@ use proxmox_sortable_macro::sortable;
use crate::firewall::parse::{match_name, match_non_whitespace, SomeStr};
use crate::firewall::types::address::IpList;
-use crate::firewall::types::alias::AliasName;
+use crate::firewall::types::alias::RuleAliasName;
use crate::firewall::types::ipset::IpsetName;
use crate::firewall::types::log::LogLevel;
use crate::firewall::types::port::PortList;
@@ -254,7 +254,7 @@ impl IpMatch {
pub enum IpAddrMatch {
Ip(IpList),
Set(IpsetName),
- Alias(AliasName),
+ Alias(RuleAliasName),
}
impl IpAddrMatch {
@@ -771,7 +771,7 @@ impl fmt::Display for Icmpv6Code {
#[cfg(test)]
mod tests {
- use crate::firewall::types::alias::AliasScope::Guest;
+ use crate::firewall::types::alias::{AliasName, AliasScope::Guest};
use proxmox_network_types::ip_address::Cidr;
use super::*;
@@ -844,7 +844,7 @@ mod tests {
IpMatch::new(
IpAddrMatch::Ip(IpList::from(Cidr::new_v4([10, 0, 0, 0], 8).unwrap())),
- IpAddrMatch::Alias(AliasName::new(Guest, "test")),
+ IpAddrMatch::Alias(RuleAliasName::Scoped(AliasName::new(Guest, "test"))),
)
.expect("valid ip match");
@@ -893,7 +893,7 @@ mod tests {
options = RuleOptions {
proto: Some("tcp".to_string()),
sport: Some("qwe".to_string()),
- source: Some("qwe".to_string()),
+ source: Some("_qwe".to_string()),
..Default::default()
};
--
2.47.3
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH proxmox-ve-rs 2/2] config: firewall: add support for legacy ipset names
2025-09-12 16:11 [pve-devel] [PATCH proxmox{-ve-rs, -firewall} 0/3] Add support for legacy ipset / alias names 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 ` Stefan Hanreich
2025-09-12 16:11 ` [pve-devel] [PATCH proxmox-firewall 1/1] fix #6107: add support for legacy ipset / alias names Stefan Hanreich
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hanreich @ 2025-09-12 16:11 UTC (permalink / raw)
To: pve-devel
Proxmox VE 8.0 introduced scopes for ipsets and aliases in the
firewall configuration. Since existing firewall configuration was not
converted to this new syntax, there are still many systems that have
at least one alias / ipset name in their firewall ruleset that does
not contain any scope. proxmox-firewall failed to parse such rules and
in turn failed to create a new ruleset, which was a common occurence
when users switched from pve-firewall to proxmox-firewall.
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
proxmox-ve-config/src/firewall/cluster.rs | 4 +
proxmox-ve-config/src/firewall/common.rs | 4 +
proxmox-ve-config/src/firewall/guest.rs | 4 +
proxmox-ve-config/src/firewall/types/ipset.rs | 97 ++++++++++++++++++-
proxmox-ve-config/src/firewall/types/rule.rs | 7 +-
.../src/firewall/types/rule_match.rs | 4 +-
6 files changed, 114 insertions(+), 6 deletions(-)
diff --git a/proxmox-ve-config/src/firewall/cluster.rs b/proxmox-ve-config/src/firewall/cluster.rs
index a477a53..d82e66f 100644
--- a/proxmox-ve-config/src/firewall/cluster.rs
+++ b/proxmox-ve-config/src/firewall/cluster.rs
@@ -53,6 +53,10 @@ impl Config {
&self.config.ipsets
}
+ pub fn ipset(&self, name: &str) -> Option<&Ipset> {
+ self.config.ipsets.get(name)
+ }
+
pub fn alias(&self, name: &str) -> Option<&Alias> {
self.config.alias(name)
}
diff --git a/proxmox-ve-config/src/firewall/common.rs b/proxmox-ve-config/src/firewall/common.rs
index 3999168..d77fe72 100644
--- a/proxmox-ve-config/src/firewall/common.rs
+++ b/proxmox-ve-config/src/firewall/common.rs
@@ -189,6 +189,10 @@ where
&self.ipsets
}
+ pub fn ipset(&self, name: &str) -> Option<&Ipset> {
+ self.ipsets.get(name)
+ }
+
pub fn alias(&self, name: &str) -> Option<&Alias> {
self.aliases.get(name)
}
diff --git a/proxmox-ve-config/src/firewall/guest.rs b/proxmox-ve-config/src/firewall/guest.rs
index 349fdb4..65ef29d 100644
--- a/proxmox-ve-config/src/firewall/guest.rs
+++ b/proxmox-ve-config/src/firewall/guest.rs
@@ -108,6 +108,10 @@ impl Config {
self.vmid
}
+ pub fn ipset(&self, name: &str) -> Option<&Ipset> {
+ self.config.ipset(name)
+ }
+
pub fn alias(&self, name: &str) -> Option<&Alias> {
self.config.alias(name)
}
diff --git a/proxmox-ve-config/src/firewall/types/ipset.rs b/proxmox-ve-config/src/firewall/types/ipset.rs
index 2df6943..3f93bb2 100644
--- a/proxmox-ve-config/src/firewall/types/ipset.rs
+++ b/proxmox-ve-config/src/firewall/types/ipset.rs
@@ -10,6 +10,7 @@ use crate::guest::vm::NetworkConfig;
use super::alias::RuleAliasName;
+/// The scope of an ipset.
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
pub enum IpsetScope {
Datacenter,
@@ -42,8 +43,47 @@ impl Display for IpsetScope {
}
}
-#[derive(Debug, Clone)]
-#[cfg_attr(test, derive(Eq, PartialEq))]
+/// An ipset name in the firewall rules without any scope identifier.
+///
+/// Legacy ipset names start with a +, followed by an alphabetic character and only contain
+/// alphanumeric characters or hyphens and underscores.
+#[derive(Debug, Clone, Eq, PartialEq)]
+#[repr(transparent)]
+pub struct LegacyIpsetName(String);
+
+impl AsRef<str> for LegacyIpsetName {
+ fn as_ref(&self) -> &str {
+ &self.0
+ }
+}
+
+impl FromStr for LegacyIpsetName {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ if let Some(name) = s.strip_prefix("+") {
+ if let Some((name, "")) = match_name(name) {
+ return Ok(Self(name.to_string()));
+ }
+
+ bail!("not a valid ipset name: {s}");
+ }
+
+ bail!("ipset name does not start with '+': {s}");
+ }
+}
+
+impl Display for LegacyIpsetName {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ self.0.fmt(f)
+ }
+}
+
+/// The new format of ipset names in firewall rules (with scope).
+///
+/// It starts with a plus sign, then a scope [`IpsetScope`], followed by a slash, and then has the
+/// same format as [`LegacyIpsetName`].
+#[derive(Debug, Clone, Eq, PartialEq)]
pub struct IpsetName {
pub scope: IpsetScope,
pub name: String,
@@ -90,6 +130,39 @@ impl Display for IpsetName {
}
}
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum RuleIpsetName {
+ Scoped(IpsetName),
+ Legacy(LegacyIpsetName),
+}
+
+proxmox_serde::forward_deserialize_to_from_str!(RuleIpsetName);
+
+impl FromStr for RuleIpsetName {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ if let Ok(scoped_name) = s.parse() {
+ return Ok(Self::Scoped(scoped_name));
+ }
+
+ if let Ok(legacy_name) = s.parse() {
+ return Ok(Self::Legacy(legacy_name));
+ }
+
+ bail!("is neither a valid scoped nor legacy ipset name: {s}");
+ }
+}
+
+impl Display for RuleIpsetName {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ Self::Scoped(ipset_name) => ipset_name.fmt(f),
+ Self::Legacy(ipset_name) => ipset_name.fmt(f),
+ }
+ }
+}
+
#[derive(Debug, Clone)]
#[cfg_attr(test, derive(Eq, PartialEq))]
pub enum IpsetAddress {
@@ -304,6 +377,26 @@ mod tests {
}
}
+ #[test]
+ fn test_parse_legacy_ipset_name() {
+ for test_case in [
+ ("+proxmox_123", "proxmox_123"),
+ ("+proxmox_---123", "proxmox_---123"),
+ ] {
+ let ipset_name = test_case
+ .0
+ .parse::<LegacyIpsetName>()
+ .expect("valid ipset name");
+
+ assert_eq!(ipset_name, LegacyIpsetName(test_case.1.to_string(),))
+ }
+
+ for name in ["guest/proxmox_123", "+-qwe", "+1qwe"] {
+ name.parse::<LegacyIpsetName>()
+ .expect_err("invalid ipset name");
+ }
+ }
+
#[test]
fn test_parse_ipset_address() {
let mut ipset_address = "10.0.0.1"
diff --git a/proxmox-ve-config/src/firewall/types/rule.rs b/proxmox-ve-config/src/firewall/types/rule.rs
index 049f270..3b2b8ca 100644
--- a/proxmox-ve-config/src/firewall/types/rule.rs
+++ b/proxmox-ve-config/src/firewall/types/rule.rs
@@ -251,8 +251,8 @@ mod tests {
use crate::firewall::types::{
address::{IpEntry, IpList},
- ipset::{IpsetName, IpsetScope},
alias::{AliasName, AliasScope, RuleAliasName},
+ ipset::{IpsetName, IpsetScope, RuleIpsetName},
log::LogLevel,
rule_match::{Icmp, IcmpCode, IpAddrMatch, IpMatch, Ports, Protocol, Udp},
};
@@ -385,7 +385,10 @@ mod tests {
AliasScope::Datacenter,
"test"
))),
- IpAddrMatch::Set(IpsetName::new(IpsetScope::Datacenter, "test"),),
+ IpAddrMatch::Set(RuleIpsetName::Scoped(IpsetName::new(
+ IpsetScope::Datacenter,
+ "test"
+ )),),
)
.unwrap()
),
diff --git a/proxmox-ve-config/src/firewall/types/rule_match.rs b/proxmox-ve-config/src/firewall/types/rule_match.rs
index 2be84c5..9b474d2 100644
--- a/proxmox-ve-config/src/firewall/types/rule_match.rs
+++ b/proxmox-ve-config/src/firewall/types/rule_match.rs
@@ -13,7 +13,7 @@ use proxmox_sortable_macro::sortable;
use crate::firewall::parse::{match_name, match_non_whitespace, SomeStr};
use crate::firewall::types::address::IpList;
use crate::firewall::types::alias::RuleAliasName;
-use crate::firewall::types::ipset::IpsetName;
+use crate::firewall::types::ipset::RuleIpsetName;
use crate::firewall::types::log::LogLevel;
use crate::firewall::types::port::PortList;
use crate::firewall::types::rule::{Direction, Verdict};
@@ -253,7 +253,7 @@ impl IpMatch {
#[cfg_attr(test, derive(Eq, PartialEq))]
pub enum IpAddrMatch {
Ip(IpList),
- Set(IpsetName),
+ Set(RuleIpsetName),
Alias(RuleAliasName),
}
--
2.47.3
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH proxmox-firewall 1/1] fix #6107: add support for legacy ipset / alias names
2025-09-12 16:11 [pve-devel] [PATCH proxmox{-ve-rs, -firewall} 0/3] Add support for legacy ipset / alias names 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
2025-09-24 10:25 ` [pve-devel] [PATCH proxmox{-ve-rs, -firewall} 0/3] Add " Stefan Hanreich
2025-09-25 12:25 ` [pve-devel] superseded: " Stefan Hanreich
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hanreich @ 2025-09-12 16:11 UTC (permalink / raw)
To: 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 <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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH proxmox{-ve-rs, -firewall} 0/3] Add support for legacy ipset / alias names
2025-09-12 16:11 [pve-devel] [PATCH proxmox{-ve-rs, -firewall} 0/3] Add support for legacy ipset / alias names Stefan Hanreich
` (2 preceding siblings ...)
2025-09-12 16:11 ` [pve-devel] [PATCH proxmox-firewall 1/1] fix #6107: add support for legacy ipset / alias names Stefan Hanreich
@ 2025-09-24 10:25 ` Stefan Hanreich
2025-09-25 12:25 ` [pve-devel] superseded: " Stefan Hanreich
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hanreich @ 2025-09-24 10:25 UTC (permalink / raw)
To: pve-devel
found an issue while re-visiting this again in the context of creating
another patch series - please do not apply for now!
On 9/12/25 6:11 PM, Stefan Hanreich wrote:
> The introduction of scopes to alias / ipset names in firewall rules in Proxmox
> VE 8 did not include any automated mechanism for converting firewall rules. Many
> users still have firewall configurations containing unscoped names. The initial
> decision to only support the new format with proxmox-firewall led to problems
> with users trying to migrate to the nftables firewall, since the daemon fails to
> parse the configuration and generates no nftables ruleset at all.
>
> proxmox-ve-rs:
>
> Stefan Hanreich (2):
> config: firewall: add support for legacy alias names
> config: firewall: add support for legacy ipset names
>
> proxmox-ve-config/src/firewall/cluster.rs | 16 ++-
> proxmox-ve-config/src/firewall/common.rs | 4 +
> proxmox-ve-config/src/firewall/guest.rs | 10 +-
> proxmox-ve-config/src/firewall/types/alias.rs | 98 ++++++++++++++++-
> proxmox-ve-config/src/firewall/types/ipset.rs | 104 +++++++++++++++++-
> proxmox-ve-config/src/firewall/types/rule.rs | 14 ++-
> .../src/firewall/types/rule_match.rs | 14 +--
> 7 files changed, 232 insertions(+), 28 deletions(-)
>
>
> proxmox-firewall:
>
> Stefan Hanreich (1):
> fix #6107: add support for legacy ipset / alias names
>
> 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(-)
>
>
> Summary over all repositories:
> 13 files changed, 508 insertions(+), 66 deletions(-)
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] superseded: [PATCH proxmox{-ve-rs, -firewall} 0/3] Add support for legacy ipset / alias names
2025-09-12 16:11 [pve-devel] [PATCH proxmox{-ve-rs, -firewall} 0/3] Add support for legacy ipset / alias names Stefan Hanreich
` (3 preceding siblings ...)
2025-09-24 10:25 ` [pve-devel] [PATCH proxmox{-ve-rs, -firewall} 0/3] Add " Stefan Hanreich
@ 2025-09-25 12:25 ` Stefan Hanreich
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hanreich @ 2025-09-25 12:25 UTC (permalink / raw)
To: pve-devel
https://lore.proxmox.com/pve-devel/20250925122403.230867-1-s.hanreich@proxmox.com/T/
On 9/12/25 6:11 PM, Stefan Hanreich wrote:
> The introduction of scopes to alias / ipset names in firewall rules in Proxmox
> VE 8 did not include any automated mechanism for converting firewall rules. Many
> users still have firewall configurations containing unscoped names. The initial
> decision to only support the new format with proxmox-firewall led to problems
> with users trying to migrate to the nftables firewall, since the daemon fails to
> parse the configuration and generates no nftables ruleset at all.
>
> proxmox-ve-rs:
>
> Stefan Hanreich (2):
> config: firewall: add support for legacy alias names
> config: firewall: add support for legacy ipset names
>
> proxmox-ve-config/src/firewall/cluster.rs | 16 ++-
> proxmox-ve-config/src/firewall/common.rs | 4 +
> proxmox-ve-config/src/firewall/guest.rs | 10 +-
> proxmox-ve-config/src/firewall/types/alias.rs | 98 ++++++++++++++++-
> proxmox-ve-config/src/firewall/types/ipset.rs | 104 +++++++++++++++++-
> proxmox-ve-config/src/firewall/types/rule.rs | 14 ++-
> .../src/firewall/types/rule_match.rs | 14 +--
> 7 files changed, 232 insertions(+), 28 deletions(-)
>
>
> proxmox-firewall:
>
> Stefan Hanreich (1):
> fix #6107: add support for legacy ipset / alias names
>
> 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(-)
>
>
> Summary over all repositories:
> 13 files changed, 508 insertions(+), 66 deletions(-)
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 6+ messages in thread