[pve-devel] [PATCH proxmox-firewall v2 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains

[pve-devel] [PATCH proxmox-firewall v2 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains

[Hannes Laimer]

... on the guest table. There is no reason to not repect that option
on those two chains. These two were missed in the referenced commit.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
Fixes: 64dc344b ("firewall: apply `nt_conntrack_allow_invalid` option to guest table")
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
no changes since v1, so I kept @Stefans T-b and R-b

 proxmox-firewall/resources/proxmox-firewall.nft | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft
index 2dd7c48..30f7b4f 100644
--- a/proxmox-firewall/resources/proxmox-firewall.nft
+++ b/proxmox-firewall/resources/proxmox-firewall.nft
@@ -356,7 +356,7 @@ table bridge proxmox-firewall-guests {
     }
 
     chain pre-vm-out {
-        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
+        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
     }
 
     chain vm-out {
@@ -384,7 +384,7 @@ table bridge proxmox-firewall-guests {
 
     chain before-bridge {
         meta protocol arp accept
-        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
+        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
     }
 
     chain forward {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-firewall v2 2/2] firewall: apply `nt_conntrack_allow_invalid` option to host table

[Hannes Laimer]

... on all chains that check for ct state. Since we support this option,
we should also use it in our firewall rule generation.

This is a follow-up to
  64dc344b ("firewall: apply `nt_conntrack_allow_invalid` option to guest table")

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
Thanks @Stefan for the review and explanation on v1!

v2:
 - also drop ct state invalid in `default-in` by default, unless
   allow_invalid option is set

 .../resources/proxmox-firewall.nft            | 17 ++++++--------
 proxmox-firewall/src/firewall.rs              | 11 ++++++---
 .../integration_tests__firewall.snap          | 23 ++++++++++++-------
 3 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft
index 30f7b4f..2456336 100644
--- a/proxmox-firewall/resources/proxmox-firewall.nft
+++ b/proxmox-firewall/resources/proxmox-firewall.nft
@@ -14,7 +14,6 @@ add chain inet proxmox-firewall allow-ndp-in
 add chain inet proxmox-firewall block-ndp-in
 add chain inet proxmox-firewall allow-ndp-out
 add chain inet proxmox-firewall block-ndp-out
-add chain inet proxmox-firewall block-conntrack-invalid
 add chain inet proxmox-firewall block-smurfs
 add chain inet proxmox-firewall allow-icmp
 add chain inet proxmox-firewall log-drop-smurfs
@@ -55,7 +54,6 @@ flush chain inet proxmox-firewall allow-ndp-in
 flush chain inet proxmox-firewall block-ndp-in
 flush chain inet proxmox-firewall allow-ndp-out
 flush chain inet proxmox-firewall block-ndp-out
-flush chain inet proxmox-firewall block-conntrack-invalid
 flush chain inet proxmox-firewall block-smurfs
 flush chain inet proxmox-firewall allow-icmp
 flush chain inet proxmox-firewall log-drop-smurfs
@@ -176,10 +174,6 @@ table inet proxmox-firewall {
         icmpv6 type { nd-router-solicit, nd-neighbor-solicit, nd-neighbor-advert } drop
     }
 
-    chain block-conntrack-invalid {
-        ct state invalid drop
-    }
-
     chain block-smurfs {
         ip saddr 0.0.0.0/32 return
         meta pkttype broadcast goto log-drop-smurfs
@@ -205,7 +199,7 @@ table inet proxmox-firewall {
         iifname "lo" accept
 
         jump allow-icmp
-        ct state related,established accept
+        ct state vmap { invalid : jump invalid-conntrack, established : accept, related : accept }
 
         meta l4proto igmp accept
 
@@ -229,7 +223,7 @@ table inet proxmox-firewall {
         oifname "lo" accept
 
         jump allow-icmp
-        ct state vmap { invalid : drop, established : accept, related : accept }
+        ct state vmap { invalid : jump invalid-conntrack, established : accept, related : accept }
     }
 
     chain option-in {}
@@ -241,7 +235,7 @@ table inet proxmox-firewall {
 
     chain before-bridge {
         meta protocol arp accept
-        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
+        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
     }
 
     chain host-bridge-input {
@@ -284,9 +278,12 @@ table inet proxmox-firewall {
     chain host-out {}
 
     chain cluster-forward {}
-    chain host-forward {}
+    chain host-forward {
+        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
+    }
 
     chain ct-in {}
+    chain invalid-conntrack { }
 }
 
 table bridge proxmox-firewall-guests {
diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs
index 88fb460..607fc75 100644
--- a/proxmox-firewall/src/firewall.rs
+++ b/proxmox-firewall/src/firewall.rs
@@ -99,6 +99,10 @@ impl Firewall {
         ChainPart::new(Self::guest_table(), "invalid-conntrack".to_string())
     }
 
+    fn host_invalid_conntrack_chain() -> ChainPart {
+        ChainPart::new(Self::host_table(), "invalid-conntrack".to_string())
+    }
+
     fn host_conntrack_chain() -> ChainPart {
         ChainPart::new(Self::host_table(), "ct-in".to_string())
     }
@@ -144,6 +148,7 @@ impl Firewall {
             Flush::chain(Self::host_option_chain(Direction::Out)),
             Flush::chain(Self::host_chain(Direction::Forward)),
             Flush::chain(Self::guest_invalid_conntrack_chain()),
+            Flush::chain(Self::host_invalid_conntrack_chain()),
             Flush::map(Self::guest_vmap(Direction::In)),
             Flush::map(Self::guest_vmap(Direction::Out)),
             Flush::map(Self::bridge_vmap(Self::guest_table())),
@@ -533,12 +538,12 @@ impl Firewall {
             log::debug!("set block_invalid_conntrack");
 
             commands.push(Add::rule(AddRule::from_statement(
-                chain_in,
-                Statement::jump("block-conntrack-invalid"),
+                Self::guest_invalid_conntrack_chain(),
+                Statement::make_drop(),
             )));
 
             commands.push(Add::rule(AddRule::from_statement(
-                Self::guest_invalid_conntrack_chain(),
+                Self::host_invalid_conntrack_chain(),
                 Statement::make_drop(),
             )));
         }
diff --git a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
index 9194fc6..24f66a5 100644
--- a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
+++ b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
@@ -104,6 +104,15 @@ snapshot_kind: text
         }
       }
     },
+    {
+      "flush": {
+        "chain": {
+          "family": "inet",
+          "table": "proxmox-firewall",
+          "name": "invalid-conntrack"
+        }
+      }
+    },
     {
       "flush": {
         "map": {
@@ -3280,14 +3289,12 @@ snapshot_kind: text
     {
       "add": {
         "rule": {
-          "family": "inet",
-          "table": "proxmox-firewall",
-          "chain": "option-in",
+          "family": "bridge",
+          "table": "proxmox-firewall-guests",
+          "chain": "invalid-conntrack",
           "expr": [
             {
-              "jump": {
-                "target": "block-conntrack-invalid"
-              }
+              "drop": null
             }
           ]
         }
@@ -3296,8 +3303,8 @@ snapshot_kind: text
     {
       "add": {
         "rule": {
-          "family": "bridge",
-          "table": "proxmox-firewall-guests",
+          "family": "inet",
+          "table": "proxmox-firewall",
           "chain": "invalid-conntrack",
           "expr": [
             {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox-firewall v2 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains

[Stefan Hanreich]

gave this a test on my machine:
* tested outgoing/incoming connectivity for guests
* tested DHCP in a simple zone
* checked generated firewall rulesets with setting on/off

small nit: settings is called nf_conntrack_allow_invalid, not
nt_conntrack_allow_invalid - maybe we could change that on commit?

consider this:
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>

On 3/12/25 14:20, Hannes Laimer wrote:
> ... on the guest table. There is no reason to not repect that option
> on those two chains. These two were missed in the referenced commit.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> Fixes: 64dc344b ("firewall: apply `nt_conntrack_allow_invalid` option to guest table")
> Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
> Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> no changes since v1, so I kept @Stefans T-b and R-b
> 
>  proxmox-firewall/resources/proxmox-firewall.nft | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft
> index 2dd7c48..30f7b4f 100644
> --- a/proxmox-firewall/resources/proxmox-firewall.nft
> +++ b/proxmox-firewall/resources/proxmox-firewall.nft
> @@ -356,7 +356,7 @@ table bridge proxmox-firewall-guests {
>      }
>  
>      chain pre-vm-out {
> -        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
> +        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
>      }
>  
>      chain vm-out {
> @@ -384,7 +384,7 @@ table bridge proxmox-firewall-guests {
>  
>      chain before-bridge {
>          meta protocol arp accept
> -        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
> +        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
>      }
>  
>      chain forward {



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied-series: [PATCH proxmox-firewall v2 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains

[Wolfgang Bumiller]

applied and s/nt_/nf_/ (except where referencing existing commit, to not
mess up copy&paste sesarches)

On Wed, Mar 12, 2025 at 02:20:24PM +0100, Hannes Laimer wrote:
> ... on the guest table. There is no reason to not repect that option
> on those two chains. These two were missed in the referenced commit.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> Fixes: 64dc344b ("firewall: apply `nt_conntrack_allow_invalid` option to guest table")
> Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
> Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> no changes since v1, so I kept @Stefans T-b and R-b
> 
>  proxmox-firewall/resources/proxmox-firewall.nft | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft
> index 2dd7c48..30f7b4f 100644
> --- a/proxmox-firewall/resources/proxmox-firewall.nft
> +++ b/proxmox-firewall/resources/proxmox-firewall.nft
> @@ -356,7 +356,7 @@ table bridge proxmox-firewall-guests {
>      }
>  
>      chain pre-vm-out {
> -        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
> +        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
>      }
>  
>      chain vm-out {
> @@ -384,7 +384,7 @@ table bridge proxmox-firewall-guests {
>  
>      chain before-bridge {
>          meta protocol arp accept
> -        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
> +        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
>      }
>  
>      chain forward {
> -- 
> 2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel