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 91D371FF179 for ; Wed, 1 Oct 2025 18:28:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AEA2925747; Wed, 1 Oct 2025 18:28:57 +0200 (CEST) From: Stefan Hanreich To: pve-devel@lists.proxmox.com Date: Wed, 1 Oct 2025 18:28:16 +0200 Message-ID: <20251001162818.320717-5-s.hanreich@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20251001162818.320717-1-s.hanreich@proxmox.com> References: <20251001162818.320717-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 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 v3 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..570a172 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::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::V6); + + 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