public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
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

  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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal