From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id E79F16DE86
for <pve-devel@lists.proxmox.com>; Mon, 28 Mar 2022 11:07:58 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id D56FC23E94
for <pve-devel@lists.proxmox.com>; Mon, 28 Mar 2022 11:07:28 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id DBA7323E86
for <pve-devel@lists.proxmox.com>; Mon, 28 Mar 2022 11:07:27 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id A88284274E
for <pve-devel@lists.proxmox.com>; Mon, 28 Mar 2022 11:07:27 +0200 (CEST)
Message-ID: <a35c7af7-2879-6882-e3d2-0dbeac68175f@proxmox.com>
Date: Mon, 28 Mar 2022 11:07:19 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
Thunderbird/91.7.0
Content-Language: en-US
To: =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= <f.gruenbichler@proxmox.com>,
pve-devel@lists.proxmox.com
References: <20220321130633.62086-1-f.ebner@proxmox.com>
<20220321130633.62086-2-f.ebner@proxmox.com>
<159a4067-39b4-c63a-c267-189c908465c3@proxmox.com>
<8f1859ba-c782-7dee-09e6-7fac1561ce74@proxmox.com>
<1648109081.y13k852cx6.astroid@nora.none>
From: Fabian Ebner <f.ebner@proxmox.com>
In-Reply-To: <1648109081.y13k852cx6.astroid@nora.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.117 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
NICE_REPLY_A -0.001 Looks like a legit reply (A)
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
T_SCC_BODY_TEXT_LINE -0.01 -
Subject: Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if
user has VM.Config.Disk
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 28 Mar 2022 09:07:59 -0000
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
>>>
>>>
>>