[RFC firewall/manager/proxmox{,-firewall} 00/13] fix #5759: keep firewall rules up across boot and shutdown

[RFC firewall/manager/proxmox{,-firewall} 00/13] fix #5759: keep firewall rules up across boot and shutdown

[Arthur Bied-Charreton]

Both the nftables and iptables firewall services depend on pve-cluster,
which depends on corosync, which itself depends on network-online. This
means that, as reported in bug #5759, there currently are windows during
both system boot and shutdown where PVE hosts have no firewall
protection.

Approach:

Both windows need to be closed without reordering the firewall services
against pve-cluster (not feasible given the pmxcfs dependency).

1. Boot:

After every successful apply, each daemon dumps the config files it
compiled from (cluster.fw, host.fw, sdn.json) to a local directory on
the root filesystem (/var/lib/pve/firewall), available before pmxcfs.
The guest configurations are not dumped, since pve-guests starts after
the firewall daemon anyway. A new oneshot service ordered before
network-pre.target runs the newly added restore command, which
recompiles and applies rules from those local dumps.

2. Shutdown

On SIGTERM the daemons now query `systemctl is-system-running`. If the
system is shutting down they leave the ruleset in place (the service
otherwise comes down before the network does, which reopens the window),
and only clear the rules on an explicit `systemctl stop`.

Customizability:

{cluster,host}.fw.override configurations can be written to
/var/lib/pve/firewall to allow custom rulesets for the boot-time window.

Open questions:

1. Boot-time firewall config overrides:

I'm honestly not sure if the .override hack is the right design.
host.fw.override may drift away from the config after network interface
pinning changes. This can/will be fixed if we decide to go with this
approach, but I am not sure how I feel about overwriting a config file
manually edited by users.

2. Is this a breaking change?

The fact that the firewall rulesets are not up during boot/shutdown is
not documented, however it has been a property of PVE for a while and
changing this might break some systems. Is this a change that needs
to wait or at least be made opt-in until PVE 10?

3. SDN config

pve-firewall and proxmox-firewall dump sdn.json in different formats:
pve-firewall writes the parsed SDN IPSets, proxmox-firewall writes the
raw running-config it reads from pmxcfs. Only the active backend ever
writes the dumps, so the formats only clash right after switching
backends, before the now-active daemon has rewritten sdn.json in its
own format. A reboot in this window makes the restore read the previous
backend's format.

Neither side errors on the foreign format; both just read it as an empty
SDN config, so the restore drops only the rules that reference SDN
IPSets and applies the rest. For a best-effort boot-time restore, and
given the plan to sunset pve-firewall, I think that is tolerable, so I
left the proper fix out of this series: unifying the format would mean
teaching pve-firewall to parse the raw running-config through a new
proxmox-ve-rs binding. Happy to revisit if deemed necessary.

Future work (deliberately out of scope for this series):

ExecReload:

A remaining "edge case" is restarting the service, since systemctl
restart just enqueues a stop and a start job, it is a little trickier to
figure out that the service is being restarted as opposed to just
stopped. One could technically query systemd for a queued start job on
SIGTERM, but as far as I can tell this would be racy, and I believe a
firewall not being up while the service is being restarted is a
defensible position. The better direction, in my opinion, is a proper
ExecReload implementation. Stefan H. and I discussed adding a proper
runtime to proxmox-firewall and reworking the signal handling, this
would fit well together with such a change.

Proper wait conditions:

We currently have no way to query the firewall status. This leads to
windows where a guest may run without its firewall rules being applied.
For example, when migrating a guest, we currently have no way to know
whether its firewall rules have been applied on the target host already.
The same applies to starting a guest, which may be running for up to 5
seconds (i.e. until the next apply, assuming nftables firewall) without
protection.

Firewall status reporting:

As discussed off-list with Stefan H., the firewall currently has no
status reporting. Rules that fail to parse are logged to the journal and
skipped, those failures are not visible anywhere in the UI. We should be
able to get this for free with the wait conditions.

Build dependencies:
- proxmox-firewall requires bumped proxmox-systemd

Runtime dependencies:
- proxmox-firewall requires bumped pve-manager
- pve-firewall requires bumped pve-manager
- pve-firewall requires bumped libpve-common-perl (for PVE::File)

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5759


pve-manager:

Arthur Bied-Charreton (1):
  network interface pinning: write new firewall config to local dir

 PVE/CLI/pve_network_interface_pinning.pm | 6 +++++-
 bin/pve-firewall-commit                  | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)


pve-firewall:

Arthur Bied-Charreton (6):
  firewall: config: sort OPTIONS when serializing
  d/control: bump libpve-common-perl
  firewall: dump configs locally after applying
  fix #5759: firewall: do not remove chains when host is shutting down
  firewall: add restore command
  fix #5759: firewall: restore from dumped config before network-pre

 debian/control                          |   2 +-
 debian/dirs                             |   1 +
 debian/pve-firewall-pre-network.service |  16 ++++
 debian/rules                            |   1 +
 src/PVE/Firewall.pm                     | 112 +++++++++++++++++++++---
 src/PVE/Service/pve_firewall.pm         |  55 +++++++++++-
 src/pve-firewall                        |  10 ++-
 7 files changed, 176 insertions(+), 21 deletions(-)
 create mode 100644 debian/pve-firewall-pre-network.service


proxmox:

Arthur Bied-Charreton (1):
  systemd: systemctl: add is-system-running helper

 proxmox-systemd/Cargo.toml       |  1 +
 proxmox-systemd/debian/control   |  2 +
 proxmox-systemd/src/lib.rs       |  2 +
 proxmox-systemd/src/systemctl.rs | 98 ++++++++++++++++++++++++++++++++
 4 files changed, 103 insertions(+)
 create mode 100644 proxmox-systemd/src/systemctl.rs


proxmox-firewall:

Arthur Bied-Charreton (5):
  firewall: fix clippy warnings
  fix #5759: firewall: do not clear rules on system shutdown
  firewall: dump config to local directory after apply
  firewall: add restore command
  fix #5759: firewall: restore from dumped config before network-pre

 Cargo.toml                                   |   1 +
 debian/dirs                                  |   1 +
 debian/proxmox-firewall-pre-network.service  |  16 ++
 debian/rules                                 |   2 +-
 proxmox-firewall/Cargo.toml                  |   3 +-
 proxmox-firewall/src/bin/proxmox-firewall.rs | 109 +++++++++++-
 proxmox-firewall/src/config.rs               | 167 ++++++++++++++++---
 proxmox-firewall/src/firewall.rs             |   8 +-
 proxmox-firewall/src/rule.rs                 |  38 ++---
 9 files changed, 295 insertions(+), 50 deletions(-)
 create mode 100644 debian/dirs
 create mode 100644 debian/proxmox-firewall-pre-network.service


Summary over all repositories:
  22 files changed, 580 insertions(+), 72 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH pve-manager 01/13] network interface pinning: write new firewall config to local dir

[Arthur Bied-Charreton]

Preparatory step for restoring firewall rules before pve-cluster is up.

The boot-time restore recompiles the ruleset from the firewall config
dumped to local disk. Interface name pinnings only take effect on the
next boot, so a host.fw dumped before a pinning change still carries the
old interface names, and recompiling from it would produce rules
matching interfaces that no longer exist.

pve-network-interface-pinning already writes the updated config to
host.fw.new. Also write it to the local dump directory so the boot-time
restore prefers it over the stale host.fw; the local copy is removed
again in pve-firewall-commit once the pinning has been committed.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 PVE/CLI/pve_network_interface_pinning.pm | 6 +++++-
 bin/pve-firewall-commit                  | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/PVE/CLI/pve_network_interface_pinning.pm b/PVE/CLI/pve_network_interface_pinning.pm
index 9dff181d..758b2107 100644
--- a/PVE/CLI/pve_network_interface_pinning.pm
+++ b/PVE/CLI/pve_network_interface_pinning.pm
@@ -22,6 +22,8 @@ use base qw(PVE::CLIHandler);
 
 my $PVEETH_LOCK = "/run/lock/proxmox-network-interface-pinning.lck";
 
+my $local_dump_dir = "/var/lib/pve/firewall";
+
 sub setup_environment {
     PVE::RPCEnvironment->setup_default_cli_env();
 }
@@ -120,7 +122,7 @@ my sub update_host_fw_config {
     my ($mapping) = @_;
 
     my $local_node = PVE::INotify::nodename();
-    print "Updating /etc/pve/nodes/$local_node/host.fw.new\n";
+    print "Updating /etc/pve/nodes/$local_node/host.fw.new and $local_dump_dir/host.fw.new\n";
 
     my $code = sub {
         my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
@@ -143,6 +145,8 @@ my sub update_host_fw_config {
         }
 
         PVE::Firewall::save_hostfw_conf($host_conf, "/etc/pve/nodes/$local_node/host.fw.new");
+        make_path($local_dump_dir, { mode => 0700 });
+        PVE::Firewall::save_hostfw_conf($host_conf, "$local_dump_dir/host.fw.new");
     };
 
     PVE::Firewall::run_locked($code);
diff --git a/bin/pve-firewall-commit b/bin/pve-firewall-commit
index 3d208f67..cbb71f58 100644
--- a/bin/pve-firewall-commit
+++ b/bin/pve-firewall-commit
@@ -23,5 +23,6 @@ if (-e $new_fw_config_file) {
     rename($new_fw_config_file, $current_fw_config_file)
         or die "failed to commit new local node firewall config '$new_fw_config_file' - $!\n";
 }
+unlink "/var/lib/pve/firewall/host.fw.new";
 
 exit 0;
-- 
2.47.3

[PATCH pve-firewall 02/13] firewall: config: sort OPTIONS when serializing

[Arthur Bied-Charreton]

Until now, firewall config serialization was iterating over the options
hash without sorting the keys, making the output non-deterministic.

Iterate over the sorted keys instead in format_options.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/Firewall.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 894a87b..6270771 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -3754,7 +3754,7 @@ my $format_options = sub {
     my $raw = '';
 
     $raw .= "[OPTIONS]\n\n";
-    foreach my $opt (keys %$options) {
+    foreach my $opt (sort keys %$options) {
         $raw .= "$opt: $options->{$opt}\n";
     }
     $raw .= "\n";
-- 
2.47.3

[PATCH pve-firewall 03/13] d/control: bump libpve-common-perl

[Arthur Bied-Charreton]

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 debian/control | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debian/control b/debian/control
index a420016..85b6a3b 100644
--- a/debian/control
+++ b/debian/control
@@ -23,7 +23,7 @@ Depends: conntrack,
          iptables,
          libpve-access-control,
          libpve-cluster-perl,
-         libpve-common-perl (>= 9.0.2),
+         libpve-common-perl (>= 9.1.12),
          libpve-network-perl (>= 0.9.9~),
          libpve-rs-perl (>= 0.8.13),
          pve-cluster (>= 6.1-6),
-- 
2.47.3

[PATCH pve-firewall 04/13] firewall: dump configs locally after applying

[Arthur Bied-Charreton]

... and remove them when pve-firewall is disabled in the config.

The firewall config lives on pmxcfs, which is only available once
pve-cluster is up, i.e. well after the network. To be able to restore
rules before that, dump the config files needed to recompile the ruleset
(cluster.fw, host.fw and the SDN config as sdn.json) to a local
directory after every successful apply. The guest configs are not
dumped, since pve-guests does not start until the actual firewall daemon
is running.

The last dumped content is cached per file, so the daemon only writes
when something changed since the last dump.

Ownership of the dumps is keyed on the nftables option in the host
config. When the firewall is disabled, the dumps are only removed if
nftables is disabled - otherwise that responsibility is on
proxmox-firewall.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 debian/dirs                     |  1 +
 src/PVE/Firewall.pm             | 79 +++++++++++++++++++++++++++++----
 src/PVE/Service/pve_firewall.pm |  5 ++-
 3 files changed, 76 insertions(+), 9 deletions(-)

diff --git a/debian/dirs b/debian/dirs
index c9e3b54..e472f56 100644
--- a/debian/dirs
+++ b/debian/dirs
@@ -1 +1,2 @@
 /var/lib/pve-firewall
+/var/lib/pve/firewall
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 6270771..ada8c1d 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -8,6 +8,7 @@ use Encode;
 use File::Basename;
 use File::Path;
 use IO::File;
+use JSON;
 use Net::IP;
 use POSIX;
 use Socket qw(AF_INET AF_INET6 inet_ntop inet_pton);
@@ -16,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;
@@ -31,6 +33,8 @@ my $pvefw_conf_dir = "/etc/pve/firewall";
 my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw";
 my $vnetfw_conf_dir = "/etc/pve/sdn/firewall";
 
+my $dump_dir = '/var/lib/pve/firewall';
+
 # dynamically include PVE::QemuServer and PVE::LXC
 # to avoid dependency problems
 my $have_qemu_server;
@@ -4084,7 +4088,7 @@ sub load_sdn_conf {
     return $sdn_config // $empty_sdn_config;
 }
 
-sub save_clusterfw_conf {
+sub serialize_clusterfw_conf {
     my ($cluster_conf) = @_;
 
     my $raw = '';
@@ -4119,11 +4123,21 @@ sub save_clusterfw_conf {
         }
     }
 
+    return $raw;
+}
+
+sub save_clusterfw_conf {
+    my ($cluster_conf, $filename) = @_;
+
+    $filename = $clusterfw_conf_filename if !defined($filename);
+
+    my $raw = serialize_clusterfw_conf($cluster_conf);
+
     if ($raw) {
         mkdir $pvefw_conf_dir;
-        PVE::Tools::file_set_contents($clusterfw_conf_filename, $raw);
+        PVE::Tools::file_set_contents($filename, $raw);
     } else {
-        unlink $clusterfw_conf_filename;
+        unlink($filename);
     }
 }
 
@@ -4147,11 +4161,8 @@ sub load_hostfw_conf {
     return generic_fw_config_parser($filename, $cluster_conf, $empty_conf, 'host');
 }
 
-sub save_hostfw_conf {
-    my ($hostfw_conf, $filename) = @_;
-
-    $filename = $hostfw_conf_filename if !defined($filename);
-
+sub serialize_hostfw_conf {
+    my ($hostfw_conf) = @_;
     my $raw = '';
 
     my $options = $hostfw_conf->{options};
@@ -4164,6 +4175,16 @@ sub save_hostfw_conf {
         $raw .= "\n";
     }
 
+    return $raw;
+}
+
+sub save_hostfw_conf {
+    my ($hostfw_conf, $filename) = @_;
+
+    $filename = $hostfw_conf_filename if !defined($filename);
+
+    my $raw = serialize_hostfw_conf($hostfw_conf);
+
     if ($raw) {
         PVE::Tools::file_set_contents($filename, $raw);
     } else {
@@ -5234,6 +5255,14 @@ sub remove_pvefw_chains {
 
 }
 
+sub delete_dumps {
+
+    unlink "$dump_dir/cluster.fw";
+    unlink "$dump_dir/host.fw";
+    unlink "$dump_dir/sdn.json";
+
+}
+
 sub remove_pvefw_chains_iptables {
     my ($iptablescmd, $table) = @_;
 
@@ -5332,21 +5361,55 @@ sub init {
     # load required modules here
 }
 
+my $dump_cache = {};
+
+sub dump_if_changed {
+    my ($f, $data) = @_;
+    if (-f $f && defined($dump_cache->{$f}) && $dump_cache->{$f} eq $data) {
+        syslog('info', "no changes to $f since last dump\n");
+        return;
+    }
+    PVE::File::file_set_contents($f, $data);
+    $dump_cache->{$f} = $data;
+    syslog('info', "dumped $f\n");
+}
+
 sub update {
     my $code = sub {
+        my $cfw_dump_path = "$dump_dir/cluster.fw";
+        my $hfw_dump_path = "$dump_dir/host.fw";
+        my $sdn_dump_path = "$dump_dir/sdn.json";
 
         my $cluster_conf = load_clusterfw_conf();
         my $hostfw_conf = load_hostfw_conf($cluster_conf);
 
         if (!is_enabled_and_not_nftables($cluster_conf, $hostfw_conf)) {
             PVE::Firewall::remove_pvefw_chains();
+            $dump_cache = {};
+            # Disabled in config: drop stale dumps so the pre-network restore does not resurrect a
+            # disabled ruleset. Leave them in nftables mode, since in that case proxmox-firewall
+            # owns the dump directory.
+            delete_dumps() if !$cluster_conf->{options}->{enable};
             return;
         }
 
+        # compile() rewrites rule actions in-place. Snapshot the untouched configs now for the
+        # post-apply boot-time dump.
+        my ($cfw_dump, $hfw_dump) = (dclone($cluster_conf), dclone($hostfw_conf));
+
         my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) =
             compile($cluster_conf, $hostfw_conf);
 
         apply_ruleset($ruleset, $hostfw_conf, $ipset_ruleset, $rulesetv6, $ebtables_ruleset);
+
+        eval {
+            mkdir($dump_dir);
+            chmod(0700, $dump_dir);
+            dump_if_changed($cfw_dump_path, serialize_clusterfw_conf($cfw_dump));
+            dump_if_changed($hfw_dump_path, serialize_hostfw_conf($hfw_dump));
+            dump_if_changed($sdn_dump_path, JSON->new->utf8->canonical->encode($cfw_dump->{sdn}));
+        };
+        syslog('err', "could not persist firewall configs at $dump_dir: $@\n") if $@;
     };
 
     run_locked($code);
diff --git a/src/PVE/Service/pve_firewall.pm b/src/PVE/Service/pve_firewall.pm
index 95901a5..9c64442 100755
--- a/src/PVE/Service/pve_firewall.pm
+++ b/src/PVE/Service/pve_firewall.pm
@@ -51,7 +51,10 @@ sub shutdown {
 
     syslog('info', "clear PVE-generated firewall rules");
 
-    eval { PVE::Firewall::remove_pvefw_chains(); };
+    eval {
+        PVE::Firewall::remove_pvefw_chains();
+        PVE::Firewall::delete_dumps();
+    };
     warn $@ if $@;
 
     $self->exit_daemon(0);
-- 
2.47.3

[PATCH pve-firewall 05/13] fix #5759: firewall: do not remove chains when host is shutting down

[Arthur Bied-Charreton]

The assumption until now was that the firewall rules should always be
cleared when the daemon is shutting down. This is correct when the
shutdown happens as the result of `systemctl stop`, should however not
be handled the same way in case of a system shutdown.

Because of the way pve-firewall is ordered, it comes down before the
network does. Clearing the rules in the host shutdown case creates a
window where the network is up without firewall protection.

When shutting down, check the operational state of the system and only
remove the chains if the system is _not_ currently shutting down.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5759
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/Service/pve_firewall.pm | 40 +++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/PVE/Service/pve_firewall.pm b/src/PVE/Service/pve_firewall.pm
index 9c64442..2fe68cc 100755
--- a/src/PVE/Service/pve_firewall.pm
+++ b/src/PVE/Service/pve_firewall.pm
@@ -41,6 +41,29 @@ my $updatetime = 10;
 
 my $initial_memory_usage;
 
+=head3 is_host_shutdown()
+
+Query C<systemctl is-system-running>, returning C<1> if the system's running state is C<stopping>,
+C<0> otherwise.
+
+Note that C<1> is also returned if the running state of the system could not be determined, in order
+to fail closed.
+
+=cut
+
+sub is_host_shutdown {
+    my $state = undef;
+
+    PVE::Tools::run_command(
+        ['systemctl', 'is-system-running'],
+        outfunc => sub { $state = PVE::Tools::trim(shift) },
+        noerr => 1,
+    );
+
+    return 1 if !defined($state) || $state eq '';
+    return $state eq 'stopping';
+}
+
 sub shutdown {
     my ($self) = @_;
 
@@ -49,13 +72,16 @@ sub shutdown {
     # wait for children
     1 while (waitpid(-1, POSIX::WNOHANG()) > 0);
 
-    syslog('info', "clear PVE-generated firewall rules");
-
-    eval {
-        PVE::Firewall::remove_pvefw_chains();
-        PVE::Firewall::delete_dumps();
-    };
-    warn $@ if $@;
+    if (is_host_shutdown()) {
+        syslog('info', "system is stopping, not removing firewall rules\n");
+    } else {
+        syslog('info', "clear PVE-generated firewall rules");
+        eval {
+            PVE::Firewall::remove_pvefw_chains();
+            PVE::Firewall::delete_dumps();
+        };
+        warn $@ if $@;
+    }
 
     $self->exit_daemon(0);
 }
-- 
2.47.3

[PATCH pve-firewall 06/13] firewall: add restore command

[Arthur Bied-Charreton]

Add a 'restore' command that recompiles and applies the firewall from
the config dumped to /var/lib/pve/firewall, to bridge the boot window
before pmxcfs (and thus the real config) is available.

It reuses update() with a local flag: configs are loaded from the local
dumps instead of pmxcfs and nothing is dumped back. A missing or
unreadable SDN dump is treated as empty, and rules referencing an
unavailable IPSet are skipped, so a partial dump still restores the rest
on a best-effort basis.

For the cluster and host config, a user-provided .override or a .new
written by the pve-network-interface-pinning take precedence over the
plain dump when present (.override -> .new -> plain).

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/Firewall.pm             | 33 ++++++++++++++++++++++++++++-----
 src/PVE/Service/pve_firewall.pm | 18 ++++++++++++++++++
 src/pve-firewall                | 10 +++++++---
 3 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index ada8c1d..76dbec1 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -4050,11 +4050,11 @@ sub lock_clusterfw_conf {
 }
 
 sub load_clusterfw_conf {
-    my ($filename) = @_;
+    my ($filename, $sdn_conf) = @_;
 
     $filename = $clusterfw_conf_filename if !defined($filename);
 
-    my $sdn_conf = load_sdn_conf();
+    $sdn_conf //= load_sdn_conf();
 
     my $empty_conf = {
         rules => [],
@@ -5374,14 +5374,32 @@ sub dump_if_changed {
     syslog('info', "dumped $f\n");
 }
 
+my sub get_local_config_dump_path {
+    my ($filename) = @_;
+    for my $p (qw(.override .new)) {
+        my $f = $filename . $p;
+        return $f if -f $f;
+    }
+    return $filename;
+}
+
 sub update {
+    my ($local) = @_;
+
     my $code = sub {
         my $cfw_dump_path = "$dump_dir/cluster.fw";
         my $hfw_dump_path = "$dump_dir/host.fw";
         my $sdn_dump_path = "$dump_dir/sdn.json";
 
-        my $cluster_conf = load_clusterfw_conf();
-        my $hostfw_conf = load_hostfw_conf($cluster_conf);
+        my $cfw_conf_path = $local ? get_local_config_dump_path($cfw_dump_path) : undef;
+        my $hfw_conf_path = $local ? get_local_config_dump_path($hfw_dump_path) : undef;
+        my $sdn_conf = undef;
+        $sdn_conf = eval { decode_json(PVE::File::file_get_contents($sdn_dump_path)) }
+            if $local;
+        $sdn_conf = undef if $local && (!ref($sdn_conf) || ref($sdn_conf->{ipset}) ne 'HASH');
+
+        my $cluster_conf = load_clusterfw_conf($cfw_conf_path, $sdn_conf);
+        my $hostfw_conf = load_hostfw_conf($cluster_conf, $hfw_conf_path);
 
         if (!is_enabled_and_not_nftables($cluster_conf, $hostfw_conf)) {
             PVE::Firewall::remove_pvefw_chains();
@@ -5389,10 +5407,13 @@ sub update {
             # Disabled in config: drop stale dumps so the pre-network restore does not resurrect a
             # disabled ruleset. Leave them in nftables mode, since in that case proxmox-firewall
             # owns the dump directory.
-            delete_dumps() if !$cluster_conf->{options}->{enable};
+            delete_dumps() if !$local && !$cluster_conf->{options}->{enable};
+            syslog('info', "iptables firewall is not enabled, not restoring rules\n") if $local;
             return;
         }
 
+        syslog('info', "restoring last remembered ruleset from $dump_dir\n") if $local;
+
         # compile() rewrites rule actions in-place. Snapshot the untouched configs now for the
         # post-apply boot-time dump.
         my ($cfw_dump, $hfw_dump) = (dclone($cluster_conf), dclone($hostfw_conf));
@@ -5402,6 +5423,8 @@ sub update {
 
         apply_ruleset($ruleset, $hostfw_conf, $ipset_ruleset, $rulesetv6, $ebtables_ruleset);
 
+        return if $local;
+
         eval {
             mkdir($dump_dir);
             chmod(0700, $dump_dir);
diff --git a/src/PVE/Service/pve_firewall.pm b/src/PVE/Service/pve_firewall.pm
index 2fe68cc..8cba523 100755
--- a/src/PVE/Service/pve_firewall.pm
+++ b/src/PVE/Service/pve_firewall.pm
@@ -10,6 +10,7 @@ use PVE::CLIHandler;
 use PVE::Cluster qw(cfs_read_file);
 use PVE::Corosync;
 use PVE::Daemon;
+use PVE::File;
 use PVE::INotify;
 use PVE::ProcFSTools;
 use PVE::RPCEnvironment;
@@ -487,6 +488,22 @@ __PACKAGE__->register_method({
     },
 });
 
+__PACKAGE__->register_method({
+    name => 'restore',
+    path => 'restore',
+    method => 'GET',
+    description => 'Attempt to restore the last remembered ruleset',
+    parameters => {
+        additionalProperties => 0,
+        properties => {},
+    },
+    returns => { type => 'null' },
+    code => sub {
+        PVE::Firewall::update(1);
+        return undef;
+    },
+});
+
 our $cmddef = {
     start => [__PACKAGE__, 'start', []],
     restart => [__PACKAGE__, 'restart', []],
@@ -494,6 +511,7 @@ our $cmddef = {
     compile => [__PACKAGE__, 'compile', []],
     simulate => [__PACKAGE__, 'simulate', []],
     localnet => [__PACKAGE__, 'localnet', []],
+    restore => [__PACKAGE__, 'restore', []],
     status => [
         __PACKAGE__,
         'status',
diff --git a/src/pve-firewall b/src/pve-firewall
index 5b62430..6343adc 100755
--- a/src/pve-firewall
+++ b/src/pve-firewall
@@ -14,12 +14,16 @@ $SIG{'__WARN__'} = sub {
     $@ = $err;
 };
 
+my $is_restore = ($ARGV[0] // '') eq 'restore';
+
 my $prepare = sub {
     my $rpcenv = PVE::RPCEnvironment->init('cli');
 
-    $rpcenv->init_request();
-    $rpcenv->set_language($ENV{LANG});
-    $rpcenv->set_user('root@pam');
+
+    # 'restore' runs at pre-network, before pmxcfs is up, and needs no ACL
+    $rpcenv->init_request() if !$is_restore;
+    $rpcenv->set_language($ENV{LANG}) if !$is_restore;
+    $rpcenv->set_user('root@pam') if !$is_restore;
 };
 
 PVE::Service::pve_firewall->run_cli_handler(prepare => $prepare);
-- 
2.47.3

[PATCH pve-firewall 07/13] fix #5759: firewall: restore from dumped config before network-pre

[Arthur Bied-Charreton]

Since pve-firewall depends on pve-cluster for its config, pve-cluster
depends on corosync, which itself depends on network-online. This
creates a window at boot where the network is up with no protection.

To address this, add a simple oneshot service depending on
network-pre [0] that applies the last remembered rules to bridge the gap
until the configured firewall daemon takes over.

[0] https://systemd.io/NETWORK_ONLINE/

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5759
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 debian/pve-firewall-pre-network.service | 16 ++++++++++++++++
 debian/rules                            |  1 +
 2 files changed, 17 insertions(+)
 create mode 100644 debian/pve-firewall-pre-network.service

diff --git a/debian/pve-firewall-pre-network.service b/debian/pve-firewall-pre-network.service
new file mode 100644
index 0000000..e1127e4
--- /dev/null
+++ b/debian/pve-firewall-pre-network.service
@@ -0,0 +1,16 @@
+[Unit]
+Description=Proxmox VE Pre-Network iptables Firewall
+Wants=network-pre.target
+Before=network-pre.target shutdown.target
+Conflicts=shutdown.target
+DefaultDependencies=no
+After=local-fs.target
+
+[Service]
+Type=oneshot
+ExecStart=/usr/sbin/pve-firewall restore
+RemainAfterExit=true
+Environment=PVE_LOG=info
+
+[Install]
+WantedBy=sysinit.target
diff --git a/debian/rules b/debian/rules
index 9bd53bc..170a90d 100755
--- a/debian/rules
+++ b/debian/rules
@@ -8,6 +8,7 @@
 
 override_dh_installsystemd:
 	dh_installsystemd --name pvefw-logger pvefw-logger.service
+	dh_installsystemd --no-start --name pve-firewall-pre-network pve-firewall-pre-network.service
 	dh_installsystemd --name pve-firewall --no-stop-on-upgrade --no-restart-after-upgrade pve-firewall.service
 
 override_dh_installinit:
-- 
2.47.3

[PATCH proxmox 08/13] systemd: systemctl: add is-system-running helper

[Arthur Bied-Charreton]

Introduce new `systemctl` module with `is_system_running` helper.

In some cases, it can be important to recognize whether a service is
being stopped as the result of a system shutdown, as opposed to an
explicit `systemctl stop`.

For example, firewalls must differentiate between a manual stop and a
shutdown. A manual stop should clear the ruleset, while a shutdown
should ideally not touch it.

`is_system_running` runs `systemctl is-system-running` and parses its
output to allow differentiating between those cases. While the `is_`
prefix implies a boolean return value by convention, the name mirrors
the systemctl subcommand, which seems less confusing than inventing a
new name for a wrapper.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 proxmox-systemd/Cargo.toml       |  1 +
 proxmox-systemd/debian/control   |  2 +
 proxmox-systemd/src/lib.rs       |  2 +
 proxmox-systemd/src/systemctl.rs | 98 ++++++++++++++++++++++++++++++++
 4 files changed, 103 insertions(+)
 create mode 100644 proxmox-systemd/src/systemctl.rs

diff --git a/proxmox-systemd/Cargo.toml b/proxmox-systemd/Cargo.toml
index 74ec03d3..387aa3c7 100644
--- a/proxmox-systemd/Cargo.toml
+++ b/proxmox-systemd/Cargo.toml
@@ -14,3 +14,4 @@ repository.workspace = true
 
 [dependencies]
 libc.workspace = true
+thiserror.workspace = true
diff --git a/proxmox-systemd/debian/control b/proxmox-systemd/debian/control
index 766c9c78..d9e5d3a3 100644
--- a/proxmox-systemd/debian/control
+++ b/proxmox-systemd/debian/control
@@ -7,6 +7,7 @@ Build-Depends-Arch: cargo:native <!nocheck>,
  rustc:native <!nocheck>,
  libstd-rust-dev <!nocheck>,
  librust-libc-0.2+default-dev (>= 0.2.107-~~) <!nocheck>,
+ librust-thiserror-2+default-dev <!nocheck>,
  libsystemd-dev <!nocheck>
 Maintainer: Proxmox Support Team <support@proxmox.com>
 Standards-Version: 4.7.2
@@ -21,6 +22,7 @@ Multi-Arch: same
 Depends:
  ${misc:Depends},
  librust-libc-0.2+default-dev (>= 0.2.107-~~),
+ librust-thiserror-2+default-dev,
  libsystemd-dev
 Provides:
  librust-proxmox-systemd+default-dev (= ${binary:Version}),
diff --git a/proxmox-systemd/src/lib.rs b/proxmox-systemd/src/lib.rs
index eff61d58..a0d8de2c 100644
--- a/proxmox-systemd/src/lib.rs
+++ b/proxmox-systemd/src/lib.rs
@@ -9,3 +9,5 @@ pub mod journal;
 pub mod notify;
 
 pub mod sd_id128;
+
+pub mod systemctl;
diff --git a/proxmox-systemd/src/systemctl.rs b/proxmox-systemd/src/systemctl.rs
new file mode 100644
index 00000000..e03820bc
--- /dev/null
+++ b/proxmox-systemd/src/systemctl.rs
@@ -0,0 +1,98 @@
+use std::{str::FromStr, string::FromUtf8Error};
+
+#[derive(thiserror::Error, Debug)]
+pub enum SystemctlError {
+    #[error("could not run systemctl: {0}")]
+    Io(#[from] std::io::Error),
+    #[error("unexpected output: {0}")]
+    UnexpectedOutput(String),
+}
+
+impl From<FromUtf8Error> for SystemctlError {
+    fn from(value: FromUtf8Error) -> Self {
+        Self::UnexpectedOutput(format!("output is not valid UTF-8: {value}"))
+    }
+}
+
+/// Possible operational states of the system as returned by `systemctl is-system-running` [0].
+///
+/// [0]: https://www.freedesktop.org/software/systemd/man/latest/systemctl.html#is-system-running
+#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
+pub enum SystemState {
+    /// Early bootup, before `basic.target` is reached or the [`SystemState::Maintenance`]
+    /// is entered.
+    Initializing,
+    /// Late bootup, before the job queue becomes idle for the first time, or one of the
+    /// rescue targets are reached.
+    Starting,
+    /// The system is fully operational.
+    Running,
+    /// The system is operational but one or more units failed.
+    Degraded,
+    /// The rescue or emergency target is active.
+    Maintenance,
+    /// The manager is shutting down.
+    Stopping,
+    /// The manager is not running. Specifically, this is the operational state if an
+    /// incompatible program is running as system manager (PID 1).
+    Offline,
+    /// The operational state could not be determined, due to lack of resources or another
+    /// error case.
+    Unknown,
+}
+
+impl std::fmt::Display for SystemState {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::Initializing => write!(f, "initializing"),
+            Self::Starting => write!(f, "starting"),
+            Self::Running => write!(f, "running"),
+            Self::Degraded => write!(f, "degraded"),
+            Self::Maintenance => write!(f, "maintenance"),
+            Self::Stopping => write!(f, "stopping"),
+            Self::Offline => write!(f, "offline"),
+            Self::Unknown => write!(f, "unknown"),
+        }
+    }
+}
+
+impl FromStr for SystemState {
+    type Err = SystemctlError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {
+            "initializing" => Ok(Self::Initializing),
+            "starting" => Ok(Self::Starting),
+            "running" => Ok(Self::Running),
+            "degraded" => Ok(Self::Degraded),
+            "maintenance" => Ok(Self::Maintenance),
+            "stopping" => Ok(Self::Stopping),
+            "offline" => Ok(Self::Offline),
+            "unknown" => Ok(Self::Unknown),
+            other => Err(SystemctlError::UnexpectedOutput(other.into())),
+        }
+    }
+}
+
+/// Get the current operational state of the system.
+///
+/// This runs `systemctl is-system-running` [0] and parses the state printed to stdout. While the
+/// command exits non-zero whenever the state is anything other than [`SystemState::Running`], this
+/// function returns any recognized state wrapped in `Ok` instead.
+///
+/// ## Errors
+///
+/// Returns an error if the command could not be spawned, its output was not valid UTF-8, or if the
+/// printed state was not one of the known [`SystemState`] variants.
+///
+/// [0]: https://www.freedesktop.org/software/systemd/man/latest/systemctl.html#is-system-running
+pub fn is_system_running() -> Result<SystemState, SystemctlError> {
+    let output = std::process::Command::new("systemctl")
+        .arg("is-system-running")
+        .output()?;
+
+    // Current system state is always printed to stdout.
+    let stdout = String::from_utf8(output.stdout)?;
+
+    SystemState::from_str(stdout.trim())
+}
-- 
2.47.3

[PATCH proxmox-firewall 09/13] firewall: fix clippy warnings

[Arthur Bied-Charreton]

No functional changes intended.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 proxmox-firewall/src/firewall.rs |  4 ++--
 proxmox-firewall/src/rule.rs     | 38 ++++++++++++++++----------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs
index f105fa8..477b69b 100644
--- a/proxmox-firewall/src/firewall.rs
+++ b/proxmox-firewall/src/firewall.rs
@@ -251,8 +251,8 @@ impl Firewall {
         if let Some(ipam_config) = self.config.ipam() {
             let ipsets = ipam_config.ipsets();
 
-            self.create_ipsets(&mut commands, &ipsets, &cluster_host_table, None)?;
-            self.create_ipsets(&mut commands, &ipsets, &guest_table, None)?;
+            self.create_ipsets(&mut commands, ipsets, &cluster_host_table, None)?;
+            self.create_ipsets(&mut commands, ipsets, &guest_table, None)?;
         }
 
         if self.config.host().is_enabled() {
diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs
index 048c00e..d33e31a 100644
--- a/proxmox-firewall/src/rule.rs
+++ b/proxmox-firewall/src/rule.rs
@@ -250,24 +250,24 @@ impl ToNftRules for RuleMatch {
             return Ok(());
         }
 
-        if let Some(log) = self.log() {
-            if let Ok(log_level) = LogLevel::try_from(log) {
-                let mut terminal_statements = Vec::new();
+        if let Some(log) = self.log()
+            && let Ok(log_level) = LogLevel::try_from(log)
+        {
+            let mut terminal_statements = Vec::new();
 
-                if let Some(limit) = env.default_log_limit() {
-                    terminal_statements.push(Statement::from(limit));
-                }
+            if let Some(limit) = env.default_log_limit() {
+                terminal_statements.push(Statement::from(limit));
+            }
 
-                terminal_statements.push(
-                    Log::new_nflog(
-                        Log::generate_prefix(env.vmid, log_level, env.chain.name(), self.verdict()),
-                        0,
-                    )
-                    .into(),
-                );
+            terminal_statements.push(
+                Log::new_nflog(
+                    Log::generate_prefix(env.vmid, log_level, env.chain.name(), self.verdict()),
+                    0,
+                )
+                .into(),
+            );
 
-                rules.push(NftRule::from_terminal_statements(terminal_statements));
-            }
+            rules.push(NftRule::from_terminal_statements(terminal_statements));
         }
 
         rules.push(NftRule::new(generate_verdict(self.verdict(), env)));
@@ -844,10 +844,10 @@ impl ToNftRules for Ipfilter<'_> {
 
 impl ToNftRules for CtHelperMacro {
     fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
-        if let Some(family) = self.family() {
-            if !env.contains_family(family) {
-                return Ok(());
-            }
+        if let Some(family) = self.family()
+            && !env.contains_family(family)
+        {
+            return Ok(());
         }
 
         if self.tcp().is_none() && self.udp().is_none() {
-- 
2.47.3

[PATCH proxmox-firewall 10/13] fix #5759: firewall: do not clear rules on system shutdown

[Arthur Bied-Charreton]

The assumption until now was that the firewall rules should always be
cleared when receiving SIGTERM. This is correct when SIGTERM is received
as the result of `systemctl stop`, should however not be handled the
same way in case of a system shutdown.

Because of the way the proxmox-firewall service is ordered, it comes
down before the network. Clearing the rules in that case creates a
window where the network is up without firewall protection.

Check the operational state of the system when receiving a stop signal
and only clear the nftables rules if the system is not currently
shutting down.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5759
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 Cargo.toml                                   |  1 +
 proxmox-firewall/Cargo.toml                  |  1 +
 proxmox-firewall/src/bin/proxmox-firewall.rs | 12 +++++++++++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Cargo.toml b/Cargo.toml
index 1dd2784..5f2fa1c 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -33,6 +33,7 @@ proxmox-network-api = "1"
 proxmox-network-types = "1"
 proxmox-serde = "1"
 proxmox-sys = "1"
+proxmox-systemd = "1.0.1"
 proxmox-ve-config = "0.10"
 
 # workspace crates
diff --git a/proxmox-firewall/Cargo.toml b/proxmox-firewall/Cargo.toml
index 6ad7e79..fa7fd34 100644
--- a/proxmox-firewall/Cargo.toml
+++ b/proxmox-firewall/Cargo.toml
@@ -22,6 +22,7 @@ proxmox-log.workspace = true
 proxmox-network-types.workspace = true
 proxmox-network-api = { workspace = true, features = [ "impl" ] }
 proxmox-nftables = { workspace = true, features = [ "config-ext" ] }
+proxmox-systemd.workspace = true
 proxmox-ve-config.workspace = true
 
 [dev-dependencies]
diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
index e8ed477..27fd67d 100644
--- a/proxmox-firewall/src/bin/proxmox-firewall.rs
+++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
@@ -10,6 +10,7 @@ use proxmox_firewall::firewall::Firewall;
 use proxmox_log as log;
 use proxmox_log::{LevelFilter, Logger};
 use proxmox_nftables::{NftClient, client::NftError};
+use proxmox_systemd::systemctl;
 use proxmox_ve_config::firewall::host::Config as HostConfig;
 
 const HELP: &str = r#"
@@ -115,7 +116,16 @@ fn run_firewall() -> Result<(), Error> {
         std::thread::sleep(Duration::from_secs(5));
     }
 
-    remove_firewall().with_context(|| "Could not remove firewall rules")
+    match systemctl::is_system_running() {
+        // Got SIGTERM as the result of a shutdown, do not remove rules.
+        Ok(systemctl::SystemState::Stopping) => {
+            log::info!("system is stopping, not removing firewall rules");
+            Ok(())
+        }
+        Err(e) => bail!("{e}"),
+        // System is not stopping, firewall was shut down explicitly, remove rules.
+        _ => remove_firewall().with_context(|| "could not remove firewall rules"),
+    }
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-- 
2.47.3

[PATCH proxmox-firewall 11/13] firewall: dump config to local directory after apply

[Arthur Bied-Charreton]

The firewall configuration files are stored on pmxcfs, which is only
available when pve-cluster is up. To be able to restore firewall rules
before the network comes up and close the boot-time window in which the
PVE host is unprotected, dump the required configuration files to a
local directory after every apply.

The last dumped version is cached for each file, the daemon only
actually writes to disk if there has been a change since the last write
or the dump file does not exist.

Ownership of the dumps is keyed on the nftables option in the host
config. When the firewall is disabled, the dumps are only removed if
nftables is enabled - otherwise that responsibility is on pve-firewall.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 debian/dirs                                  |  1 +
 proxmox-firewall/Cargo.toml                  |  2 +-
 proxmox-firewall/src/bin/proxmox-firewall.rs | 73 ++++++++++++++++++--
 proxmox-firewall/src/config.rs               | 56 +++++++++++----
 proxmox-firewall/src/firewall.rs             |  4 ++
 5 files changed, 117 insertions(+), 19 deletions(-)
 create mode 100644 debian/dirs

diff --git a/debian/dirs b/debian/dirs
new file mode 100644
index 0000000..34355af
--- /dev/null
+++ b/debian/dirs
@@ -0,0 +1 @@
+/var/lib/pve/firewall
diff --git a/proxmox-firewall/Cargo.toml b/proxmox-firewall/Cargo.toml
index fa7fd34..77ca285 100644
--- a/proxmox-firewall/Cargo.toml
+++ b/proxmox-firewall/Cargo.toml
@@ -22,9 +22,9 @@ proxmox-log.workspace = true
 proxmox-network-types.workspace = true
 proxmox-network-api = { workspace = true, features = [ "impl" ] }
 proxmox-nftables = { workspace = true, features = [ "config-ext" ] }
+proxmox-sys.workspace = true
 proxmox-systemd.workspace = true
 proxmox-ve-config.workspace = true
 
 [dev-dependencies]
 insta = { workspace = true, features = [ "json" ] }
-proxmox-sys.workspace = true
diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
index 27fd67d..5b20ca0 100644
--- a/proxmox-firewall/src/bin/proxmox-firewall.rs
+++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
@@ -1,3 +1,6 @@
+use std::collections::HashMap;
+use std::os::unix::fs::PermissionsExt;
+use std::path::Path;
 use std::sync::Arc;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::time::{Duration, Instant};
@@ -5,11 +8,14 @@ use std::time::{Duration, Instant};
 use anyhow::{Context, Error, bail, format_err};
 use pico_args::Arguments;
 
-use proxmox_firewall::config::{FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader};
+use proxmox_firewall::config::{
+    DUMP_DIR, FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader,
+};
 use proxmox_firewall::firewall::Firewall;
 use proxmox_log as log;
 use proxmox_log::{LevelFilter, Logger};
 use proxmox_nftables::{NftClient, client::NftError};
+use proxmox_sys::fs;
 use proxmox_systemd::systemctl;
 use proxmox_ve_config::firewall::host::Config as HostConfig;
 
@@ -42,14 +48,58 @@ fn remove_firewall() -> Result<(), std::io::Error> {
     Ok(())
 }
 
+fn dump_config(cfg: &FirewallConfig, cache: &mut HashMap<&str, Vec<u8>>) {
+    let dump_dir = Path::new(DUMP_DIR);
+    if !dump_dir.exists()
+        && let Err(e) = fs::create_path(dump_dir, None, None)
+    {
+        log::warn!("could not create {DUMP_DIR}: {e} - config will not be dumped");
+        return;
+    }
+
+    if let Err(e) = std::fs::set_permissions(dump_dir, std::fs::Permissions::from_mode(0o700)) {
+        log::warn!("could not set permissions on {DUMP_DIR}: {e}");
+    }
+
+    for (bytes, name) in [
+        (cfg.cluster_raw(), "cluster.fw"),
+        (cfg.host_raw(), "host.fw"),
+        (cfg.sdn_raw(), "sdn.json"),
+    ] {
+        let out = dump_dir.join(name);
+        match bytes {
+            Some(b) if !out.exists() || b != cache.get(name).map(Vec::as_slice).unwrap_or(&[]) => {
+                match fs::replace_file(&out, b, fs::CreateOptions::new(), true) {
+                    Err(e) => log::warn!("could not dump {name} to {out:?}: {e}"),
+                    Ok(()) => {
+                        log::info!("successfully dumped {out:?}");
+                        cache.insert(name, b.to_vec());
+                    }
+                }
+            }
+            Some(_) => log::info!("no changes to {name} since last dump to {out:?}"),
+            None => {
+                log::info!("nothing to dump for {name}");
+                cache.remove(name);
+                _ = std::fs::remove_file(&out);
+            }
+        }
+    }
+}
+
 fn create_firewall_instance() -> Result<Firewall, Error> {
     let config = FirewallConfig::new(&PveFirewallConfigLoader::new(), &PveNftConfigLoader::new())?;
     Ok(Firewall::new(config))
 }
 
-fn handle_firewall() -> Result<(), Error> {
-    let firewall = create_firewall_instance()?;
+fn delete_dumps(cache: &mut HashMap<&str, Vec<u8>>) {
+    cache.clear();
+    for f in ["cluster.fw", "host.fw", "sdn.json"] {
+        _ = std::fs::remove_file(Path::new(DUMP_DIR).join(f));
+    }
+}
 
+fn handle_firewall(firewall: &Firewall) -> Result<(), Error> {
     if !firewall.is_enabled() {
         return remove_firewall().with_context(|| "could not remove firewall tables".to_string());
     }
@@ -95,6 +145,8 @@ 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 cache = HashMap::new();
+
     while !term.load(Ordering::Relaxed) {
         if force_disable_flag.exists() {
             if let Err(error) = remove_firewall() {
@@ -106,9 +158,18 @@ fn run_firewall() -> Result<(), Error> {
         }
         let start = Instant::now();
 
-        if let Err(error) = handle_firewall() {
-            log::error!("error updating firewall rules: {error:#}");
-        }
+        match create_firewall_instance() {
+            Err(e) => log::error!("could not load firewall configuration: {e}"),
+            Ok(fw) => match handle_firewall(&fw) {
+                Err(e) => log::error!("error updating firewall rules: {e:#}"),
+                Ok(()) if fw.is_enabled() => dump_config(fw.config(), &mut cache),
+                // is_enabled() is false, nftables set here means the cluster firewall is disabled
+                // and the dumps are owned by us, so we clean them up. With nftables unset, they
+                // would be owned by pve-firewall.
+                Ok(()) if fw.config().host().nftables() => delete_dumps(&mut cache),
+                Ok(()) => (),
+            },
+        };
 
         let duration = start.elapsed();
         log::info!("firewall update time: {}ms", duration.as_millis());
diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs
index 341e05a..eb8dd21 100644
--- a/proxmox-firewall/src/config.rs
+++ b/proxmox-firewall/src/config.rs
@@ -1,7 +1,7 @@
 use std::collections::BTreeMap;
 use std::default::Default;
 use std::fs::{self, DirEntry, File, ReadDir};
-use std::io::{self, BufReader};
+use std::io::{self, BufRead, BufReader};
 
 use anyhow::{Context, Error, bail, format_err};
 
@@ -74,6 +74,16 @@ fn open_config_file(path: &str) -> Result<Option<File>, Error> {
     }
 }
 
+fn read_opt(reader: Option<Box<dyn BufRead>>) -> Result<Option<Vec<u8>>, Error> {
+    reader
+        .map(|mut r| {
+            let mut buf = Vec::new();
+            r.read_to_end(&mut buf)?;
+            Ok(buf)
+        })
+        .transpose()
+}
+
 fn open_config_folder(path: &str) -> Result<Option<ReadDir>, Error> {
     match fs::read_dir(path) {
         Ok(paths) => Ok(Some(paths)),
@@ -104,6 +114,8 @@ const SDN_RUNNING_CONFIG_PATH: &str = "/etc/pve/sdn/.running-config";
 const SDN_IPAM_PATH: &str = "/etc/pve/sdn/pve-ipam-state.json";
 const SDN_IPAM_PATH_LEGACY: &str = "/etc/pve/priv/ipam.db"; // TODO: remove with PVE 9+
 
+pub const DUMP_DIR: &str = "/var/lib/pve/firewall";
+
 impl FirewallConfigLoader for PveFirewallConfigLoader {
     fn cluster(&self) -> Result<Option<Box<dyn io::BufRead>>, Error> {
         log::info!("loading cluster config");
@@ -298,11 +310,14 @@ pub struct FirewallConfig {
     sdn_config: Option<FirewallSdnConfig>,
     ipam_config: Option<FirewallIpamConfig>,
     interface_mapping: AltnameMapping,
+    cluster_raw: Option<Vec<u8>>,
+    host_raw: Option<Vec<u8>>,
+    sdn_raw: Option<Vec<u8>>,
 }
 
 impl FirewallConfig {
-    fn parse_cluster(firewall_loader: &dyn FirewallConfigLoader) -> Result<ClusterConfig, Error> {
-        match firewall_loader.cluster()? {
+    fn parse_cluster(raw: Option<&[u8]>) -> Result<ClusterConfig, Error> {
+        match raw {
             Some(data) => ClusterConfig::parse(data),
             None => {
                 log::info!("no cluster config found, falling back to default");
@@ -311,8 +326,8 @@ impl FirewallConfig {
         }
     }
 
-    fn parse_host(firewall_loader: &dyn FirewallConfigLoader) -> Result<HostConfig, Error> {
-        match firewall_loader.host()? {
+    fn parse_host(raw: Option<&[u8]>) -> Result<HostConfig, Error> {
+        match raw {
             Some(data) => HostConfig::parse(data),
             None => {
                 log::info!("no host config found, falling back to default");
@@ -355,10 +370,8 @@ impl FirewallConfig {
         Ok(guests)
     }
 
-    pub fn parse_sdn(
-        firewall_loader: &dyn FirewallConfigLoader,
-    ) -> Result<Option<FirewallSdnConfig>, Error> {
-        Ok(match firewall_loader.sdn_running_config()? {
+    pub fn parse_sdn(raw: Option<&[u8]>) -> Result<Option<FirewallSdnConfig>, Error> {
+        Ok(match raw {
             Some(data) => {
                 let running_config: RunningConfig = serde_json::from_reader(data)?;
                 let config = SdnConfig::try_from(running_config)?;
@@ -437,15 +450,22 @@ impl FirewallConfig {
         firewall_loader: &dyn FirewallConfigLoader,
         nft_loader: &dyn NftConfigLoader,
     ) -> Result<Self, Error> {
+        let cluster_raw = read_opt(firewall_loader.cluster()?)?;
+        let host_raw = read_opt(firewall_loader.host()?)?;
+        let sdn_raw = read_opt(firewall_loader.sdn_running_config()?)?;
+
         Ok(Self {
-            cluster_config: Self::parse_cluster(firewall_loader)?,
-            host_config: Self::parse_host(firewall_loader)?,
+            cluster_config: Self::parse_cluster(cluster_raw.as_deref())?,
+            host_config: Self::parse_host(host_raw.as_deref())?,
             guest_config: Self::parse_guests(firewall_loader)?,
             bridge_config: Self::parse_bridges(firewall_loader)?,
-            sdn_config: Self::parse_sdn(firewall_loader)?,
+            sdn_config: Self::parse_sdn(sdn_raw.as_deref())?,
             ipam_config: Self::parse_ipam(firewall_loader)?,
             nft_config: Self::parse_nft(nft_loader)?,
             interface_mapping: firewall_loader.interface_mapping()?,
+            cluster_raw,
+            host_raw,
+            sdn_raw,
         })
     }
 
@@ -453,10 +473,18 @@ impl FirewallConfig {
         &self.cluster_config
     }
 
+    pub fn cluster_raw(&self) -> Option<&[u8]> {
+        self.cluster_raw.as_deref()
+    }
+
     pub fn host(&self) -> &HostConfig {
         &self.host_config
     }
 
+    pub fn host_raw(&self) -> Option<&[u8]> {
+        self.host_raw.as_deref()
+    }
+
     pub fn guests(&self) -> &BTreeMap<Vmid, GuestConfig> {
         &self.guest_config
     }
@@ -473,6 +501,10 @@ impl FirewallConfig {
         self.sdn_config.as_ref()
     }
 
+    pub fn sdn_raw(&self) -> Option<&[u8]> {
+        self.sdn_raw.as_deref()
+    }
+
     pub fn ipam(&self) -> Option<&FirewallIpamConfig> {
         self.ipam_config.as_ref()
     }
diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs
index 477b69b..7f3e633 100644
--- a/proxmox-firewall/src/firewall.rs
+++ b/proxmox-firewall/src/firewall.rs
@@ -63,6 +63,10 @@ impl Firewall {
         Self { config }
     }
 
+    pub fn config(&self) -> &FirewallConfig {
+        &self.config
+    }
+
     pub fn is_enabled(&self) -> bool {
         self.config.is_enabled()
     }
-- 
2.47.3

[PATCH proxmox-firewall 12/13] firewall: add restore command

[Arthur Bied-Charreton]

Add a 'restore' command that recompiles and applies the firewall from
the config dumped to /var/lib/pve/firewall, to bridge the boot window
before pmxcfs (and thus the real config) is available.

It reuses handle_firewall() along with a local firewall config loader.
The config files are loaded from the local dump instead of pmxcfs and
nothing is dumped back. A missing or unreadable SDN dump is treated as
empty and rules referencing an unavailable IPSet are skipped, so a
partial dump still restores the rest on a best-effort basis.

For the cluster and host config, a user-provided .override or a .new
written by the pve-network-interface-pinning tool take precedence over
the plain dump when present (.override -> .new -> plain).

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 proxmox-firewall/src/bin/proxmox-firewall.rs |  26 ++++-
 proxmox-firewall/src/config.rs               | 111 +++++++++++++++++--
 2 files changed, 128 insertions(+), 9 deletions(-)

diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
index 5b20ca0..e96083c 100644
--- a/proxmox-firewall/src/bin/proxmox-firewall.rs
+++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
@@ -9,7 +9,8 @@ use anyhow::{Context, Error, bail, format_err};
 use pico_args::Arguments;
 
 use proxmox_firewall::config::{
-    DUMP_DIR, FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader,
+    DUMP_DIR, FirewallConfig, LocalFirewallConfigLoader, LocalNftConfigLoader,
+    PveFirewallConfigLoader, PveNftConfigLoader,
 };
 use proxmox_firewall::firewall::Firewall;
 use proxmox_log as log;
@@ -29,6 +30,7 @@ COMMANDS:
   compile           Compile and print firewall rules as accepted by 'nft -j -f -'
   start             Execute proxmox-firewall service in foreground
   localnet          Print the contents of the management ipset
+  restore           Attempt to restore the last remembered ruleset
 "#;
 
 const RULE_BASE: &str = include_str!("../../resources/proxmox-firewall.nft");
@@ -87,6 +89,14 @@ fn dump_config(cfg: &FirewallConfig, cache: &mut HashMap<&str, Vec<u8>>) {
     }
 }
 
+fn create_local_firewall_instance() -> Result<Firewall, Error> {
+    let config = FirewallConfig::new(
+        &LocalFirewallConfigLoader::new(),
+        &LocalNftConfigLoader::new(),
+    )?;
+    Ok(Firewall::new(config))
+}
+
 fn create_firewall_instance() -> Result<Firewall, Error> {
     let config = FirewallConfig::new(&PveFirewallConfigLoader::new(), &PveNftConfigLoader::new())?;
     Ok(Firewall::new(config))
@@ -189,6 +199,17 @@ fn run_firewall() -> Result<(), Error> {
     }
 }
 
+fn restore() -> Result<(), Error> {
+    let fw = create_local_firewall_instance()?;
+    match fw.is_enabled() {
+        true => handle_firewall(&fw),
+        false => {
+            log::info!("nftables firewall is not enabled, not restoring rules");
+            Ok(())
+        }
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum Command {
     Compile,
@@ -196,6 +217,7 @@ pub enum Command {
     Skeleton,
     Start,
     Localnet,
+    Restore,
 }
 
 impl std::str::FromStr for Command {
@@ -208,6 +230,7 @@ impl std::str::FromStr for Command {
             "skeleton" => Command::Skeleton,
             "start" => Command::Start,
             "localnet" => Command::Localnet,
+            "restore" => Command::Restore,
             cmd => {
                 bail!("{cmd} is not a valid command")
             }
@@ -240,6 +263,7 @@ fn run_command(command: Command) -> Result<(), Error> {
                 println!("{ip}");
             }
         }
+        Command::Restore => restore()?,
     };
 
     Ok(())
diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs
index eb8dd21..83b9161 100644
--- a/proxmox-firewall/src/config.rs
+++ b/proxmox-firewall/src/config.rs
@@ -1,7 +1,8 @@
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashMap};
 use std::default::Default;
 use std::fs::{self, DirEntry, File, ReadDir};
 use std::io::{self, BufRead, BufReader};
+use std::path::{Path, PathBuf};
 
 use anyhow::{Context, Error, bail, format_err};
 
@@ -60,15 +61,16 @@ impl PveFirewallConfigLoader {
 /// opens a configuration file
 ///
 /// It returns a file handle to the file or [`None`] if it doesn't exist.
-fn open_config_file(path: &str) -> Result<Option<File>, Error> {
-    match File::open(path) {
+fn open_config_file<P: Into<PathBuf>>(path: P) -> Result<Option<File>, Error> {
+    let path: PathBuf = path.into();
+    match File::open(&path) {
         Ok(data) => Ok(Some(data)),
         Err(err) if err.kind() == io::ErrorKind::NotFound => {
-            log::info!("config file does not exist: {path}");
+            log::info!("config file does not exist: {path:?}");
             Ok(None)
         }
         Err(err) => {
-            let context = format!("unable to open configuration file at {path}");
+            let context = format!("unable to open configuration file at {path:?}");
             Err(anyhow::Error::new(err).context(context))
         }
     }
@@ -155,7 +157,7 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
     ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
         log::info!("loading guest #{vmid} config");
 
-        let fd = open_config_file(&GuestMap::config_path(vmid, entry))?;
+        let fd = open_config_file(GuestMap::config_path(vmid, entry))?;
 
         if let Some(file) = fd {
             let buf_reader = Box::new(BufReader::new(file)) as Box<dyn io::BufRead>;
@@ -168,7 +170,7 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
     fn guest_firewall_config(&self, vmid: &Vmid) -> Result<Option<Box<dyn io::BufRead>>, Error> {
         log::info!("loading guest #{vmid} firewall config");
 
-        let fd = open_config_file(&GuestMap::firewall_config_path(vmid))?;
+        let fd = open_config_file(GuestMap::firewall_config_path(vmid))?;
 
         if let Some(file) = fd {
             let buf_reader = Box::new(BufReader::new(file)) as Box<dyn io::BufRead>;
@@ -230,7 +232,7 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
     ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
         log::info!("loading firewall config for bridge {bridge_name}");
 
-        let fd = open_config_file(&format!("/etc/pve/sdn/firewall/{bridge_name}.fw"))?;
+        let fd = open_config_file(format!("/etc/pve/sdn/firewall/{bridge_name}.fw"))?;
 
         if let Some(file) = fd {
             let buf_reader = Box::new(BufReader::new(file)) as Box<dyn io::BufRead>;
@@ -247,6 +249,81 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
     }
 }
 
+#[derive(Debug, Default)]
+pub struct LocalFirewallConfigLoader {}
+
+impl LocalFirewallConfigLoader {
+    pub fn new() -> Self {
+        Self::default()
+    }
+}
+
+fn load_first_of(candidates: &[&str], what: &str) -> Result<Option<Box<dyn BufRead>>, Error> {
+    for p in candidates.iter().map(|p| Path::new(DUMP_DIR).join(p)) {
+        if let Some(fd) = open_config_file(&p)? {
+            log::info!("loaded {what} config from {p:?}");
+            let reader = Box::new(BufReader::new(fd)) as Box<dyn BufRead>;
+            return Ok(Some(reader));
+        }
+    }
+    Ok(None)
+}
+
+impl FirewallConfigLoader for LocalFirewallConfigLoader {
+    fn cluster(&self) -> Result<Option<Box<dyn BufRead>>, Error> {
+        load_first_of(&["cluster.fw.override", "cluster.fw"], "cluster")
+    }
+
+    fn host(&self) -> Result<Option<Box<dyn BufRead>>, Error> {
+        load_first_of(&["host.fw.override", "host.fw.new", "host.fw"], "host")
+    }
+
+    fn sdn_running_config(&self) -> Result<Option<Box<dyn BufRead>>, Error> {
+        let path = Path::new(DUMP_DIR).join("sdn.json");
+        if let Some(fd) = open_config_file(&path)? {
+            let reader = Box::new(BufReader::new(fd)) as Box<dyn BufRead>;
+            return Ok(Some(reader));
+        }
+
+        Ok(None)
+    }
+
+    fn guest_config(
+        &self,
+        _: &Vmid,
+        _: &GuestEntry,
+    ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
+        Ok(None)
+    }
+
+    fn guest_firewall_config(&self, _: &Vmid) -> Result<Option<Box<dyn io::BufRead>>, Error> {
+        Ok(None)
+    }
+
+    fn guest_list(&self) -> Result<GuestMap, Error> {
+        Ok(GuestMap::from(HashMap::new()))
+    }
+
+    fn bridge_firewall_config(
+        &self,
+        _: &BridgeName,
+    ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
+        Ok(None)
+    }
+
+    fn bridge_list(&self) -> Result<Vec<BridgeName>, Error> {
+        Ok(vec![])
+    }
+
+    fn ipam(&self) -> Result<Option<Box<dyn io::BufRead>>, Error> {
+        Ok(None)
+    }
+
+    fn interface_mapping(&self) -> Result<AltnameMapping, Error> {
+        Ok(AltnameMapping::from_iter([]))
+    }
+}
+
 pub trait NftConfigLoader {
     fn chains(&self) -> Result<Option<CommandOutput>, Error>;
 }
@@ -271,6 +348,24 @@ impl NftConfigLoader for PveNftConfigLoader {
     }
 }
 
+#[derive(Debug, Default)]
+pub struct LocalNftConfigLoader {}
+
+impl LocalNftConfigLoader {
+    pub fn new() -> Self {
+        Self::default()
+    }
+}
+
+impl NftConfigLoader for LocalNftConfigLoader {
+    fn chains(&self) -> Result<Option<CommandOutput>, Error> {
+        let commands = Commands::new(vec![List::chains()]);
+
+        NftClient::run_json_commands(&commands)
+            .with_context(|| "unable to query nft chains".to_string())
+    }
+}
+
 pub struct FirewallSdnConfig {
     _config: SdnConfig,
     ipsets: BTreeMap<String, Ipset>,
-- 
2.47.3

[PATCH proxmox-firewall 13/13] fix #5759: firewall: restore from dumped config before network-pre

[Arthur Bied-Charreton]

proxmox-firewall depends on pve-cluster, pve-cluster depends on
corosync, which itself depends on network-online. This creates a
boot-time window where the network is up on the PVE host without it
having any firewall protection.

To address this, add a simple oneshot service depending on
network-pre [0] that applies the last remembered rules to bridge the
gap until the configured firewall daemon takes over.

[0] https://systemd.io/NETWORK_ONLINE/

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5759
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 debian/proxmox-firewall-pre-network.service | 16 ++++++++++++++++
 debian/rules                                |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100644 debian/proxmox-firewall-pre-network.service

diff --git a/debian/proxmox-firewall-pre-network.service b/debian/proxmox-firewall-pre-network.service
new file mode 100644
index 0000000..83c686f
--- /dev/null
+++ b/debian/proxmox-firewall-pre-network.service
@@ -0,0 +1,16 @@
+[Unit]
+Description=Proxmox VE Pre-Network NFT Firewall
+Wants=network-pre.target
+Before=network-pre.target shutdown.target
+Conflicts=shutdown.target
+DefaultDependencies=no
+After=local-fs.target
+
+[Service]
+Type=oneshot
+ExecStart=/usr/libexec/proxmox/proxmox-firewall restore
+RemainAfterExit=true
+Environment=PVE_LOG=info
+
+[Install]
+WantedBy=sysinit.target
diff --git a/debian/rules b/debian/rules
index 0dc4e0f..99a6797 100755
--- a/debian/rules
+++ b/debian/rules
@@ -28,4 +28,4 @@ override_dh_auto_configure:
 
 override_dh_installsystemd:
 	dh_installsystemd proxmox-firewall.service
-
+	dh_installsystemd --no-start --name proxmox-firewall-pre-network proxmox-firewall-pre-network.service
-- 
2.47.3