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 43A231FF165 for ; Thu, 25 Sep 2025 14:21:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4DA141FA71; Thu, 25 Sep 2025 14:21:48 +0200 (CEST) From: Stefan Hanreich To: pve-devel@lists.proxmox.com Date: Thu, 25 Sep 2025 14:21:41 +0200 Message-ID: <20250925122142.228719-5-s.hanreich@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20250925122142.228719-1-s.hanreich@proxmox.com> References: <20250925122142.228719-1-s.hanreich@proxmox.com> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.182 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 KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods 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. RDNS_NONE 0.793 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an SPF Record Subject: [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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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); + + 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); + + 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": "==", -- 2.47.3 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel