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

[Fabian Ebner <f.ebner@proxmox.com>]

Am 24.03.22 um 09:18 schrieb Fabian Grünbichler:
> 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).
> 

Sounds good, I'll go for that in v2.

>>>> +	} 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?
>

I'm not sure. For clone, that's currently not checked (it's enough to
have VM.Clone and Datastore.AllocateSpace on the target storage, and I
kept it consistent with that for the proposed import-from), so it would
be a bit weird if listing the images requires it, while the actual
operation doesn't. But I don't mind adding it, if you want me to?

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