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

[Leo Nunner <l.nunner@proxmox.com>]

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