From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Fabian Ebner <f.ebner@proxmox.com>, pve-devel@lists.proxmox.com
Subject: Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk
Date: Mon, 28 Mar 2022 13:36:48 +0200 [thread overview]
Message-ID: <1648465767.vrjikn849t.astroid@nora.none> (raw)
In-Reply-To: <a35c7af7-2879-6882-e3d2-0dbeac68175f@proxmox.com>
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.
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).
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?
next prev parent reply other threads:[~2022-03-28 11:37 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-21 13:06 [pve-devel] [PATCH-SERIES storage/manager/container/qemu-server] improve check_volume_access Fabian Ebner
2022-03-21 13:06 ` [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk Fabian Ebner
2022-03-22 8:31 ` Fabian Ebner
2022-03-22 9:31 ` Fabian Ebner
2022-03-24 8:18 ` Fabian Grünbichler
2022-03-28 9:07 ` Fabian Ebner
2022-03-28 11:36 ` Fabian Grünbichler [this message]
2022-03-29 7:48 ` Fabian Ebner
2022-03-29 12:33 ` Fabian Grünbichler
2022-03-21 13:06 ` [pve-devel] [PATCH storage 2/4] check volume accesss: add content type parameter Fabian Ebner
2022-03-21 13:06 ` [pve-devel] [PATCH storage 3/4] pvesm: extract config: add content type check Fabian Ebner
2022-03-21 13:06 ` [pve-devel] [PATCH storage 4/4] api: file restore: use check_volume_access to restrict content type Fabian Ebner
2022-03-21 13:06 ` [pve-devel] [PATCH manager 1/2] pveam: remove: add content type check Fabian Ebner
2022-03-21 13:06 ` [pve-devel] [PATCH manager 2/2] api: vzdump: extract config: " Fabian Ebner
2022-03-21 13:06 ` [pve-devel] [PATCH container 1/1] api: create/modify: add content type checks Fabian Ebner
2022-03-21 13:06 ` [pve-devel] [PATCH qemu-server " Fabian Ebner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1648465767.vrjikn849t.astroid@nora.none \
--to=f.gruenbichler@proxmox.com \
--cc=f.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox