[pve-devel] [PATCH storage 3/3] Mask world rwx and group wx for newly allocated images and when converting to base image

[Fabian Ebner]

Following the rationale in afdfbe5594be5a0a61943de10cc5671ac53cbf79, mask
these bits for 'clone_image' and 'volume_import'. Also mask in 'chmod' for
new base images for consistency.

Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
---

This would make the permissions more consistent, but is not really important.

We need to be careful to not mask subvols though,
see 1f5734bb8d8397013615c368b8845abb3e74bab5.
The generic 'volume_import' uses tar for subvols and the other plugins with
their own 'volume_import', i.e. both LVM and both ZFS, are not affected by
umask AFAICT. And for 'clone_image' only LVMThin, RBD, and ZFS allow subvols
and those are not affected by umask AFAICT.

The obvious and maybe nicer alternative would be to instead do the masking
inside those plugins where umask actually has an effect. If I'm not missing
anything, it wouldn't be many places, only in the generic Plugin.pm
(alloc_image, clone_image, create_base, volume_import) and in Glusterfs.pm
(alloc_image and clone_image). The other FS-based plugins don't override
thes methods.

 PVE/Storage.pm        | 10 +++++++---
 PVE/Storage/Plugin.pm |  2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 3e65e06..9b1b914 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -675,7 +675,9 @@ sub vdisk_clone {
 
     # lock shared storage
     return $plugin->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
-	my $volname = $plugin->clone_image($scfg, $storeid, $volname, $vmid, $snap);
+	my $volname = $run_with_umask->(umask|0037, sub {
+	    return $plugin->clone_image($scfg, $storeid, $volname, $vmid, $snap);
+	});
 	return "$storeid:$volname";
     });
 }
@@ -1412,8 +1414,10 @@ sub volume_import {
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
     return $plugin->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
-	return $plugin->volume_import($scfg, $storeid, $fh, $volname, $format,
-				      $base_snapshot, $with_snapshots);
+	return $run_with_umask->(umask|0037, sub {
+	    return $plugin->volume_import($scfg, $storeid, $fh, $volname, $format,
+					  $base_snapshot, $with_snapshots);
+	});
     });
 }
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index d0f1df6..1babf34 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -521,7 +521,7 @@ sub create_base {
 
     # We try to protect base volume
 
-    chmod(0444, $newpath); # nobody should write anything
+    chmod(0440, $newpath); # nobody should write anything
 
     # also try to set immutable flag
     eval { run_command(['/usr/bin/chattr', '+i', $newpath]); };
-- 
2.20.1