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 268986E159 for ; Tue, 29 Mar 2022 09:48:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1B8962BFA5 for ; Tue, 29 Mar 2022 09:48:48 +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 54A292BF9B for ; Tue, 29 Mar 2022 09:48:47 +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 15A6E43506 for ; Tue, 29 Mar 2022 09:48:41 +0200 (CEST) Message-ID: Date: Tue, 29 Mar 2022 09:48:34 +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?= , Proxmox VE development discussion 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> <1648465767.vrjikn849t.astroid@nora.none> From: Fabian Ebner In-Reply-To: <1648465767.vrjikn849t.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 - 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] 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: Tue, 29 Mar 2022 07:48:48 -0000 Am 28.03.22 um 13:36 schrieb Fabian Grünbichler: > On March 28, 2022 11:07 am, Fabian Ebner wrote: >> 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: >>>>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm >>>>>> index 6112991..efa304a 100755 >>>>>> --- a/PVE/Storage.pm >>>>>> +++ b/PVE/Storage.pm > > [..] > >>>>>> + } 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? > > for listing a storage's contents we also already check Datastore.Audit | > Datastore.AllocateSpace (as part of the API schema), but for info and > attribute updating we only check `check_volume_access` which would > mean that with your change these suddenly allow brute force listing > (with /vm permission, but no permissions on the storage) which > doesn't seem ideal. for those two API endpoints with the current version > of check_volume_access one of Datastore.Allocatespace, Allocate or Audit > (depending on volume type) is needed implicitly via check_volume_access.. > > basically I see two options: > - extend your new branch in check_volume_access to require Datastore.X > (Audit or Allocatespace?) in addition to VM.Config.Disk => import-from > would require it, info/update_attributes in the storage API would > require it if they take that branch > - change info/update_attributes to require any of Datastore.Allocate, > Datastore.AllocateSpace, Datastore.Audit => import-from would not > require them. import-from in its current form doesn't use check_volume_access() for the source volume if it belongs to a VM, but requires VM.Clone. Just like the clone_vm API call. So it's not import-from itself that would require different things depending on the variant we choose, but e.g. listing images for import-from in the UI. > > I think I prefer the first variant, since it's internally consistent in > check_volume_access (all the branches check some storage priv, unless > the special 'we checked already and if the volume is owned by this VMID > it's okay' path is taken via a passed in owner $vmid) and is less > 'pitfall-y' (w.r.t. opening brute-force code paths like the info one). I also prefer the first variant, but it can lead to a case where I cannot - via UI, to a target storage I have access to - import-from a single disk of a VM (just because I cannot list the image), but can clone the whole VM to the same target storage. > > we could of course think about extending it further in the direction of > 'Datastore.Audit | Datastore.AllocateSpace' vs 'Datastore.AllocateSpace' > via a flag to differentiate between reading a volume and > writing/allocating one (and then in import-from, the source would only > need Audit, while the target would need AllocateSpace). but that would > require some more thought I think.. > > side-note: the check in clone_vm is a bit strange, it overrides the > source storage with the target storage, but not for the vmstatestorage, > so it basically rechecks the permissions for that single config key but > not for any others.. maybe we should even drop the check for > vmstatestorage? if it's in the config, somebody with the appropriate > permission put it there after all, and if a user can clone that VM all > the config comes with it? I'm not opposed to dropping that either. It's not like it changes the fact that the user can create state images on that storage (assuming the VM.Snapshot permission is not only granted for the clone's ID or something, but those are edge-cases worth ignoring IMHO).