* [pve-devel] [PATCH proxmox-firewall 1/2] firewall: fix generating rules with both type and code set
2025-10-08 13:14 [pve-devel] [PATCH proxmox-firewall 0/2] Fix generating rules with both ICMP type and code set Stefan Hanreich
@ 2025-10-08 13:14 ` Stefan Hanreich
2025-10-08 13:14 ` [pve-devel] [PATCH proxmox-firewall 2/2] firewall: regenerate snapshot for tests Stefan Hanreich
1 sibling, 0 replies; 3+ messages in thread
From: Stefan Hanreich @ 2025-10-08 13:14 UTC (permalink / raw)
To: pve-devel
If a rule had both type and code set, then proxmox-firewall would only
generate a rule that matched on the specified icmp code, but not the
icmp type. This means that a code was blocked for every ICMP type.
Change up the order of type and code as well, such that the statement
matching on the type is first. This makes the generated rules easier
to read and more logical sense (semantics of codes are dependent on
the type).
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
proxmox-firewall/src/rule.rs | 32 +++++++++++++++----------
proxmox-firewall/tests/input/cluster.fw | 3 +++
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs
index 07f7924..0a4f110 100644
--- a/proxmox-firewall/src/rule.rs
+++ b/proxmox-firewall/src/rule.rs
@@ -651,17 +651,21 @@ impl ToNftRules for Icmp {
fn to_nft_rules(&self, rules: &mut Vec<NftRule>, _env: &NftRuleEnv) -> Result<(), Error> {
for rule in rules.iter_mut() {
if matches!(rule.family(), Some(Family::V4) | None) {
- if let Some(icmp_code) = self.code() {
+ if let Some(icmp_type) = self.ty() {
rule.push(
- Match::new_eq(Payload::field("icmp", "code"), Expression::from(icmp_code))
+ Match::new_eq(Payload::field("icmp", "type"), Expression::from(icmp_type))
.into(),
);
- } else if let Some(icmp_type) = self.ty() {
+ }
+
+ if let Some(icmp_code) = self.code() {
rule.push(
- Match::new_eq(Payload::field("icmp", "type"), Expression::from(icmp_type))
+ Match::new_eq(Payload::field("icmp", "code"), Expression::from(icmp_code))
.into(),
);
- } else {
+ }
+
+ if self.code().is_none() && self.ty().is_none() {
rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from("icmp")).into());
}
@@ -679,23 +683,27 @@ impl ToNftRules for Icmpv6 {
for rule in rules.iter_mut() {
if matches!(rule.family(), Some(Family::V6) | None) {
- if let Some(icmp_code) = self.code() {
+ if let Some(icmp_type) = self.ty() {
rule.push(
Match::new_eq(
- Payload::field("icmpv6", "code"),
- Expression::from(icmp_code),
+ Payload::field("icmpv6", "type"),
+ Expression::from(icmp_type),
)
.into(),
);
- } else if let Some(icmp_type) = self.ty() {
+ }
+
+ if let Some(icmp_code) = self.code() {
rule.push(
Match::new_eq(
- Payload::field("icmpv6", "type"),
- Expression::from(icmp_type),
+ Payload::field("icmpv6", "code"),
+ Expression::from(icmp_code),
)
.into(),
);
- } else {
+ }
+
+ if self.code().is_none() && self.ty().is_none() {
rule.push(
Match::new_eq(Meta::new("l4proto"), Expression::from("icmpv6")).into(),
);
diff --git a/proxmox-firewall/tests/input/cluster.fw b/proxmox-firewall/tests/input/cluster.fw
index 4f50cc1..c3460c8 100644
--- a/proxmox-firewall/tests/input/cluster.fw
+++ b/proxmox-firewall/tests/input/cluster.fw
@@ -22,6 +22,9 @@ dc/network1
GROUP network1 -i eth0
IN ACCEPT -log nolog
IN ACCEPT -dest +network1
+FORWARD DROP -p ipv6-icmp -log nolog -icmp-type ttl-zero-during-transit
+IN ACCEPT -p icmp -log nolog -icmp-type host-unreachable
+OUT REJECT -p icmp -log nolog -icmp-type router-solicitation
[group network1]
--
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] 3+ messages in thread* [pve-devel] [PATCH proxmox-firewall 2/2] firewall: regenerate snapshot for tests
2025-10-08 13:14 [pve-devel] [PATCH proxmox-firewall 0/2] Fix generating rules with both ICMP type and code set Stefan Hanreich
2025-10-08 13:14 ` [pve-devel] [PATCH proxmox-firewall 1/2] firewall: fix generating rules with both " Stefan Hanreich
@ 2025-10-08 13:14 ` Stefan Hanreich
1 sibling, 0 replies; 3+ messages in thread
From: Stefan Hanreich @ 2025-10-08 13:14 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
.../integration_tests__firewall.snap | 104 ++++++++++++++++++
1 file changed, 104 insertions(+)
diff --git a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
index dcb9531..9751134 100644
--- a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
+++ b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
@@ -2076,6 +2076,44 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
}
}
},
+ {
+ "add": {
+ "rule": {
+ "family": "inet",
+ "table": "proxmox-firewall",
+ "chain": "cluster-in",
+ "expr": [
+ {
+ "match": {
+ "op": "==",
+ "left": {
+ "payload": {
+ "protocol": "icmp",
+ "field": "type"
+ }
+ },
+ "right": 3
+ }
+ },
+ {
+ "match": {
+ "op": "==",
+ "left": {
+ "payload": {
+ "protocol": "icmp",
+ "field": "code"
+ }
+ },
+ "right": 1
+ }
+ },
+ {
+ "accept": null
+ }
+ ]
+ }
+ }
+ },
{
"add": {
"rule": {
@@ -2141,6 +2179,34 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
}
}
},
+ {
+ "add": {
+ "rule": {
+ "family": "inet",
+ "table": "proxmox-firewall",
+ "chain": "cluster-out",
+ "expr": [
+ {
+ "match": {
+ "op": "==",
+ "left": {
+ "payload": {
+ "protocol": "icmp",
+ "field": "type"
+ }
+ },
+ "right": "router-solicitation"
+ }
+ },
+ {
+ "jump": {
+ "target": "do-reject"
+ }
+ }
+ ]
+ }
+ }
+ },
{
"add": {
"rule": {
@@ -2179,6 +2245,44 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
}
}
},
+ {
+ "add": {
+ "rule": {
+ "family": "inet",
+ "table": "proxmox-firewall",
+ "chain": "cluster-forward",
+ "expr": [
+ {
+ "match": {
+ "op": "==",
+ "left": {
+ "payload": {
+ "protocol": "icmpv6",
+ "field": "type"
+ }
+ },
+ "right": 3
+ }
+ },
+ {
+ "match": {
+ "op": "==",
+ "left": {
+ "payload": {
+ "protocol": "icmpv6",
+ "field": "code"
+ }
+ },
+ "right": 0
+ }
+ },
+ {
+ "drop": null
+ }
+ ]
+ }
+ }
+ },
{
"add": {
"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] 3+ messages in thread