From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 202571FF1A6 for ; Fri, 5 Dec 2025 11:09:08 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0304B131E7; Fri, 5 Dec 2025 11:09:34 +0100 (CET) Message-ID: <12c04c3b-3608-4c83-a3fe-b1bf6912f791@proxmox.com> Date: Fri, 5 Dec 2025 11:08:59 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Robert Obkircher References: <20251128132458.177476-1-r.obkircher@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20251128132458.177476-1-r.obkircher@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.725 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [rule.rs, cluster.rs] Subject: Re: [pve-devel] [PATCH v1 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 Reviewed-by: Stefan Hanreich 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 > --- > 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