Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk

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

On March 22, 2022 10:31 am, Fabian Ebner wrote:
> Am 22.03.22 um 09:31 schrieb Fabian Ebner:
>> Am 21.03.22 um 14:06 schrieb Fabian Ebner:
>>> Listing guest images should not require Datastore.Allocate in this
>>> case. In preparation for adding disk import to the GUI.
>>>
>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>> ---
>>>  PVE/Storage.pm | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>> index 6112991..efa304a 100755
>>> --- a/PVE/Storage.pm
>>> +++ b/PVE/Storage.pm
>>> @@ -486,6 +486,8 @@ sub check_volume_access {
>>>  	} elsif ($vtype eq 'backup' && $ownervm) {
>>>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
>>>  	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> 
> @Fabian G. should access to backups also be allowed if the user /just/
> has Datastore.Allocate?
> 
> Otherwise, backups cannot be listed or removed (there is a separate
> check, but works similarly) and attributes cannot be changed by a
> supposedly high-privileged user.

yeah, I think Datastore.Allocate could be allowed here, it's documented 
as:

Datastore.Allocate: create/modify/remove a datastore and delete volumes

but in practice, any user that has Datastore.Allocate likely also has 
Datastore.AllocateSpace anyway which is probably why nobody complained 
yet ;)

> On the other hand, we also use this check for extractconfig, where it
> makes sense to be limited to users with VM.Backup, but could be changed
> at the call site of course.

IMHO same as above applies here, and I think the idea here was 'if you have 
VM.Backup and Datastore.AllocateSpace you are allowed to access "your 
own" backups, even if you can't access the whole range of files on the 
storage', the code was just not very thought through (and in practice, 
high-privileged users on storages are usually also high-privileged users 
on guests, so nobody noticed/cared).

I think adding an early return with the check from the else branch with 
$noerr set and replacing the else branch with a `die` would be fine (so 
Datastore admins are always allowed to access all volumes on their 
storages).

>>> +	} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
>>> +	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
>> 
>> Of course this needs to be or-ed with the Datastore.Allocate privilege.
>> Will fix it in v2.

and and-ed with Datastore.AllocateSpace?

>> 
>>>  	} else {
>>>  	    # allow if we are Datastore administrator
>>>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']);
>> 
>> 
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>> 
>> 
>