[pve-devel] [PATCH proxmox-ve-rs v2 1/2] fix #5927: firewall: alias: lowercase names of aliases

* [pve-devel] [PATCH proxmox-ve-rs v2 1/2] fix #5927: firewall: alias: lowercase names of aliases
@ 2024-11-25 17:04 Stefan Hanreich
  2024-11-25 17:04 ` [pve-devel] [PATCH proxmox-ve-rs v2 2/2] firewall: alias: document difference between Alias and AliasName Stefan Hanreich
  2024-11-25 21:06 ` [pve-devel] applied-series: [PATCH proxmox-ve-rs v2 1/2] fix #5927: firewall: alias: lowercase names of aliases Thomas Lamprecht
  0 siblings, 2 replies; 3+ messages in thread
From: Stefan Hanreich @ 2024-11-25 17:04 UTC (permalink / raw)
  To: pve-devel

pve-firewall lowercases the names of aliases when reading from the
configuration as well as when comparing source / destination entries
with the entries in the parsed aliases. In order to stay
backwards-compatible we also need to lowercase any parsed alias name.
I decided to this in the constructor and switch all call sites to the
new constructor, so there's only one place where we have to handle
lowercasing the string.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---

Changes from v1 to v2:
* fix typo and improve description in documentation of Alias
* add bugzilla issue # to commit msg

Also leaving the thoughts I posted under v1 here:

Leo supplied a patch already [1] that changes the handling of names in
the firewall. Might make sense to revisit this patch series in the
future to unify how cases are handled in the firewall. Nevertheless,
this patch acts as a quick fix for users running into issues with the
firewall due to the difference in handling cases in pve-firewall and
proxmox-firewall.

[1] https://lists.proxmox.com/pipermail/pve-devel/2023-January/055596.html


 proxmox-ve-config/src/firewall/types/alias.rs | 49 ++++++++++++++-----
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/proxmox-ve-config/src/firewall/types/alias.rs b/proxmox-ve-config/src/firewall/types/alias.rs
index 5dfaa41..553931e 100644
--- a/proxmox-ve-config/src/firewall/types/alias.rs
+++ b/proxmox-ve-config/src/firewall/types/alias.rs
@@ -53,10 +53,7 @@ impl FromStr for AliasName {
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         match s.split_once('/') {
-            Some((prefix, name)) if !name.is_empty() => Ok(Self {
-                scope: prefix.parse()?,
-                name: name.to_string(),
-            }),
+            Some((prefix, name)) if !name.is_empty() => Ok(Self::new(prefix.parse()?, name)),
             _ => {
                 bail!("Invalid Alias name!")
             }
@@ -65,10 +62,18 @@ impl FromStr for AliasName {
 }
 
 impl AliasName {
+    /// Creates a new [`AliasName`].
+    ///
+    /// It will convert any ASCII characters contained in the name into lowercase. This is for
+    /// maintaining backwards-compatiblity with pve-firewall, where all aliases are lowercased when
+    /// reading from the config.
     pub fn new(scope: AliasScope, name: impl Into<String>) -> Self {
+        let mut lowercase_name = name.into();
+        lowercase_name.make_ascii_lowercase();
+
         Self {
             scope,
-            name: name.into(),
+            name: lowercase_name,
         }
     }
 
@@ -90,13 +95,21 @@ pub struct Alias {
 }
 
 impl Alias {
+    /// Creates a new [`Alias`].
+    ///
+    /// It will convert any ASCII characters contained in the name into lowercase. This is for
+    /// maintaining backwards-compatiblity with pve-firewall, where all aliases are lowercased when
+    /// reading from the config.
     pub fn new(
         name: impl Into<String>,
         address: impl Into<Cidr>,
         comment: impl Into<Option<String>>,
     ) -> Self {
+        let mut lowercase_name = name.into();
+        lowercase_name.make_ascii_lowercase();
+
         Self {
-            name: name.into(),
+            name: lowercase_name,
             address: address.into(),
             comment: comment.into(),
         }
@@ -135,11 +148,7 @@ impl FromStr for Alias {
             None => None,
         };
 
-        Ok(Alias {
-            name: name.to_string(),
-            address,
-            comment,
-        })
+        Ok(Alias::new(name, address, comment))
     }
 }
 
@@ -159,6 +168,16 @@ mod tests {
         for alias in ["-- 10.0.0.1/32", "0asd 10.0.0.1/32", "__test 10.0.0.0/32"] {
             alias.parse::<Alias>().expect_err("invalid alias");
         }
+
+        let alias = "pRoxMox 10.0.0.0/32 # a comment"
+            .parse::<Alias>()
+            .expect("valid alias");
+        assert_eq!(alias.name(), "proxmox");
+        assert_eq!(
+            alias.address(),
+            &Cidr::new_v4([10, 0, 0, 0], 32).expect("valid CIDR")
+        );
+        assert_eq!(alias.comment(), Some("a comment"));
     }
 
     #[test]
@@ -171,4 +190,12 @@ mod tests {
             name.parse::<AliasName>().expect_err("invalid alias name");
         }
     }
+
+    #[test]
+    fn test_parse_alias_case() {
+        for name in ["dc/PROxMoX", "guest/PROXMOX"] {
+            let alias_name = name.parse::<AliasName>().expect("valid alias name");
+            assert_eq!(alias_name.name(), "proxmox");
+        }
+    }
 }
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 3+ messages in thread