[pve-devel] [PATCH proxmox-ve-rs 2/2] config: firewall: add support for legacy ipset names

[Stefan Hanreich <s.hanreich@proxmox.com>]

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