[pve-devel] [PATCH qemu-server 1/2] PVE/API2/Qemu/update_vm_api: refactor drive permission check

[pve-devel] [PATCH qemu-server 1/2] PVE/API2/Qemu/update_vm_api: refactor drive permission check

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Qemu.pm | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 005e655..057b8ff 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1263,6 +1263,17 @@ my $update_vm_api  = sub {
 	    }
 	    my $bootorder_deleted = grep {$_ eq 'bootorder'} @delete;
 
+	    my $check_drive_perms = sub {
+		my ($opt, $val) = @_;
+		my $drive = PVE::QemuServer::parse_drive($opt, $val);
+		# FIXME: cloudinit: CDROM or Disk?
+		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
+		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
+		} else {
+		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
+		}
+	    };
+
 	    foreach my $opt (@delete) {
 		$modified->{$opt} = 1;
 		$conf = PVE::QemuConfig->load_config($vmid); # update/reload
@@ -1300,12 +1311,7 @@ my $update_vm_api  = sub {
 		    }
 		} elsif (PVE::QemuServer::is_valid_drivename($opt)) {
 		    PVE::QemuConfig->check_protection($conf, "can't remove drive '$opt'");
-		    my $drive = PVE::QemuServer::parse_drive($opt, $val);
-		    if (PVE::QemuServer::drive_is_cdrom($drive)) {
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
-		    } else {
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
-		    }
+		    $check_drive_perms->($opt, $val);
 		    PVE::QemuServer::vmconfig_register_unused_drive($storecfg, $vmid, $conf, PVE::QemuServer::parse_drive($opt, $val))
 			if $is_pending_val;
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
@@ -1340,13 +1346,7 @@ my $update_vm_api  = sub {
 		my $arch = PVE::QemuServer::get_vm_arch($conf);
 
 		if (PVE::QemuServer::is_valid_drivename($opt)) {
-		    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
-		    # FIXME: cloudinit: CDROM or Disk?
-		    if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
-		    } else {
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
-		    }
+		    $check_drive_perms->($opt, $param->{$opt});
 		    PVE::QemuServer::vmconfig_register_unused_drive($storecfg, $vmid, $conf, PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}))
 			if defined($conf->{pending}->{$opt});
 
-- 
2.20.1

[pve-devel] [PATCH qemu-server 2/2] fix #2175: PVE/API2/Qemu: update_vm_api: check old drive for permissions too

[Dominik Csapak]

otherwise a user with only VM.Config.CDROM can detach a disk from a VM
by updating it to a cdrom drive

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
this is breaking api change, but fits more in line what we would
want for the permissions

 PVE/API2/Qemu.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 057b8ff..1e540f5 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1346,6 +1346,12 @@ my $update_vm_api  = sub {
 		my $arch = PVE::QemuServer::get_vm_arch($conf);
 
 		if (PVE::QemuServer::is_valid_drivename($opt)) {
+		    # old drive
+		    if ($conf->{$opt}) {
+			$check_drive_perms->($opt, $conf->{$opt});
+		    }
+
+		    # new drive
 		    $check_drive_perms->($opt, $param->{$opt});
 		    PVE::QemuServer::vmconfig_register_unused_drive($storecfg, $vmid, $conf, PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}))
 			if defined($conf->{pending}->{$opt});
-- 
2.20.1

[pve-devel] applied: [PATCH qemu-server 1/2] PVE/API2/Qemu/update_vm_api: refactor drive permission check

[Thomas Lamprecht]

On 24.06.21 16:25, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
>

applied both patches, thanks!