[PATCH-SERIES storage v2 0/2] api: prune backups: improve permission checks

[PATCH-SERIES storage v2 0/2] api: prune backups: improve permission checks

[Fiona Ebner]

Changes in v2:
* Different approach. Do not use check_volume_access() for filtering.
  Aligning the permissions between the dry run and active run is more
  natural here.

Align the permission checks for dry run and active run, since that is
natural here. And remove an unnatural restriction for the case where
an explicit guest ID is specified.

pve-storage:

Fiona Ebner (2):
  api: prune backups: align permission check for dry run and active run
  api: prune backups: allow Datastore.Allocate even without VM.Backup

 src/PVE/API2/Storage/PruneBackups.pm | 37 +++++++++++++++++++---------
 1 file changed, 25 insertions(+), 12 deletions(-)


Summary over all repositories:
  1 files changed, 25 insertions(+), 12 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH storage v2 1/2] api: prune backups: align permission check for dry run and active run

[Fiona Ebner]

The API endpoint for listing storage content filters volumes with
check_volume_access(). The dry run for prune backups should not return
volumes that the content listing does not show. While
check_volume_access() could be used there as well, access to a backup
volume already requires 'Datastore.AllocateSpace' for the storage and
'VM.Backup' for the guest, or 'Datastore.Allocate' for the storage.
These are the same permissions required for the actual pruning.

The dry run should show what an actual pruning invocation with the
same parameters would do. Using check_volume_access() would, for
invocations without an explicit guest ID, show what a storage-wide
prune would do for all guest groups the user has access to. This would
be in contrast to the actual pruning invocation, which requires
'Datastore.Allocate' for an invocation without an explicit guest ID,
so there would be a mismatch.

Together, these points suggest aligning the permission check between
the endpoint for the dry run and the endpoint for the active run.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Storage/PruneBackups.pm | 34 ++++++++++++++++++----------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/src/PVE/API2/Storage/PruneBackups.pm b/src/PVE/API2/Storage/PruneBackups.pm
index 2920c899..7863c36e 100644
--- a/src/PVE/API2/Storage/PruneBackups.pm
+++ b/src/PVE/API2/Storage/PruneBackups.pm
@@ -12,6 +12,19 @@ use PVE::Tools qw(extract_param);
 
 use base qw(PVE::RESTHandler);
 
+my sub assert_prune_permission {
+    my ($rpcenv, $authuser, $storeid, $vmid) = @_;
+
+    if (defined($vmid)) {
+        $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
+        $rpcenv->check($authuser, "/vms/$vmid", ['VM.Backup']);
+    } else {
+        $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.Allocate']);
+    }
+
+    return;
+}
+
 __PACKAGE__->register_method({
     name => 'dryrun',
     path => '',
@@ -20,12 +33,9 @@ __PACKAGE__->register_method({
         "Get prune information for backups. NOTE: this is only a preview and might not be "
         . "what a subsequent prune call does if backups are removed/added in the meantime.",
     permissions => {
-        check => [
-            'perm',
-            '/storage/{storage}',
-            ['Datastore.Audit', 'Datastore.AllocateSpace'],
-            any => 1,
-        ],
+        description => "You need the 'Datastore.Allocate' privilege on the storage "
+            . "(or if a VM ID is specified, 'Datastore.AllocateSpace' and 'VM.Backup' for the VM).",
+        user => 'all',
     },
     protected => 1,
     proxyto => 'node',
@@ -100,6 +110,9 @@ __PACKAGE__->register_method({
     code => sub {
         my ($param) = @_;
 
+        my $rpcenv = PVE::RPCEnvironment::get();
+        my $authuser = $rpcenv->get_user();
+
         my $cfg = PVE::Storage::config();
 
         my $vmid = extract_param($param, 'vmid');
@@ -110,6 +123,8 @@ __PACKAGE__->register_method({
         $prune_backups = PVE::JSONSchema::parse_property_string('prune-backups', $prune_backups)
             if defined($prune_backups);
 
+        assert_prune_permission($rpcenv, $authuser, $storeid, $vmid);
+
         return PVE::Storage::prune_backups($cfg, $storeid, $prune_backups, $vmid, $type, 1);
     },
 });
@@ -177,12 +192,7 @@ __PACKAGE__->register_method({
         $prune_backups = PVE::JSONSchema::parse_property_string('prune-backups', $prune_backups)
             if defined($prune_backups);
 
-        if (defined($vmid)) {
-            $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
-            $rpcenv->check($authuser, "/vms/$vmid", ['VM.Backup']);
-        } else {
-            $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.Allocate']);
-        }
+        assert_prune_permission($rpcenv, $authuser, $storeid, $vmid);
 
         my $id = (defined($vmid) ? "$vmid@" : '') . $storeid;
         my $worker = sub {
-- 
2.47.3

[PATCH storage v2 2/2] api: prune backups: allow Datastore.Allocate even without VM.Backup

[Fiona Ebner]

A user with 'Datastore.Allocate' permissions for the storage can prune
all backups on the storage. If the same user specifies an explicit
guest ID, additionally the 'VM.Backup' permission for the VM was
required. Remove this restriction, since removing a subset should not
require more permissions and is also at odds with the logic in
check_volume_access().

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Storage/PruneBackups.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Storage/PruneBackups.pm b/src/PVE/API2/Storage/PruneBackups.pm
index 7863c36e..5f62e2b7 100644
--- a/src/PVE/API2/Storage/PruneBackups.pm
+++ b/src/PVE/API2/Storage/PruneBackups.pm
@@ -4,6 +4,7 @@ use strict;
 use warnings;
 
 use PVE::Cluster;
+use PVE::Exception qw(raise_perm_exc);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::RESTHandler;
 use PVE::RPCEnvironment;
@@ -15,11 +16,13 @@ use base qw(PVE::RESTHandler);
 my sub assert_prune_permission {
     my ($rpcenv, $authuser, $storeid, $vmid) = @_;
 
+    return if $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.Allocate'], 1);
+
     if (defined($vmid)) {
         $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
         $rpcenv->check($authuser, "/vms/$vmid", ['VM.Backup']);
     } else {
-        $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.Allocate']);
+        raise_perm_exc("/storage/$storeid, Datastore.Allocate");
     }
 
     return;
-- 
2.47.3