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 B14DD1FF179 for ; Wed, 1 Oct 2025 09:31:53 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D82101832F; Wed, 1 Oct 2025 09:31:58 +0200 (CEST) Message-ID: Date: Wed, 1 Oct 2025 09:31:54 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Stefan Hanreich References: <20250925122142.228719-1-s.hanreich@proxmox.com> <20250925122142.228719-5-s.hanreich@proxmox.com> Content-Language: en-US From: Hannes Laimer In-Reply-To: <20250925122142.228719-5-s.hanreich@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1759303892395 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.043 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] Subject: Re: [pve-devel] [PATCH proxmox-firewall v2 3/3] fix #6336: fix ipfilter matching logic 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" small typo, other than that LGTM! On 9/25/25 14:21, Stefan Hanreich wrote: > Matching on ipsets in the firewall generally works by matching on two > sets (one for match, one for nomatch): > > ip saddr @ipfilter ip saddr != @ipfilter-nomatch > > Ipfilters were created with the comparison operators simply inverted, > which leads to ipfilters with empty nomatch sets never working, since > the second expression always evaluates to false on empty sets: > > ip saddr != @ipfilter ip saddr = @ipfilter-nomatch drop > > In order to properly invert the logic, two statements need to be > generated (analogous to negation in boolean logic): > > ip saddr != @ipfilter drop > ip saddr = @ipfilter-nomatch drop > > To avoid cluttering the existing set handling function and to clearly > separate the rule generation logic, introduce a completely new > function that handles ipfilters separately. This also allows the set > handling function to validate the name of ipsets in a later commit, > since it only operates on ipsets that have to exist in the firewall > config, whereas ipfilters do not necessarily exist in the firewall > configuration. > > Signed-off-by: Stefan Hanreich > --- > proxmox-firewall/src/rule.rs | 131 ++++++++++++++++-- > .../integration_tests__firewall.snap | 100 +++++++++++++ > 2 files changed, 218 insertions(+), 13 deletions(-) > > diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs > index 2512537..23e87a5 100644 > --- a/proxmox-firewall/src/rule.rs > +++ b/proxmox-firewall/src/rule.rs > @@ -283,12 +283,15 @@ impl ToNftRules for RuleMatch { > } > } > > +/// Handle matching on an ipset. > +/// > +/// This function adds statements to the `rules` that match on the ipset with the given name > +/// `name`. It matches the IPs contained in the ipset with the field given in `field_name`. > fn handle_set( > rules: &mut Vec, > name: &IpsetName, > field_name: &str, > env: &NftRuleEnv, > - contains: bool, > ) -> Result<(), Error> { > let mut new_rules = rules > .drain(..) > @@ -303,7 +306,7 @@ fn handle_set( > > rule.append(&mut vec![ > Match::new( > - if contains { Operator::Eq } else { Operator::Ne }, > + Operator::Eq, > field.clone(), > Expression::set_name(&SetName::ipset_name( > Family::V4, > @@ -314,7 +317,7 @@ fn handle_set( > ) > .into(), > Match::new( > - if contains { Operator::Ne } else { Operator::Eq }, > + Operator::Ne, > field, > Expression::set_name(&SetName::ipset_name( > Family::V4, > @@ -337,7 +340,7 @@ fn handle_set( > > rule.append(&mut vec![ > Match::new( > - if contains { Operator::Eq } else { Operator::Ne }, > + Operator::Eq, > field.clone(), > Expression::set_name(&SetName::ipset_name( > Family::V6, > @@ -348,7 +351,7 @@ fn handle_set( > ) > .into(), > Match::new( > - if contains { Operator::Ne } else { Operator::Eq }, > + Operator::Ne, > field, > Expression::set_name(&SetName::ipset_name( > Family::V6, > @@ -372,6 +375,114 @@ fn handle_set( > Ok(()) > } > > +/// Handle matching on an ipfilter. > +/// > +/// This function adds statements to the `rules` that match if the IP in the field `field_name` > +/// does not fulfill the criteria of the given ipfilter. > +fn handle_ipfilter( > + rules: &mut Vec, > + name: &IpsetName, > + field_name: &str, > + env: &NftRuleEnv, > +) -> Result<(), Error> { > + let mut new_rules = rules > + .drain(..) > + .flat_map(|rule| { > + let mut new_rules = Vec::new(); > + > + if matches!(rule.family(), Some(Family::V4) | None) && env.contains_family(Family::V4) { > + let field = Payload::field("ip", field_name); > + > + let mut match_rule = rule.clone(); > + match_rule.set_family(Family::V4); > + > + match_rule.push( > + Match::new( > + Operator::Ne, > + field.clone(), > + Expression::set_name(&SetName::ipset_name( > + Family::V4, > + name, > + env.vmid, > + false, > + )), > + ) > + .into(), > + ); > + > + new_rules.push(match_rule); > + > + let mut nomatch_rule = rule.clone(); > + nomatch_rule.set_family(Family::V4); > + > + nomatch_rule.push( > + Match::new( > + Operator::Eq, > + field, > + Expression::set_name(&SetName::ipset_name( > + Family::V4, > + name, > + env.vmid, > + true, > + )), > + ) > + .into(), > + ); > + > + new_rules.push(nomatch_rule); > + } > + > + if matches!(rule.family(), Some(Family::V6) | None) && env.contains_family(Family::V6) { > + let field = Payload::field("ip6", field_name); > + > + let mut match_rule = rule.clone(); > + match_rule.set_family(Family::V4); > + ^ this should be `Family::V6` > + match_rule.push( > + Match::new( > + Operator::Ne, > + field.clone(), > + Expression::set_name(&SetName::ipset_name( > + Family::V6, > + name, > + env.vmid, > + false, > + )), > + ) > + .into(), > + ); > + > + new_rules.push(match_rule); > + > + let mut nomatch_rule = rule.clone(); > + nomatch_rule.set_family(Family::V4); > + ^ also > + nomatch_rule.push( > + Match::new( > + Operator::Eq, > + field, > + Expression::set_name(&SetName::ipset_name( > + Family::V6, > + name, > + env.vmid, > + true, > + )), > + ) > + .into(), > + ); > + > + new_rules.push(nomatch_rule); > + } > + > + new_rules > + }) > + .collect::>(); > + > + rules.append(&mut new_rules); > + > + Ok(()) > +} > + > fn handle_match( > rules: &mut Vec, > ip: &IpAddrMatch, > @@ -447,7 +558,7 @@ fn handle_match( > > Ok(()) > } > - IpAddrMatch::Set(name) => handle_set(rules, name, field_name, env, true), > + IpAddrMatch::Set(name) => handle_set(rules, name, field_name, env), > } > } > > @@ -679,13 +790,7 @@ impl ToNftRules for Ipfilter<'_> { > ); > > let mut ipfilter_rules = vec![base_rule.clone()]; > - handle_set( > - &mut ipfilter_rules, > - self.ipset().name(), > - "saddr", > - env, > - false, > - )?; > + handle_ipfilter(&mut ipfilter_rules, self.ipset().name(), "saddr", env)?; > rules.append(&mut ipfilter_rules); > > if env.contains_family(Family::V4) { > diff --git a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap > index feeda5b..71285af 100644 > --- a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap > +++ b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap > @@ -4279,6 +4279,31 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" > "right": "@v4-guest-100/ipfilter-net1" > } > }, > + { > + "drop": null > + } > + ] > + } > + } > + }, > + { > + "add": { > + "rule": { > + "family": "bridge", > + "table": "proxmox-firewall-guests", > + "chain": "guest-100-out", > + "expr": [ > + { > + "match": { > + "op": "==", > + "left": { > + "meta": { > + "key": "iifname" > + } > + }, > + "right": "veth100i1" > + } > + }, > { > "match": { > "op": "==", > @@ -4328,6 +4353,31 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" > "right": "@v6-guest-100/ipfilter-net1" > } > }, > + { > + "drop": null > + } > + ] > + } > + } > + }, > + { > + "add": { > + "rule": { > + "family": "bridge", > + "table": "proxmox-firewall-guests", > + "chain": "guest-100-out", > + "expr": [ > + { > + "match": { > + "op": "==", > + "left": { > + "meta": { > + "key": "iifname" > + } > + }, > + "right": "veth100i1" > + } > + }, > { > "match": { > "op": "==", > @@ -4579,6 +4629,31 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" > "right": "@v4-guest-100/ipfilter-net3" > } > }, > + { > + "drop": null > + } > + ] > + } > + } > + }, > + { > + "add": { > + "rule": { > + "family": "bridge", > + "table": "proxmox-firewall-guests", > + "chain": "guest-100-out", > + "expr": [ > + { > + "match": { > + "op": "==", > + "left": { > + "meta": { > + "key": "iifname" > + } > + }, > + "right": "veth100i3" > + } > + }, > { > "match": { > "op": "==", > @@ -4628,6 +4703,31 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" > "right": "@v6-guest-100/ipfilter-net3" > } > }, > + { > + "drop": null > + } > + ] > + } > + } > + }, > + { > + "add": { > + "rule": { > + "family": "bridge", > + "table": "proxmox-firewall-guests", > + "chain": "guest-100-out", > + "expr": [ > + { > + "match": { > + "op": "==", > + "left": { > + "meta": { > + "key": "iifname" > + } > + }, > + "right": "veth100i3" > + } > + }, > { > "match": { > "op": "==", _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel