* [pve-devel] [PATCH proxmox-ve-rs v2 1/1] config: guest: store network devices in BTreeMap
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 ` Stefan Hanreich
2025-10-01 7:23 ` Hannes Laimer
2025-09-25 12:21 ` [pve-devel] [PATCH proxmox-firewall v2 1/3] run rustfmt Stefan Hanreich
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Stefan Hanreich @ 2025-09-25 12:21 UTC (permalink / raw)
To: pve-devel
Enables iterating over the network devices in order, without
performing any additional sorting at the call sites. This makes the
output in proxmox-firewall stable, which is useful for test cases as
well as for comparing the output of different proxmox-firewall runs.
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
proxmox-ve-config/src/guest/vm.rs | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/proxmox-ve-config/src/guest/vm.rs b/proxmox-ve-config/src/guest/vm.rs
index cc97781..baff1b8 100644
--- a/proxmox-ve-config/src/guest/vm.rs
+++ b/proxmox-ve-config/src/guest/vm.rs
@@ -1,4 +1,4 @@
-use std::collections::HashMap;
+use std::collections::BTreeMap;
use std::io;
use std::str::FromStr;
@@ -278,7 +278,7 @@ impl FromStr for NetworkDevice {
#[derive(Debug, Default)]
#[cfg_attr(test, derive(Eq, PartialEq))]
pub struct NetworkConfig {
- network_devices: HashMap<i64, NetworkDevice>,
+ network_devices: BTreeMap<i64, NetworkDevice>,
}
impl NetworkConfig {
@@ -300,12 +300,12 @@ impl NetworkConfig {
bail!("No index found in net key string: {key}")
}
- pub fn network_devices(&self) -> &HashMap<i64, NetworkDevice> {
+ pub fn network_devices(&self) -> &BTreeMap<i64, NetworkDevice> {
&self.network_devices
}
pub fn parse<R: io::BufRead>(input: R) -> Result<Self, Error> {
- let mut network_devices = HashMap::new();
+ let mut network_devices = BTreeMap::new();
for line in input.lines() {
let line = line?;
--
2.47.3
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [pve-devel] [PATCH proxmox-ve-rs v2 1/1] config: guest: store network devices in BTreeMap
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
0 siblings, 1 reply; 10+ messages in thread
From: Hannes Laimer @ 2025-10-01 7:23 UTC (permalink / raw)
To: Proxmox VE development discussion, Stefan Hanreich
On 9/25/25 14:22, Stefan Hanreich wrote:
> Enables iterating over the network devices in order, without
> performing any additional sorting at the call sites. This makes the
> output in proxmox-firewall stable, which is useful for test cases as
> well as for comparing the output of different proxmox-firewall runs.
>
Are we actually explicitly sorting anywhere? I didn't really find any
places. If there should be, we can drop those. Nonetheless this makes a
lot if sense.
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> proxmox-ve-config/src/guest/vm.rs | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/proxmox-ve-config/src/guest/vm.rs b/proxmox-ve-config/src/guest/vm.rs
> index cc97781..baff1b8 100644
> --- a/proxmox-ve-config/src/guest/vm.rs
> +++ b/proxmox-ve-config/src/guest/vm.rs
> @@ -1,4 +1,4 @@
> -use std::collections::HashMap;
> +use std::collections::BTreeMap;
> use std::io;
> use std::str::FromStr;
>
> @@ -278,7 +278,7 @@ impl FromStr for NetworkDevice {
> #[derive(Debug, Default)]
> #[cfg_attr(test, derive(Eq, PartialEq))]
> pub struct NetworkConfig {
> - network_devices: HashMap<i64, NetworkDevice>,
> + network_devices: BTreeMap<i64, NetworkDevice>,
> }
>
> impl NetworkConfig {
> @@ -300,12 +300,12 @@ impl NetworkConfig {
> bail!("No index found in net key string: {key}")
> }
>
> - pub fn network_devices(&self) -> &HashMap<i64, NetworkDevice> {
> + pub fn network_devices(&self) -> &BTreeMap<i64, NetworkDevice> {
> &self.network_devices
> }
>
> pub fn parse<R: io::BufRead>(input: R) -> Result<Self, Error> {
> - let mut network_devices = HashMap::new();
> + let mut network_devices = BTreeMap::new();
>
> for line in input.lines() {
> let line = line?;
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [pve-devel] [PATCH proxmox-ve-rs v2 1/1] config: guest: store network devices in BTreeMap
2025-10-01 7:23 ` Hannes Laimer
@ 2025-10-01 8:17 ` Stefan Hanreich
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Hanreich @ 2025-10-01 8:17 UTC (permalink / raw)
To: Hannes Laimer, Proxmox VE development discussion
On 10/1/25 9:22 AM, Hannes Laimer wrote:
> On 9/25/25 14:22, Stefan Hanreich wrote:
>> Enables iterating over the network devices in order, without
>> performing any additional sorting at the call sites. This makes the
>> output in proxmox-firewall stable, which is useful for test cases as
>> well as for comparing the output of different proxmox-firewall runs.
>>
>
> Are we actually explicitly sorting anywhere? I didn't really find any
> places. If there should be, we can drop those. Nonetheless this makes a
> lot if sense.
no, we're currently not - the value is used here [1] to generate the
ipfilter sets or here [2] for MAC address filtering. The generated JSON
for the ruleset is unstable if there's no sorting somewhere along the
line which leads to flakey integration tests.
I preferred BTreeMaps over HashMaps + sorting, since then the call site
doesn't need to remember sorting and if we have multiple users then
sorting only need to happen once.
[1]
https://git.proxmox.com/?p=proxmox-firewall.git;a=blob;f=proxmox-firewall/src/firewall.rs;h=8cac190170a8d69e6f2c9479390f7ec0fea50a88;hb=HEAD#l793
[2]
https://git.proxmox.com/?p=proxmox-firewall.git;a=blob;f=proxmox-firewall/src/firewall.rs;h=8cac190170a8d69e6f2c9479390f7ec0fea50a88;hb=HEAD#l598
>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>> ---
>> proxmox-ve-config/src/guest/vm.rs | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/proxmox-ve-config/src/guest/vm.rs b/proxmox-ve-config/
>> src/guest/vm.rs
>> index cc97781..baff1b8 100644
>> --- a/proxmox-ve-config/src/guest/vm.rs
>> +++ b/proxmox-ve-config/src/guest/vm.rs
>> @@ -1,4 +1,4 @@
>> -use std::collections::HashMap;
>> +use std::collections::BTreeMap;
>> use std::io;
>> use std::str::FromStr;
>> @@ -278,7 +278,7 @@ impl FromStr for NetworkDevice {
>> #[derive(Debug, Default)]
>> #[cfg_attr(test, derive(Eq, PartialEq))]
>> pub struct NetworkConfig {
>> - network_devices: HashMap<i64, NetworkDevice>,
>> + network_devices: BTreeMap<i64, NetworkDevice>,
>> }
>> impl NetworkConfig {
>> @@ -300,12 +300,12 @@ impl NetworkConfig {
>> bail!("No index found in net key string: {key}")
>> }
>> - pub fn network_devices(&self) -> &HashMap<i64, NetworkDevice> {
>> + pub fn network_devices(&self) -> &BTreeMap<i64, NetworkDevice> {
>> &self.network_devices
>> }
>> pub fn parse<R: io::BufRead>(input: R) -> Result<Self, Error> {
>> - let mut network_devices = HashMap::new();
>> + let mut network_devices = BTreeMap::new();
>> for line in input.lines() {
>> let line = line?;
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pve-devel] [PATCH proxmox-firewall v2 1/3] run rustfmt
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 ` Stefan Hanreich
2025-09-25 12:21 ` [pve-devel] [PATCH proxmox-firewall v2 2/3] ipfilter: fix wrong entries for containers Stefan Hanreich
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Stefan Hanreich @ 2025-09-25 12:21 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
proxmox-firewall/src/config.rs | 6 +++---
proxmox-firewall/src/firewall.rs | 6 +++---
proxmox-firewall/src/object.rs | 6 +++---
proxmox-firewall/src/rule.rs | 30 ++++++++++++++----------------
4 files changed, 23 insertions(+), 25 deletions(-)
diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs
index d6a4df5..8bd9f2a 100644
--- a/proxmox-firewall/src/config.rs
+++ b/proxmox-firewall/src/config.rs
@@ -3,7 +3,7 @@ use std::default::Default;
use std::fs::{self, DirEntry, File, ReadDir};
use std::io::{self, BufReader};
-use anyhow::{bail, format_err, Context, Error};
+use anyhow::{Context, Error, bail, format_err};
use proxmox_log as log;
@@ -17,10 +17,10 @@ use proxmox_ve_config::guest::types::Vmid;
use proxmox_ve_config::guest::{GuestEntry, GuestMap};
use proxmox_ve_config::host::types::BridgeName;
-use proxmox_network_api::{get_network_interfaces, AltnameMapping};
+use proxmox_network_api::{AltnameMapping, get_network_interfaces};
+use proxmox_nftables::NftClient;
use proxmox_nftables::command::{CommandOutput, Commands, List, ListOutput};
use proxmox_nftables::types::ListChain;
-use proxmox_nftables::NftClient;
use proxmox_ve_config::sdn::{
config::{RunningConfig, SdnConfig},
ipam::{Ipam, IpamJson},
diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs
index 8cac190..5012610 100644
--- a/proxmox-firewall/src/firewall.rs
+++ b/proxmox-firewall/src/firewall.rs
@@ -1,7 +1,7 @@
use std::collections::BTreeMap;
use std::fs;
-use anyhow::{Error, bail};
+use anyhow::{bail, Error};
use proxmox_log as log;
@@ -25,17 +25,17 @@ use proxmox_ve_config::firewall::guest::Config as GuestConfig;
use proxmox_ve_config::firewall::host::Config as HostConfig;
use proxmox_network_types::ip_address::{Cidr, Ipv6Cidr};
-use proxmox_ve_config::firewall::types::Group;
use proxmox_ve_config::firewall::types::ipset::{
Ipfilter, Ipset, IpsetEntry, IpsetName, IpsetScope,
};
use proxmox_ve_config::firewall::types::log::{LogLevel as ConfigLogLevel, LogRateLimit};
use proxmox_ve_config::firewall::types::rule::{Direction, Verdict as ConfigVerdict};
+use proxmox_ve_config::firewall::types::Group;
use proxmox_ve_config::guest::types::Vmid;
use crate::config::FirewallConfig;
use crate::object::{NftObjectEnv, ToNftObjects};
-use crate::rule::{NftRule, NftRuleEnv, generate_verdict};
+use crate::rule::{generate_verdict, NftRule, NftRuleEnv};
static CLUSTER_TABLE_NAME: &str = "proxmox-firewall";
static HOST_TABLE_NAME: &str = "proxmox-firewall";
diff --git a/proxmox-firewall/src/object.rs b/proxmox-firewall/src/object.rs
index a7575bb..42423b9 100644
--- a/proxmox-firewall/src/object.rs
+++ b/proxmox-firewall/src/object.rs
@@ -1,19 +1,19 @@
-use anyhow::{Error, format_err};
+use anyhow::{format_err, Error};
use proxmox_log as log;
use proxmox_nftables::{
- Command, Expression,
command::{Add, Flush},
expression::Prefix,
types::{
AddCtHelper, AddElement, CtHelperProtocol, ElementType, L3Protocol, SetConfig, SetFlag,
SetName, TablePart,
},
+ Command, Expression,
};
use proxmox_ve_config::{
firewall::{
ct_helper::CtHelperMacro,
- types::{Alias, Ipset, alias::AliasName, ipset::IpsetAddress},
+ types::{alias::AliasName, ipset::IpsetAddress, Alias, Ipset},
},
guest::types::Vmid,
};
diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs
index 77bc6ea..2512537 100644
--- a/proxmox-firewall/src/rule.rs
+++ b/proxmox-firewall/src/rule.rs
@@ -1,20 +1,19 @@
use std::ops::{Deref, DerefMut};
-use anyhow::{Error, bail, format_err};
+use anyhow::{bail, format_err, Error};
use proxmox_log as log;
use proxmox_nftables::{
- Expression, Statement,
expression::{Ct, IpFamily, Meta, Payload, Prefix},
statement::{Log, LogLevel, Match, Operator},
types::{AddRule, ChainPart, SetName, TableFamily, TablePart},
+ Expression, Statement,
};
use proxmox_ve_config::{
firewall::{
ct_helper::CtHelperMacro,
- fw_macros::{FwMacro, get_macro},
+ fw_macros::{get_macro, FwMacro},
types::{
- Alias, Rule,
alias::AliasName,
ipset::{Ipfilter, IpsetName},
log::LogRateLimit,
@@ -22,6 +21,7 @@ use proxmox_ve_config::{
rule_match::{
Icmp, Icmpv6, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Sctp, Tcp, Udp,
},
+ Alias, Rule,
},
},
guest::types::Vmid,
@@ -691,18 +691,16 @@ impl ToNftRules for Ipfilter<'_> {
if env.contains_family(Family::V4) {
base_rule.set_family(Family::V4);
- base_rule.append(&mut vec![
- Match::new_ne(
- Payload::field("arp", "saddr ip"),
- Expression::set_name(&SetName::ipset_name(
- Family::V4,
- self.ipset().name(),
- env.vmid,
- false,
- )),
- )
- .into(),
- ]);
+ base_rule.append(&mut vec![Match::new_ne(
+ Payload::field("arp", "saddr ip"),
+ Expression::set_name(&SetName::ipset_name(
+ Family::V4,
+ self.ipset().name(),
+ env.vmid,
+ false,
+ )),
+ )
+ .into()]);
rules.push(base_rule);
}
--
2.47.3
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* [pve-devel] [PATCH proxmox-firewall v2 2/3] ipfilter: fix wrong entries for containers
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 ` 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 16:30 ` [pve-devel] superseded: [PATCH proxmox{-ve-rs, -firewall} v2 0/4] Fix ipfilters in proxmox-firewall Stefan Hanreich
4 siblings, 0 replies; 10+ messages in thread
From: Stefan Hanreich @ 2025-09-25 12:21 UTC (permalink / raw)
To: pve-devel
The firewall used the CIDR from the container network configuration
for autogenerating the IP filter. If an IP of 192.0.2.1/24 was
configured, then the whole 192.0.2.0/24 range was allowed instead of
only 192.0.2.1 .
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
proxmox-firewall/src/firewall.rs | 10 +-
proxmox-firewall/tests/input/100.conf | 1 +
.../integration_tests__firewall.snap | 316 ++++++++++++++++++
3 files changed, 324 insertions(+), 3 deletions(-)
diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs
index 5012610..ddf839b 100644
--- a/proxmox-firewall/src/firewall.rs
+++ b/proxmox-firewall/src/firewall.rs
@@ -24,7 +24,7 @@ use proxmox_ve_config::firewall::ct_helper::get_cthelper;
use proxmox_ve_config::firewall::guest::Config as GuestConfig;
use proxmox_ve_config::firewall::host::Config as HostConfig;
-use proxmox_network_types::ip_address::{Cidr, Ipv6Cidr};
+use proxmox_network_types::ip_address::{Cidr, Ipv4Cidr, Ipv6Cidr};
use proxmox_ve_config::firewall::types::ipset::{
Ipfilter, Ipset, IpsetEntry, IpsetName, IpsetScope,
};
@@ -815,11 +815,15 @@ impl Firewall {
ipset.push(IpsetEntry::from(Cidr::from(cidr)));
if let Some(ip_address) = network_device.ip() {
- ipset.push(IpsetEntry::from(Cidr::from(ip_address)));
+ ipset.push(IpsetEntry::from(Cidr::from(Ipv4Cidr::from(
+ *ip_address.address(),
+ ))));
}
if let Some(ip6_address) = network_device.ip6() {
- ipset.push(IpsetEntry::from(Cidr::from(ip6_address)));
+ ipset.push(IpsetEntry::from(Cidr::from(Ipv6Cidr::from(
+ *ip6_address.address(),
+ ))));
}
commands.append(&mut ipset.to_nft_objects(&env)?);
diff --git a/proxmox-firewall/tests/input/100.conf b/proxmox-firewall/tests/input/100.conf
index cf9af7f..1f81186 100644
--- a/proxmox-firewall/tests/input/100.conf
+++ b/proxmox-firewall/tests/input/100.conf
@@ -5,6 +5,7 @@ hostname: host1
memory: 512
net1: name=eth0,bridge=simple1,firewall=1,hwaddr=BC:24:11:4D:B0:FF,ip=dhcp,ip6=fd80::1234/64,type=veth
net2: name=eth0,bridge=simple1,hwaddr=BC:24:11:4D:B0:FF,ip=dhcp,ip6=fd80::1234/64,type=veth
+net3: name=eth0,bridge=simple2,firewall=1,hwaddr=BC:24:11:4D:B0:FE,ip=192.0.2.10/24,ip6=fd80::1235/64,type=veth
ostype: debian
rootfs: local-lvm:vm-90001-disk-0,size=2G
swap: 512
diff --git a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
index e3db8ae..feeda5b 100644
--- a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
+++ b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
@@ -4384,6 +4384,306 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
}
}
},
+ {
+ "add": {
+ "set": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v4-guest-100/ipfilter-net3",
+ "type": "ipv4_addr",
+ "flags": [
+ "interval"
+ ]
+ }
+ }
+ },
+ {
+ "flush": {
+ "set": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v4-guest-100/ipfilter-net3"
+ }
+ }
+ },
+ {
+ "add": {
+ "set": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v4-guest-100/ipfilter-net3-nomatch",
+ "type": "ipv4_addr",
+ "flags": [
+ "interval"
+ ]
+ }
+ }
+ },
+ {
+ "flush": {
+ "set": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v4-guest-100/ipfilter-net3-nomatch"
+ }
+ }
+ },
+ {
+ "add": {
+ "element": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v4-guest-100/ipfilter-net3",
+ "elem": [
+ {
+ "prefix": {
+ "addr": "192.0.2.10",
+ "len": 32
+ }
+ }
+ ]
+ }
+ }
+ },
+ {
+ "add": {
+ "set": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v6-guest-100/ipfilter-net3",
+ "type": "ipv6_addr",
+ "flags": [
+ "interval"
+ ]
+ }
+ }
+ },
+ {
+ "flush": {
+ "set": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v6-guest-100/ipfilter-net3"
+ }
+ }
+ },
+ {
+ "add": {
+ "set": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v6-guest-100/ipfilter-net3-nomatch",
+ "type": "ipv6_addr",
+ "flags": [
+ "interval"
+ ]
+ }
+ }
+ },
+ {
+ "flush": {
+ "set": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v6-guest-100/ipfilter-net3-nomatch"
+ }
+ }
+ },
+ {
+ "add": {
+ "element": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "name": "v6-guest-100/ipfilter-net3",
+ "elem": [
+ {
+ "prefix": {
+ "addr": "fe80::be24:11ff:fe4d:b0fe",
+ "len": 128
+ }
+ },
+ {
+ "prefix": {
+ "addr": "fd80::1235",
+ "len": 128
+ }
+ }
+ ]
+ }
+ }
+ },
+ {
+ "add": {
+ "rule": {
+ "family": "bridge",
+ "table": "proxmox-firewall-guests",
+ "chain": "guest-100-in",
+ "expr": [
+ {
+ "match": {
+ "op": "==",
+ "left": {
+ "meta": {
+ "key": "oifname"
+ }
+ },
+ "right": "veth100i3"
+ }
+ },
+ {
+ "match": {
+ "op": "!=",
+ "left": {
+ "payload": {
+ "protocol": "arp",
+ "field": "daddr ip"
+ }
+ },
+ "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": "!=",
+ "left": {
+ "payload": {
+ "protocol": "ip",
+ "field": "saddr"
+ }
+ },
+ "right": "@v4-guest-100/ipfilter-net3"
+ }
+ },
+ {
+ "match": {
+ "op": "==",
+ "left": {
+ "payload": {
+ "protocol": "ip",
+ "field": "saddr"
+ }
+ },
+ "right": "@v4-guest-100/ipfilter-net3-nomatch"
+ }
+ },
+ {
+ "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": "!=",
+ "left": {
+ "payload": {
+ "protocol": "ip6",
+ "field": "saddr"
+ }
+ },
+ "right": "@v6-guest-100/ipfilter-net3"
+ }
+ },
+ {
+ "match": {
+ "op": "==",
+ "left": {
+ "payload": {
+ "protocol": "ip6",
+ "field": "saddr"
+ }
+ },
+ "right": "@v6-guest-100/ipfilter-net3-nomatch"
+ }
+ },
+ {
+ "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": "!=",
+ "left": {
+ "payload": {
+ "protocol": "arp",
+ "field": "saddr ip"
+ }
+ },
+ "right": "@v4-guest-100/ipfilter-net3"
+ }
+ },
+ {
+ "drop": null
+ }
+ ]
+ }
+ }
+ },
{
"add": {
"rule": {
@@ -4742,6 +5042,14 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
"target": "guest-100-in"
}
}
+ ],
+ [
+ "veth100i3",
+ {
+ "goto": {
+ "target": "guest-100-in"
+ }
+ }
]
]
}
@@ -4886,6 +5194,14 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
"target": "guest-100-out"
}
}
+ ],
+ [
+ "veth100i3",
+ {
+ "goto": {
+ "target": "guest-100-out"
+ }
+ }
]
]
}
--
2.47.3
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* [pve-devel] [PATCH proxmox-firewall v2 3/3] fix #6336: fix ipfilter matching logic
2025-09-25 12:21 [pve-devel] [PATCH proxmox{-ve-rs, -firewall} v2 0/4] Fix ipfilters in proxmox-firewall Stefan Hanreich
` (2 preceding siblings ...)
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
2025-10-01 7:31 ` Hannes Laimer
2025-10-01 16:30 ` [pve-devel] superseded: [PATCH proxmox{-ve-rs, -firewall} v2 0/4] Fix ipfilters in proxmox-firewall Stefan Hanreich
4 siblings, 1 reply; 10+ messages in thread
From: Stefan Hanreich @ 2025-09-25 12:21 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [pve-devel] [PATCH proxmox-firewall v2 3/3] fix #6336: fix ipfilter matching logic
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
0 siblings, 1 reply; 10+ messages in thread
From: Hannes Laimer @ 2025-10-01 7:31 UTC (permalink / raw)
To: Proxmox VE development discussion, Stefan Hanreich
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@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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [pve-devel] [PATCH proxmox-firewall v2 3/3] fix #6336: fix ipfilter matching logic
2025-10-01 7:31 ` Hannes Laimer
@ 2025-10-01 8:11 ` Stefan Hanreich
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Hanreich @ 2025-10-01 8:11 UTC (permalink / raw)
To: Hannes Laimer, Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pve-devel] superseded: [PATCH proxmox{-ve-rs, -firewall} v2 0/4] Fix ipfilters in proxmox-firewall
2025-09-25 12:21 [pve-devel] [PATCH proxmox{-ve-rs, -firewall} v2 0/4] Fix ipfilters in proxmox-firewall Stefan Hanreich
` (3 preceding siblings ...)
2025-09-25 12:21 ` [pve-devel] [PATCH proxmox-firewall v2 3/3] fix #6336: fix ipfilter matching logic Stefan Hanreich
@ 2025-10-01 16:30 ` Stefan Hanreich
4 siblings, 0 replies; 10+ messages in thread
From: Stefan Hanreich @ 2025-10-01 16:30 UTC (permalink / raw)
To: pve-devel
https://lore.proxmox.com/pve-devel/20251001162818.320717-1-s.hanreich@proxmox.com/T/#m96f63f1eaf21149b7140dec51b82bf3d97942c97
On 9/25/25 2:21 PM, Stefan Hanreich wrote:
> This patch series addresses two issues with ipfilters:
>
> * containers would have the wrong CIDR inserted into the auto-generated ipfilter
> ipsets
> * The nomatch logic isn't working correctly, due to wrong inversion of logic,
> leading to ipfilters not working at all
>
> Including the rustfmt patch here as well, instead of separately since we touch
> some of the imports that get changed there - leading to conflicts on applying
> otherwise.
>
> Changes from v1:
> * properly regenerate test-output with the proxmox-ve-rs patch applied
> * improve documentation of handle_set and handle_ipfilter
>
> proxmox-ve-rs:
>
> Stefan Hanreich (1):
> config: guest: store network devices in BTreeMap
>
> proxmox-ve-config/src/guest/vm.rs | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>
> proxmox-firewall:
>
> Stefan Hanreich (3):
> run rustfmt
> ipfilter: fix wrong entries for containers
> fix #6336: fix ipfilter matching logic
>
> proxmox-firewall/src/config.rs | 6 +-
> proxmox-firewall/src/firewall.rs | 16 +-
> proxmox-firewall/src/object.rs | 6 +-
> proxmox-firewall/src/rule.rs | 161 +++++--
> proxmox-firewall/tests/input/100.conf | 1 +
> .../integration_tests__firewall.snap | 416 ++++++++++++++++++
> 6 files changed, 565 insertions(+), 41 deletions(-)
>
>
> Summary over all repositories:
> 7 files changed, 569 insertions(+), 45 deletions(-)
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 10+ messages in thread