[PATCH cluster v4 0/2] pmxcfs fix backup directory permissions

[PATCH cluster v4 0/2] pmxcfs fix backup directory permissions

[Shannon Sterz]

Changelog
---------

changes since v3 (thanks @ Thomas Lamprecht):
* moved changing the permissions of the backup directory when creating
  a backup to commit #1
* use in-built `rename` instead of `File::Copy::move`

changes since v2 (thanks @ Thomas Lamprecht):
* use chmod to always set the MODE of the backup directory before
  backing up.
* rotate the old config.db out of the way of the new one so we can
  continue a cluster join even if a backup fails.

changes since v1 (thanks @ Thomas Lamprecht):
* added a patch fixing the mkdir call in `cfs_backup_database`
* version guarded the permission fix up in the post install hook
* dropped the `-R` flag provided to the `chmod` call in the postinst
  hook (it did not provide any extra security and made lintian complain)

Shannon Sterz (2):
  pmxcfs: remove world-readable permissions from backups
  pmxcfs: don't abort join when backup fails and keep old config
    database

 debian/pve-cluster.dirs     |  1 +
 debian/pve-cluster.postinst |  8 ++++++++
 src/PVE/Cluster.pm          | 10 ++++++++--
 src/PVE/Cluster/Setup.pm    |  5 +++--
 4 files changed, 20 insertions(+), 4 deletions(-)

--
2.47.3

[PATCH cluster v4 1/2] pmxcfs: remove world-readable permissions from backups

[Shannon Sterz]

this could lead to information disclosure of data that is private
within pmxcfs. currently the impact is low as a backup is only
triggered before joining a new cluster. however, if we trigger more
backups going forward, this could leak sensitive information.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---

the version in the postinst hook should probably be adapted when
pve-cluster is bumped.

 debian/pve-cluster.dirs     | 1 +
 debian/pve-cluster.postinst | 8 ++++++++
 src/PVE/Cluster.pm          | 5 +++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/debian/pve-cluster.dirs b/debian/pve-cluster.dirs
index a07aa4e..df4a258 100644
--- a/debian/pve-cluster.dirs
+++ b/debian/pve-cluster.dirs
@@ -1,2 +1,3 @@
 /usr/bin
 /var/lib/pve-cluster
+/var/lib/pve-cluster/backup
diff --git a/debian/pve-cluster.postinst b/debian/pve-cluster.postinst
index 34bf0e1..048199e 100644
--- a/debian/pve-cluster.postinst
+++ b/debian/pve-cluster.postinst
@@ -11,12 +11,20 @@ remove_fabrics_directory() {
   fi
 }

+update_permissions() {
+    chmod 0600 /var/lib/pve-cluster/backup;
+}
+
 case "$1" in
   configure)
     # TODO: remove with PVE 10+
     if dpkg --compare-versions "$2" 'lt' '9.0.1'; then
       remove_fabrics_directory
     fi
+
+    if dpkg --compare-versions "$2" 'lt' '9.1.1'; then
+      update_permissions
+    fi
   ;;
 esac

diff --git a/src/PVE/Cluster.pm b/src/PVE/Cluster.pm
index bdb465f..e96a7fe 100644
--- a/src/PVE/Cluster.pm
+++ b/src/PVE/Cluster.pm
@@ -9,7 +9,7 @@ use File::Path qw(make_path);
 use JSON;
 use List::Util;
 use Net::SSLeay;
-use POSIX qw(ENOENT);
+use POSIX qw(ENOENT EEXIST);
 use Socket;
 use Storable qw(dclone);
 use Time::HiRes qw(usleep);
@@ -892,7 +892,8 @@ sub complete_migration_target {

 # NOTE: filesystem must be offline here, no DB changes allowed
 sub cfs_backup_database {
-    mkdir $dbbackupdir;
+    mkdir $dbbackupdir or $!{EEXIST} or die "failed to create backup dir - $!\n";
+    chmod 0600, $dbbackupdir or die "failed to change mode for backup dir - $!\n";

     my $ctime = time();
     my $backup_fn = "$dbbackupdir/config-$ctime.sql.gz";
--
2.47.3

[PATCH cluster v4 2/2] pmxcfs: don't abort join when backup fails and keep old config database

[Shannon Sterz]

by moving it out of the way of the new database instead of unlinking
it. this should improve situations where a failed backup would leave
the cluster that is being joined in an inconsistent state, while
making sure that the pre-existing database on the joining node stays
around.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 src/PVE/Cluster.pm       | 5 +++++
 src/PVE/Cluster/Setup.pm | 5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Cluster.pm b/src/PVE/Cluster.pm
index e96a7fe..e678479 100644
--- a/src/PVE/Cluster.pm
+++ b/src/PVE/Cluster.pm
@@ -917,4 +917,9 @@ sub cfs_backup_database {
     return $dbfile;
 }
 
+# NOTE: Should only be called on cluster join to clean up a database pre-join.
+sub cfs_move_existing_db {
+    rename $dbfile, "$dbfile.bak";
+}
+
 1;
diff --git a/src/PVE/Cluster/Setup.pm b/src/PVE/Cluster/Setup.pm
index 75d3507..9de6235 100644
--- a/src/PVE/Cluster/Setup.pm
+++ b/src/PVE/Cluster/Setup.pm
@@ -808,8 +808,9 @@ sub finish_join {
     my $cmd = ['systemctl', 'stop', 'pve-cluster'];
     PVE::Tools::run_command($cmd, errmsg => "can't stop pve-cluster service");
 
-    my $dbfile = PVE::Cluster::cfs_backup_database();
-    unlink $dbfile;
+    eval { PVE::Cluster::cfs_backup_database(); };
+    warn $@ if $@;
+    PVE::Cluster::cfs_move_existing_db();
 
     $cmd = ['systemctl', 'start', 'corosync', 'pve-cluster'];
     PVE::Tools::run_command($cmd, errmsg => "starting pve-cluster failed");
-- 
2.47.3

applied: [PATCH cluster v4 0/2] pmxcfs fix backup directory permissions

[Thomas Lamprecht]

On Wed, 18 Mar 2026 12:11:09 +0100, Shannon Sterz wrote:
> Changelog
> ---------
> 
> changes since v3 (thanks @ Thomas Lamprecht):
> * moved changing the permissions of the backup directory when creating
>   a backup to commit #1
> * use in-built `rename` instead of `File::Copy::move`
> 
> [...]

Applied, with some adation fleeced in (and recorded them in the commit
message), thanks!

[1/2] pmxcfs: remove world-readable permissions from backups
      commit: efc6f66cfa3f7aed4ed8c9eac8c03deecebb3450
[2/2] pmxcfs: don't abort join when backup fails and keep old config database
      commit: 004742c3c39c97b65dc6d10e8d021c8193a7fd16