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 <[email protected]> >> --- >> 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 [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
