From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 04FFB696F4 for ; Thu, 24 Mar 2022 09:18:59 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E685E2DB42 for ; Thu, 24 Mar 2022 09:18:28 +0100 (CET) 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 1470C2DB38 for ; Thu, 24 Mar 2022 09:18:28 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E323A46F6F for ; Thu, 24 Mar 2022 09:18:27 +0100 (CET) Date: Thu, 24 Mar 2022 09:18:15 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Fabian Ebner , 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> In-Reply-To: <8f1859ba-c782-7dee-09e6-7fac1561ce74@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1648109081.y13k852cx6.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.180 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 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [storage.pm, proxmox.com] 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Mar 2022 08:18:59 -0000 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 >>> --- >>> 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']); >=20 > @Fabian G. should access to backups also be allowed if the user /just/ > has Datastore.Allocate? >=20 > 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=20 as: Datastore.Allocate: create/modify/remove a datastore and delete volumes but in practice, any user that has Datastore.Allocate likely also has=20 Datastore.AllocateSpace anyway which is probably why nobody complained=20 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= =20 VM.Backup and Datastore.AllocateSpace you are allowed to access "your=20 own" backups, even if you can't access the whole range of files on the=20 storage', the code was just not very thought through (and in practice,=20 high-privileged users on storages are usually also high-privileged users=20 on guests, so nobody noticed/cared). I think adding an early return with the check from the else branch with=20 $noerr set and replacing the else branch with a `die` would be fine (so=20 Datastore admins are always allowed to access all volumes on their=20 storages). >>> + } elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) { >>> + $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']); >>=20 >> Of course this needs to be or-ed with the Datastore.Allocate privilege. >> Will fix it in v2. and and-ed with Datastore.AllocateSpace? >>=20 >>> } else { >>> # allow if we are Datastore administrator >>> $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']); >>=20 >>=20 >> _______________________________________________ >> pve-devel mailing list >> pve-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>=20 >>=20 >=20