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

[pve-devel] [PATCH v2 qemu-server manager docs] 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.

Changes from v1:
    - Switch to purely checking for VM.Config.CDROM 
      (discussed offlist)
    - Document VM.Config.Cloudinit in docs
    - Whitespaces fixes

manager:

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

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

qemu-server:

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(-)

docs:

Leo Nunner (1):
  Document VM.Config.Cloudinit permission

 pveum.adoc | 1 +
 1 file changed, 1 insertion(+)

-- 
2.30.2

[pve-devel] [PATCH v2 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.CDROM, VM.Config.Cloudinit) from adding
a new cloud-init drive. Some checks needed to be adapted so that editing
cloud-init devices was possible with VM.Config.CDROM instead of
VM.Config.Disk.

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

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 6e9d03b4..121b0f53 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -569,6 +569,8 @@ 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 noVMConfigCDROMPerm = !caps.vms['VM.Config.CDROM'];
+	    const noVMConfigCloudinitPerm = !caps.vms['VM.Config.Cloudinit'];
 
 	    me.down('#addUsb').setDisabled(noSysConsolePerm || isAtLimit('usb'));
 	    me.down('#addPci').setDisabled(noSysConsolePerm || isAtLimit('hostpci'));
@@ -578,7 +580,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(noVMConfigCDROMPerm || noVMConfigCloudinitPerm || hasCloudInit);
 
 	    if (!rec) {
 		remove_btn.disable();
@@ -594,11 +596,11 @@ Ext.define('PVE.qemu.HardwareView', {
 	    const pending = deleted || me.hasPendingChanges(key);
 
 	    const isCloudInit = isCloudInitKey(value);
-	    const isCDRom = value && !!value.toString().match(/media=cdrom/) && !isCloudInit;
+	    const isCDRom = value && !!value.toString().match(/media=cdrom/);
 
 	    const isUnusedDisk = key.match(/^unused\d+/);
-	    const isUsedDisk = !isUnusedDisk && row.isOnStorageBus && !isCDRom && !isCloudInit;
-	    const isDisk = isCloudInit || isUnusedDisk || isUsedDisk;
+	    const isUsedDisk = !isUnusedDisk && row.isOnStorageBus && !isCDRom;
+	    const isDisk = isUnusedDisk || isUsedDisk;
 	    const isEfi = key === 'efidisk0';
 	    const tpmMoveable = key === 'tpmstate0' && !me.pveSelNode.data.running;
 
@@ -701,7 +703,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.CDROM'] || !caps.vms['VM.Config.Cloudinit'],
 				handler: editorFactory('CIDriveEdit'),
 			    },
 			    {
-- 
2.30.2

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

[Dominik Csapak]

applied, thanks

made a short follow up to also disable the remove button for cloudinit drives
if one does not have cloud init permissions

[pve-devel] [PATCH v2 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.

Due to the regex letting names fall through before, Cloud-init drives
were being checked as disks, even though they are actually treated as
CDROM drives. Due to this, it makes more sense to check for
VM.Config.CDROM instead, while also requiring VM.Config.Cloudinit, since
generating a Cloud-init drive already generates default values that are
passed to the VM.

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 30348e6..7453ecb 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1623,11 +1623,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.CDROM']);
+		} 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] applied: [PATCH v2 qemu-server 1/1] fix #4321: properly check cloud-init drive permissions

[Thomas Lamprecht]

Am 16/11/2022 um 18:34 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.
> 
> Due to the regex letting names fall through before, Cloud-init drives
> were being checked as disks, even though they are actually treated as
> CDROM drives. Due to this, it makes more sense to check for
> VM.Config.CDROM instead, while also requiring VM.Config.Cloudinit, since
> generating a Cloud-init drive already generates default values that are
> passed to the VM.
> 
> 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(-)
> 
>

applied, thanks!

[pve-devel] [PATCH docs 1/1] Document VM.Config.Cloudinit permission

[Leo Nunner]

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 pveum.adoc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pveum.adoc b/pveum.adoc
index 52de14d..65d874a 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -781,6 +781,7 @@ Virtual machine related privileges::
 * `VM.Config.Network`: add/modify/remove network devices
 * `VM.Config.HWType`: modify emulated hardware types
 * `VM.Config.Options`: modify any other VM configuration
+* `VM.Config.Cloudinit`: modify Cloud-init parameters
 * `VM.Snapshot`: create/delete VM snapshots
 
 Storage related privileges::
-- 
2.30.2

[pve-devel] applied: [PATCH docs 1/1] Document VM.Config.Cloudinit permission

[Thomas Lamprecht]

Am 16/11/2022 um 18:34 schrieb Leo Nunner:
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
>  pveum.adoc | 1 +
>  1 file changed, 1 insertion(+)
> 
>

applied, thanks!