[pve-devel] [PATCH proxmox-firewall v2 3/3] fix #6336: fix ipfilter matching logic
Hannes Laimer
h.laimer at proxmox.com
Wed Oct 1 09:31:54 CEST 2025
small typo, other than that LGTM!
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 at 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": "==",
More information about the pve-devel
mailing list