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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 9, 2022 1:51 pm, Dominik Csapak wrote:
> 
> 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

I meant the latter ;) I think with something like "hw_map:NAME" it would be
pretty clear. and yeah, it would need to be added client/GUI side, not in the
API else it would be really confusing..

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

there is a second downside - we don't have a way to add another non-mapping
value that uses a valid (i.e., readable!) name. obviously doesn't matter if we
are sure no such values have to be added in the future (or if we don't care that
we need to encode them in way that makes them live in a different namespace than
the map entry names, which would in turn restrict future extensions of the name
schema there).

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

no, I don't insist on it - just wanted to make sure it's a conscious decision
and not an oversight :) when in doubt, I'd rather err on the side of having a
simple, understandable prefix (preserving future extensibility).

what actually originally prompted me was that I find "/hardware/$device->{host}"
to be a really weird ACL path, as it's not immediately obvious that "host" here
is overloaded to either mean the original host-hardware-ID or the newly added
name of the map entry. having it prefixed with a helper to check for and extract
the name would make the code more readable IMHO.

>> 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;
>>> +};
>>> +
>>>
>>> [...]