Re: [pve-devel] [PATCH qemu-server v3 08/13] PVE/API2/Qemu: add permission checks for mapped pci devices

[Dominik Csapak <d.csapak@proxmox.com>]

On 11/9/22 13:14, Fabian Grünbichler wrote:
> On September 20, 2022 2:50 pm, Dominik Csapak wrote:
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/API2/Qemu.pm | 54 ++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 52 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 7afd7a4..d6d393f 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -26,6 +26,7 @@ use PVE::QemuServer::Drive;
>>   use PVE::QemuServer::ImportDisk;
>>   use PVE::QemuServer::Monitor qw(mon_cmd);
>>   use PVE::QemuServer::Machine;
>> +use PVE::QemuServer::PCI;
>>   use PVE::QemuServer::USB qw(parse_usb_device);
>>   use PVE::QemuMigrate;
>>   use PVE::RPCEnvironment;
>> @@ -603,6 +604,26 @@ my $check_vm_create_usb_perm = sub {
>>       return 1;
>>   };
>>   
>> +my $check_vm_create_hostpci_perm = sub {
>> +    my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
>> +
>> +    return 1 if $authuser eq 'root@pam';
>> +
>> +    foreach my $opt (keys %{$param}) {
>> +	next if $opt !~ m/^hostpci\d+$/;
>> +
>> +	my $device = PVE::JSONSchema::parse_property_string('pve-qm-hostpci', $param->{$opt});
>> +	if ($device->{host} !~ m/:/) {
> 
> while thinking about the priv patch I decided to check out the ACL handling as
> well - sorry for not asking earlier about this aspect!
> 
> would it make sense to have a prefix for "ID of mapped hardware" instead of
> claiming "everything that doesn't contain ':'" as namespace?

i mean we could, but a few downsides come to mind:
* if we prefix the ids in the hardware-map itself, the user always
   sees that prefix, which is useless to them, or we have to remove it
   everywhere when displaying, which is imho unnecessary work
* if we only prefix it in the vm config, we have to add/remove it
   in the gui/cli (or api?) which also means users that edit
   the config don't have the direct correlation with the hardware map

basically, more work for us all around

for usb devices we actually check the vendor/id/path regex first, then 
for spice (which is the only exception to the namespace clash) and
all other values we interpret as mapped devices
(imho the small downside of not being able to name a mapping 'spice'
is worth the work we save by not introducing a prefix/namespace)

we could do the same for pci ids of course, i just used the shortcut
of having ':'

ofc if you insist on having some separated namespace by prefix (or
similar), i'll implement that, i'm not very attached to the current
implementation ;)

> 
> the same also applies to the USB ACL checks and the other checks in this patch..
> 
>> +	    $rpcenv->check_full($authuser, "/hardware/$device->{host}", ['Hardware.Use']);
> 
> this and similar sites would then also be more explicit:
> 
> my $hw_id = ..; # extract actual ID
> $rpvenv->check_full($authuser, "/hardware/$hw_id", ['Hardware.Use']);
> 
>> +	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
>> +	} else {
>> +	    die "only root can set '$opt' config for non-mapped devices\n";
>> +	}
>> +    }
>> +
>> +    return 1;
>> +};
>> +
>>
>> [...]
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>