* [pve-devel] [PATCH proxmox{-ve-rs, -firewall} v2 0/4] Fix ipfilters in proxmox-firewall
@ 2025-09-25 12:21 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
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Stefan Hanreich @ 2025-09-25 12:21 UTC (permalink / raw)
To: pve-devel
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(-)
--
Generated by git-murpp 0.8.0
_______________________________________________
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-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
* [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-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-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
* 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] 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
end of thread, other threads:[~2025-10-01 16:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2025-10-01 16:30 ` [pve-devel] superseded: [PATCH proxmox{-ve-rs, -firewall} v2 0/4] Fix ipfilters in proxmox-firewall Stefan Hanreich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox