[pve-devel] [PATCH v1 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs

[pve-devel] [PATCH v1 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs

[Robert Obkircher]

Match the behavior of the corresponding perl code in parse_fw_rule and
treat everything after the first '#' on a line as the comment. This
works because validation prevents fields like the interface name from
contianing that symbol.

This fixes a bug where the firewall wouldn't start if a comment on a
rule contained a number sign.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
The test in cluster.rs might be a bit overkill. Also, rust-analyzer
doesn't seem to work correctly at the end of that file as I can't seem
navigate to the defninitions of fields or methods.


 proxmox-ve-config/src/firewall/cluster.rs    | 66 +++++++++++++++++++-
 proxmox-ve-config/src/firewall/types/rule.rs |  8 ++-
 2 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/proxmox-ve-config/src/firewall/cluster.rs b/proxmox-ve-config/src/firewall/cluster.rs
index 69d3bcd..b8ecb69 100644
--- a/proxmox-ve-config/src/firewall/cluster.rs
+++ b/proxmox-ve-config/src/firewall/cluster.rs
@@ -147,7 +147,8 @@ mod tests {
         log::{LogLevel, LogRateLimitTimescale},
         rule::{Kind, RuleGroup},
         rule_match::{
-            Icmpv6, Icmpv6Code, Icmpv6Type, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Tcp, Udp
+            Icmpv6, Icmpv6Code, Icmpv6Type, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Tcp,
+            Udp,
         },
     };
 
@@ -394,4 +395,67 @@ IN BGP(REJECT) -log crit -source 1.2.3.4
         assert!(empty_config.config.ipsets.is_empty());
         assert!(empty_config.config.groups.is_empty());
     }
+
+    #[test]
+    fn test_parse_config_comments() {
+        const CONFIG: &str = r#"
+# ignore lines that start with a # symbol
+[OPTIONS]
+[ALIASES]
+
+alias1 192.168.0.1 # comment # on alias1
+
+[IPSET ipset1] # comment # on ipset1
+
+dc/alias1 # comment # on ipset1 #1
+
+[RULES]
+
+GROUP testgroup # comment # on rule #1
+IN ACCEPT -p udp -dport 1234 -log nolog # comment # on rule #2
+
+[group testgroup] # comment # on testgroup
+
+IN ACCEPT -source dc/alias1 -p tcp -sport 1111 -log nolog # comment # on testgroup #1
+
+"#;
+
+        let mut config = CONFIG.as_bytes();
+        let config = Config::parse(&mut config).unwrap();
+
+        assert_eq!(config.config.aliases.len(), 1);
+        assert_eq!(
+            config.config.aliases["alias1"].comment(),
+            Some("comment # on alias1")
+        );
+
+        assert_eq!(config.config.ipsets.len(), 1);
+
+        let ipset1 = &config.config.ipsets["ipset1"];
+        assert_eq!(ipset1.comment.as_deref(), Some("comment # on ipset1"));
+
+        assert_eq!(ipset1.len(), 1);
+        assert_eq!(ipset1[0].comment.as_deref(), Some("comment # on ipset1 #1"));
+
+        assert_eq!(config.config.rules.len(), 2);
+        assert_eq!(
+            config.config.rules[0].comment.as_deref(),
+            Some("comment # on rule #1")
+        );
+        assert_eq!(
+            config.config.rules[1].comment.as_deref(),
+            Some("comment # on rule #2")
+        );
+
+        assert_eq!(config.config.groups.len(), 1);
+
+        let entry = &config.config.groups["testgroup"];
+        assert_eq!(entry.comment(), Some("comment # on testgroup"));
+
+        assert_eq!(entry.rules().len(), 1);
+        assert_eq!(
+            entry.rules()[0].comment.as_deref(),
+            Some("comment # on testgroup #1")
+        );
+    }
 }
diff --git a/proxmox-ve-config/src/firewall/types/rule.rs b/proxmox-ve-config/src/firewall/types/rule.rs
index 811256c..2648fa8 100644
--- a/proxmox-ve-config/src/firewall/types/rule.rs
+++ b/proxmox-ve-config/src/firewall/types/rule.rs
@@ -115,7 +115,7 @@ impl FromStr for Rule {
             bail!("rule must not contain any newlines!");
         }
 
-        let (line, comment) = match input.rsplit_once('#') {
+        let (line, comment) = match input.split_once('#') {
             Some((line, comment)) if !comment.is_empty() => (line.trim(), Some(comment.trim())),
             _ => (input.trim(), None),
         };
@@ -261,13 +261,15 @@ mod tests {
 
     #[test]
     fn test_parse_rule() {
-        let mut rule: Rule = "|GROUP tgr -i eth0 # acomm".parse().expect("valid rule");
+        let mut rule: Rule = "|GROUP tgr -i eth0 # acomm #comment"
+            .parse()
+            .expect("valid rule");
 
         assert_eq!(
             rule,
             Rule {
                 disabled: true,
-                comment: Some("acomm".to_string()),
+                comment: Some("acomm #comment".to_string()),
                 kind: Kind::Group(RuleGroup {
                     group: "tgr".to_string(),
                     iface: Some("eth0".to_string()),
-- 
2.47.3



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