[PATCH firewall/proxmox-firewall v2 0/2] fix bridge-nf-call settings during upgrades and handover

[PATCH firewall/proxmox-firewall v2 0/2] fix bridge-nf-call settings during upgrades and handover

[Stefan Hanreich]

pve-firewall needs net.bridge.bridge-nf-call-ip{,6}tables set to 1, so
that bridged guest traffic passes through iptables and conntrack. It
sets them at runtime, while 10-pve.conf sets them to 0. Whenever the
sysctl.d configuration is re-applied, e.g. when systemd-sysctl is
restarted during a package upgrade, they drop back to 0 until the next
update round of pve-firewall, up to 10 seconds later. Conntrack misses
the bridged traffic in between, so packets of established connections
get dropped as invalid afterwards.

The pve-firewall patch writes a drop-in to /run/sysctl.d while the
iptables based firewall is active, so re-applying the configuration
keeps the values at 1.

The proxmox-firewall patch fixes a related issue when switching to the
nftables based firewall at runtime. It does not need the sysctls, but
nothing reset them either, so the host and cluster forward rules also
applied to bridged guest traffic until the next re-apply. It now
re-applies the configured values once when it takes over, which keeps
any override by the admin.

Changes from v1 -> v2:
* fixed pve-common dependency to require the proper version for file_exists


pve-firewall:

Stefan Hanreich (1):
  fix #8268: firewall: keep bridge-nf-call sysctls set on upgrade

 debian/control      |  4 ++--
 src/PVE/Firewall.pm | 34 +++++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 3 deletions(-)


proxmox-firewall:

Stefan Hanreich (1):
  service: re-apply bridge sysctls when taking over from pve-firewall

 proxmox-firewall/src/bin/proxmox-firewall.rs | 35 ++++++++++++++++++++
 1 file changed, 35 insertions(+)


Summary over all repositories:
  3 files changed, 70 insertions(+), 3 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH pve-firewall v2 1/2] fix #8268: firewall: keep bridge-nf-call sysctls set on upgrade

[Stefan Hanreich]

Restarting systemd-sysctl, as done on some package upgrades, or
loading br_netfilter re-applies the sysctl.d configuration. This
resets bridge-nf-call-ip{,6}tables to the 0 from 10-pve.conf until
the next update round enables them again. Bridged traffic bypasses
conntrack in that window, so conntrack loses track of established
connections and drops their packets as invalid afterwards.

Write a drop-in to /run/sysctl.d while the iptables based firewall is
active, so re-applying the configuration keeps the values. Only remove
it once the firewall is disabled or switched to nftables, not on
daemon shutdown, as the daemon itself may be restarted during the same
upgrade.

The nftables based proxmox-firewall does not need the drop-in. It
filters bridged guest traffic in a bridge table, which has its own
conntrack support, so it does not need br_netfilter. Remove the
drop-in before handing over to proxmox-firewall, so a re-apply after
the takeover cannot pick up its values.

The new code uses PVE::File directly, including file_exists, which is
available since libpve-common-perl 9.1.8, so raise the dependency
accordingly.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=8268
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 debian/control      |  4 ++--
 src/PVE/Firewall.pm | 34 +++++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/debian/control b/debian/control
index 7fbbe33..1128d8e 100644
--- a/debian/control
+++ b/debian/control
@@ -9,7 +9,7 @@ Build-Depends: debhelper-compat (= 13),
                libnetfilter-log-dev,
                libpve-access-control,
                libpve-cluster-perl,
-               libpve-common-perl (>= 7.3-2),
+               libpve-common-perl (>= 9.1.8),
                proxmox-firewall-data (>= 0.1),
                pve-cluster (>= 6.0-4),
                pve-doc-generator (>= 5.3-3),
@@ -24,7 +24,7 @@ Depends: conntrack,
          iptables,
          libpve-access-control,
          libpve-cluster-perl,
-         libpve-common-perl (>= 9.0.2),
+         libpve-common-perl (>= 9.1.8),
          libpve-network-perl (>= 1.6.8~),
          libpve-rs-perl (>= 0.8.13),
          proxmox-firewall-data (>= 0.1),
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index dd5d92c..082e916 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -17,6 +17,7 @@ use Storable qw(dclone);
 use PVE::Cluster;
 use PVE::Corosync;
 use PVE::Exception qw(raise raise_param_exc);
+use PVE::File;
 use PVE::INotify;
 use PVE::JSONSchema qw(register_standard_option get_standard_option);
 use PVE::Network;
@@ -2110,8 +2111,34 @@ sub rules_audit_permissions {
 
 # core functions
 
+my $bridge_sysctl_dropin_dir = "/run/sysctl.d";
+# must sort after 10-pve.conf, and must not reuse a name from another sysctl.d directory, which
+# would mask that file
+my $bridge_sysctl_dropin = "$bridge_sysctl_dropin_dir/90-pve-firewall-bridge.conf";
+
+my sub write_bridge_sysctl_dropin {
+    return if PVE::File::file_exists($bridge_sysctl_dropin);
+
+    eval {
+        File::Path::make_path($bridge_sysctl_dropin_dir);
+        PVE::File::file_set_contents(
+            $bridge_sysctl_dropin,
+            "net.bridge.bridge-nf-call-iptables = 1\nnet.bridge.bridge-nf-call-ip6tables = 1\n",
+        );
+    };
+    warn "failed to write '$bridge_sysctl_dropin' - $@" if $@;
+}
+
+my sub remove_bridge_sysctl_dropin {
+    if (!unlink($bridge_sysctl_dropin)) {
+        warn "failed to unlink '$bridge_sysctl_dropin' - $!\n" if !$!{ENOENT};
+    }
+}
+
 sub enable_bridge_firewall {
 
+    write_bridge_sysctl_dropin();
+
     PVE::ProcFSTools::write_proc_entry("/proc/sys/net/bridge/bridge-nf-call-iptables", "1");
     PVE::ProcFSTools::write_proc_entry("/proc/sys/net/bridge/bridge-nf-call-ip6tables", "1");
 
@@ -5573,10 +5600,15 @@ my sub is_enabled_and_not_nftables {
     $host_conf = load_hostfw_conf($cluster_conf) if !defined($host_conf);
 
     my $is_nftables = get_nftables_option($cluster_conf, $host_conf);
+    my $enabled_and_not_nftables = $cluster_conf->{options}->{enable} && !$is_nftables;
+
+    # remove the drop-in before handing over, as proxmox-firewall re-applies the bridge sysctls
+    # once it takes over
+    remove_bridge_sysctl_dropin() if !$enabled_and_not_nftables;
 
     update_force_nftables_disable_flag($cluster_conf->{options}->{enable}, $is_nftables);
 
-    return $cluster_conf->{options}->{enable} && !$is_nftables;
+    return $enabled_and_not_nftables;
 }
 
 sub init {
-- 
2.47.3

[PATCH proxmox-firewall v2 2/2] service: re-apply bridge sysctls when taking over from pve-firewall

[Stefan Hanreich]

While active, the iptables based pve-firewall sets
bridge-nf-call-ip{,6}tables to 1. Switching to nftables at runtime
leaves them at that value until something re-applies the sysctl.d
configuration, usually the next package upgrade. Until then
br_netfilter also passes bridged guest traffic to the inet table, so
the host and cluster forward rules apply to it on top of the guest
rules.

Re-apply the configured values once each time pve-firewall hands over
by removing the force-disable flag, and on daemon start. pve-firewall
only sets the values while the flag exists. Checking the flag instead
of the parsed configuration means an invalid entry in any firewall
config cannot keep the values at 1.

Without an override by the admin, this resets them to the 0 from
10-pve.conf. Re-applying the configuration instead of writing 0 keeps
such overrides. pve-firewall removes its drop-in for these sysctls
before handing over, so it does not affect the re-apply.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 proxmox-firewall/src/bin/proxmox-firewall.rs | 35 ++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
index e8ed477..f0b4d15 100644
--- a/proxmox-firewall/src/bin/proxmox-firewall.rs
+++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
@@ -46,6 +46,30 @@ fn create_firewall_instance() -> Result<Firewall, Error> {
     Ok(Firewall::new(config))
 }
 
+/// Re-applies the sysctl.d configuration of the bridge-nf-call sysctls.
+///
+/// The legacy pve-firewall sets them to 1 while it is active. After switching over, br_netfilter
+/// would then keep passing bridged guest traffic to the inet table as well, until something else
+/// re-applies the configuration. Re-applying it instead of writing 0 keeps overrides of the admin.
+fn reapply_bridge_sysctls() -> Result<(), Error> {
+    let output = std::process::Command::new("/usr/lib/systemd/systemd-sysctl")
+        .args([
+            "--prefix=net.bridge.bridge-nf-call-iptables",
+            "--prefix=net.bridge.bridge-nf-call-ip6tables",
+        ])
+        .output()?;
+
+    if !output.status.success() {
+        bail!(
+            "systemd-sysctl failed with {} - {}",
+            output.status,
+            String::from_utf8_lossy(&output.stderr).trim(),
+        );
+    }
+
+    Ok(())
+}
+
 fn handle_firewall() -> Result<(), Error> {
     let firewall = create_firewall_instance()?;
 
@@ -94,8 +118,12 @@ fn run_firewall() -> Result<(), Error> {
     // we're disabled here without the need to parse the config, avoiding log-spam errors from that
     let force_disable_flag = std::path::Path::new(FORCE_DISABLE_FLAG_FILE);
 
+    let mut bridge_sysctls_reapplied = false;
+
     while !term.load(Ordering::Relaxed) {
         if force_disable_flag.exists() {
+            bridge_sysctls_reapplied = false;
+
             if let Err(error) = remove_firewall() {
                 log::error!("unable to disable firewall: {error:?}");
             }
@@ -105,6 +133,13 @@ fn run_firewall() -> Result<(), Error> {
         }
         let start = Instant::now();
 
+        if !bridge_sysctls_reapplied {
+            match reapply_bridge_sysctls() {
+                Ok(()) => bridge_sysctls_reapplied = true,
+                Err(error) => log::error!("unable to re-apply bridge sysctls: {error:#}"),
+            }
+        }
+
         if let Err(error) = handle_firewall() {
             log::error!("error updating firewall rules: {error:#}");
         }
-- 
2.47.3