* [pve-devel] [PATCH v1 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs
@ 2025-11-28 13:22 Robert Obkircher
2025-12-05 10:08 ` Stefan Hanreich
0 siblings, 1 reply; 2+ messages in thread
From: Robert Obkircher @ 2025-11-28 13:22 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [pve-devel] [PATCH v1 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs
2025-11-28 13:22 [pve-devel] [PATCH v1 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs Robert Obkircher
@ 2025-12-05 10:08 ` Stefan Hanreich
0 siblings, 0 replies; 2+ messages in thread
From: Stefan Hanreich @ 2025-12-05 10:08 UTC (permalink / raw)
To: Proxmox VE development discussion, Robert Obkircher
Ran this on a test machine, could reproduce the issue without the patch
applied and confirm that it is fixed with the patch applied.
some small comments inline
Consider this:
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
On 11/28/25 2:24 PM, Robert Obkircher wrote:
> 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.
Don't think so - always good to have additional tests to have our bases
covered - it also gives me as a reviewer additional confidence in your
patch series.
>
>
> 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,
This seems to be a simple auto-format change unrelated to the patch? Imo
always better to submit them up-front / separately.
> },
> };
>
> @@ -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()),
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-12-05 10:09 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-11-28 13:22 [pve-devel] [PATCH v1 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs Robert Obkircher
2025-12-05 10:08 ` Stefan Hanreich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox