From: Stefan Hanreich <s.hanreich@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH proxmox-firewall v2 3/3] fix #6336: fix ipfilter matching logic
Date: Thu, 25 Sep 2025 14:21:41 +0200 [thread overview]
Message-ID: <20250925122142.228719-5-s.hanreich@proxmox.com> (raw)
In-Reply-To: <20250925122142.228719-1-s.hanreich@proxmox.com>
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 <verdict>
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 <s.hanreich@proxmox.com>
---
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<NftRule>,
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<NftRule>,
+ 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::<Vec<NftRule>>();
+
+ rules.append(&mut new_rules);
+
+ Ok(())
+}
+
fn handle_match(
rules: &mut Vec<NftRule>,
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
prev parent reply other threads:[~2025-09-25 12:21 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-25 12:21 [pve-devel] [PATCH proxmox{-ve-rs, -firewall} v2 0/4] Fix ipfilters in proxmox-firewall Stefan Hanreich
2025-09-25 12:21 ` [pve-devel] [PATCH proxmox-ve-rs v2 1/1] config: guest: store network devices in BTreeMap Stefan Hanreich
2025-09-25 12:21 ` [pve-devel] [PATCH proxmox-firewall v2 1/3] run rustfmt Stefan Hanreich
2025-09-25 12:21 ` [pve-devel] [PATCH proxmox-firewall v2 2/3] ipfilter: fix wrong entries for containers Stefan Hanreich
2025-09-25 12:21 ` Stefan Hanreich [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250925122142.228719-5-s.hanreich@proxmox.com \
--to=s.hanreich@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.