[pve-devel] [PATCH proxmox-ve-rs 1/2] config: firewall: add support for legacy alias 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. 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