From: Stefan Hanreich <s.hanreich@proxmox.com>
To: Hannes Laimer <h.laimer@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH proxmox-firewall v2 3/3] fix #6336: fix ipfilter matching logic
Date: Wed, 1 Oct 2025 10:11:58 +0200 [thread overview]
Message-ID: <640c74a9-a616-4a3c-99be-3a0e43f15e13@proxmox.com> (raw)
In-Reply-To: <e4b9bb63-a3f0-4590-b957-0583fb95ed0d@proxmox.com>
On 10/1/25 9:31 AM, Hannes Laimer wrote:
> small typo, other than that LGTM!
excellent catch, tyvm!
> 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 <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);
>> +
>
> ^ 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::<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": "==",
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next prev parent reply other threads:[~2025-10-01 8:11 UTC|newest]
Thread overview: 10+ 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-10-01 7:23 ` Hannes Laimer
2025-10-01 8:17 ` 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 ` [pve-devel] [PATCH proxmox-firewall v2 3/3] fix #6336: fix ipfilter matching logic Stefan Hanreich
2025-10-01 7:31 ` Hannes Laimer
2025-10-01 8:11 ` Stefan Hanreich [this message]
2025-10-01 16:30 ` [pve-devel] superseded: [PATCH proxmox{-ve-rs, -firewall} v2 0/4] Fix ipfilters in proxmox-firewall Stefan Hanreich
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=640c74a9-a616-4a3c-99be-3a0e43f15e13@proxmox.com \
--to=s.hanreich@proxmox.com \
--cc=h.laimer@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox