[pve-devel] [PATCH qemu-server manager] Properly check cloud-init drive permissions

[pve-devel] [PATCH qemu-server manager] Properly check cloud-init drive permissions

[Leo Nunner]

The "Add CloudInit Drive" button in the UI was only enabled when the user
had Sys.Console permissions, even if the other correct permissions were 
set. The backend check for these permissions also needed to be revamped 
to be more consistent.

Leo Nunner (1):
  fix #4321: properly check cloud-init drive permissions

 PVE/API2/Qemu.pm        | 6 ++++--
 PVE/QemuServer/Drive.pm | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

Leo Nunner (1):
  fix #4321: properly check cloud-init drive permissions

 www/manager6/qemu/HardwareView.js | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/1] fix #4321: properly check cloud-init drive permissions

[Leo Nunner]

The process for editing cloud-init drives checked for inconsistent
permissions: for adding, the VM.Config.Disk permission was needed, while
the VM.Config.CDROM permission was needed to remove a drive. The regex
in drive_is_cloudinit needed to be adapted since the drive names have
different formats before/after they are actually generated. Furthermore,
the VM.Config.Cloudinit permission should be required to edit
cloud-init drives.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 PVE/API2/Qemu.pm        | 6 ++++--
 PVE/QemuServer/Drive.pm | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 3ec31c2..cacd233 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1512,11 +1512,13 @@ my $update_vm_api  = sub {
 	    my $check_drive_perms = sub {
 		my ($opt, $val) = @_;
 		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
-		# FIXME: cloudinit: CDROM or Disk?
-		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
+		if(PVE::QemuServer::drive_is_cloudinit($drive)) {
+		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Cloudinit', 'VM.Config.Disk']);
+		} elsif (PVE::QemuServer::drive_is_cdrom($drive, 1)) { # CDROM
 		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
 		} else {
 		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
+
 		}
 	    };
 
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 1dc6171..12a1fbe 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -540,7 +540,7 @@ sub verify_bootdisk {
 
 sub drive_is_cloudinit {
     my ($drive) = @_;
-    return $drive->{file} =~ m@[:/]vm-\d+-cloudinit(?:\.$QEMU_FORMAT_RE)?$@;
+    return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@;
 }
 
 sub drive_is_cdrom {
-- 
2.30.2

[pve-devel] [PATCH manager 1/1] fix #4321: properly check cloud-init drive permissions

[Leo Nunner]

Checking for only Sys.Console prevents users who actually have the
correct permissions (VM.Config.Disk, VM.Config.Cloudinit) from adding
a new cloud-init drive.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 6e9d03b4..c7843b82 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -569,6 +569,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	    const noVMConfigHWTypePerm = !caps.vms['VM.Config.HWType'];
 	    const noVMConfigNetPerm = !caps.vms['VM.Config.Network'];
 	    const noVMConfigDiskPerm = !caps.vms['VM.Config.Disk'];
+	    const noVMConfigCloudinitPerm = !caps.vms['VM.Config.Cloudinit'];
 
 	    me.down('#addUsb').setDisabled(noSysConsolePerm || isAtLimit('usb'));
 	    me.down('#addPci').setDisabled(noSysConsolePerm || isAtLimit('hostpci'));
@@ -578,7 +579,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	    me.down('#addRng').setDisabled(noSysConsolePerm || isAtLimit('rng'));
 	    efidisk_menuitem.setDisabled(noVMConfigDiskPerm || isAtLimit('efidisk'));
 	    me.down('#addTpmState').setDisabled(noSysConsolePerm || isAtLimit('tpmstate'));
-	    me.down('#addCloudinitDrive').setDisabled(noSysConsolePerm || hasCloudInit);
+	    me.down('#addCloudinitDrive').setDisabled(noVMConfigDiskPerm || noVMConfigCloudinitPerm || hasCloudInit);
 
 	    if (!rec) {
 		remove_btn.disable();
@@ -701,7 +702,7 @@ Ext.define('PVE.qemu.HardwareView', {
 				text: gettext('CloudInit Drive'),
 				itemId: 'addCloudinitDrive',
 				iconCls: 'fa fa-fw fa-cloud black',
-				disabled: !caps.nodes['Sys.Console'],
+				disabled: !caps.vms['Vm.Config.Disk'] || !caps.vms['Vm.Config.Cloudinit'],
 				handler: editorFactory('CIDriveEdit'),
 			    },
 			    {
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 1/1] fix #4321: properly check cloud-init drive permissions

[Thomas Lamprecht]

Am 03/11/2022 um 14:04 schrieb Leo Nunner:
> The process for editing cloud-init drives checked for inconsistent
> permissions: for adding, the VM.Config.Disk permission was needed, while
> the VM.Config.CDROM permission was needed to remove a drive. The regex
> in drive_is_cloudinit needed to be adapted since the drive names have
> different formats before/after they are actually generated. Furthermore,
> the VM.Config.Cloudinit permission should be required to edit
> cloud-init drives.
> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
>  PVE/API2/Qemu.pm        | 6 ++++--
>  PVE/QemuServer/Drive.pm | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 3ec31c2..cacd233 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -1512,11 +1512,13 @@ my $update_vm_api  = sub {
>  	    my $check_drive_perms = sub {
>  		my ($opt, $val) = @_;
>  		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
> -		# FIXME: cloudinit: CDROM or Disk?
> -		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
> +		if(PVE::QemuServer::drive_is_cloudinit($drive)) {

missing white after `if`

> +		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Cloudinit', 'VM.Config.Disk']);

while I don't think that the perm change is problematic w.r.t. backward compat
in practice, it seems a bit odd to argue for disk only, maybe we need to also
check for CDROM, as cloudinit *adds* a cdrom drive, so it may required that too.

But, to decide that I wanted to check our privilege docs, only to notice that we
do not mention the cloudinit one there at all, not great...

https://pve.proxmox.com/pve-docs/chapter-pveum.html#_privileges

Please check the dev history to see if Cloudinit is deemed enough to add the CDROM
or if we should add that priv to the check too, then re-send this with updated
commit message, the whitespace fixes and a docs patch.

Re: [pve-devel] [PATCH qemu-server 1/1] fix #4321: properly check cloud-init drive permissions

[Leo Nunner]

On 2022-11-16 13:15, Thomas Lamprecht wrote:
> Am 03/11/2022 um 14:04 schrieb Leo Nunner:
>> The process for editing cloud-init drives checked for inconsistent
>> permissions: for adding, the VM.Config.Disk permission was needed, while
>> the VM.Config.CDROM permission was needed to remove a drive. The regex
>> in drive_is_cloudinit needed to be adapted since the drive names have
>> different formats before/after they are actually generated. Furthermore,
>> the VM.Config.Cloudinit permission should be required to edit
>> cloud-init drives.
>>
>> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
>> ---
>>   PVE/API2/Qemu.pm        | 6 ++++--
>>   PVE/QemuServer/Drive.pm | 2 +-
>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 3ec31c2..cacd233 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -1512,11 +1512,13 @@ my $update_vm_api  = sub {
>>   	    my $check_drive_perms = sub {
>>   		my ($opt, $val) = @_;
>>   		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
>> -		# FIXME: cloudinit: CDROM or Disk?
>> -		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
>> +		if(PVE::QemuServer::drive_is_cloudinit($drive)) {
> missing white after `if`
>
>> +		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Cloudinit', 'VM.Config.Disk']);
> while I don't think that the perm change is problematic w.r.t. backward compat
> in practice, it seems a bit odd to argue for disk only, maybe we need to also
> check for CDROM, as cloudinit *adds* a cdrom drive, so it may required that too.
>
> But, to decide that I wanted to check our privilege docs, only to notice that we
> do not mention the cloudinit one there at all, not great...
>
> https://pve.proxmox.com/pve-docs/chapter-pveum.html#_privileges
>
> Please check the dev history to see if Cloudinit is deemed enough to add the CDROM
> or if we should add that priv to the check too, then re-send this with updated
> commit message, the whitespace fixes and a docs patch.
>
AFAICT, VM.Config.Cloudinit seems to have been introduced solely to set 
cloudinit options in the config itself [1], so I don't think it should 
be the only requirement for adding cloudinit drives. I agree though, now 
that I think about it, solely checking for VM.Config.Disk as an added 
requirement doesn't make much sense; the only reason why it checked for 
this permission in the first place was that the string for cloudinit 
drives fell through the regex in drive_is_cdrom before being generated, 
and was thus being seen as a disk, not a CDROM drive. So, for 
backwards-compatibility purposes, I guess VM.Config.Cloudinit && 
(VM.Config.Disk || VM.Config.CDROM) would make the most sense.

[1] 
https://git.proxmox.com/?p=qemu-server.git;a=commit;h=fc701af7406336d18e4f20d61010c7de69fdd9f1