[pve-devel] [PATCH proxmox-firewall 1/2] firewall: fix generating rules with both type and code set

[Stefan Hanreich]

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 at 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