[pve-devel] [PATCH proxmox-firewall v2 1/1] service: flush firewall rules on force disable

[Gabriel Goller]

On 04.07.2024 14:36, Stefan Hanreich wrote:
>When disabling the nftables firewall again, there is a race condition
>where the nftables ruleset never gets flushed and persists after
>disabling.
>
>The nftables firewall update loop does a noop when the force disable
>file exists. It only flushes the ruleset when nftables is disabled in
>the configuration file but the force disable file does not yet exist.
>
>This can lead to the following situation:
>
>* nftables is activated and created its ruleset
>* user switches from nftables firewall back to iptables firewall
>* pve-firewall runs and creates the force disable file
>* proxmox-firewall sees that the file exists and does nothing
>
>Reported-by: Hannes Laimer <h.laimer at proxmox.com>
>Signed-off-by: Stefan Hanreich <s.hanreich at proxmox.com>
>---
>Changes from v1 to v2:
>* Removed misleading/wrong section about the probability of this
>  happening
>* Added a detailed description of the scenario this commit prevents
>
> proxmox-firewall/src/bin/proxmox-firewall.rs | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
>index f7e816e..5133cbf 100644
>--- a/proxmox-firewall/src/bin/proxmox-firewall.rs
>+++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
>@@ -91,6 +91,10 @@ fn main() -> Result<(), std::io::Error> {
>
>     while !term.load(Ordering::Relaxed) {
>         if force_disable_flag.exists() {
>+            if let Err(error) = remove_firewall() {
>+                log::error!("unable to disable firewall: {error:#}");

Note that `std::io::Error` does not handle pretty-printing any different
in `Display`. So this outputs the same as '{error}'. To also print the
`ErrorKind` use either '{error:?}' or '{error:#?}', which produce:

     Custom { kind: NotFound, error: "cool" }

or

     Custom {
         kind: NotFound,
         error: "cool",
     }


>+            }
>+
>             std::thread::sleep(Duration::from_secs(5));
>             continue;
>         }
>-- 
>2.39.2
>
>
>_______________________________________________
>pve-devel mailing list
>pve-devel at lists.proxmox.com
>https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>