Re: [pve-devel] [PATCH guest-common v2 1/6] guest helpers: add helper to overrule active tasks of a specific type

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 05/04/2024 um 15:13 schrieb Friedrich Weber:
> On 04/04/2024 17:20, Thomas Lamprecht wrote:
>> Or does it even make sense to check this at all?
>> As long as the user has the rights to execute a stop they probably
>> should also be able to force it at any time, even for other users?
> 
> The usecase sounds reasonable, but would technically amount to a small
> privilege escalation: Currently, if user A and user B both have
> PVEVMUser and user A starts a `vzshutdown` task, user B must have
> `Sys.Modify` to kill the task.
> 

Hmm, yeah, this is definitively something one could argue about as
we currently do not have much overruling functionality of tasks triggered
by other users that can manage a specific resource, but not the whole
system. So this is essential new territory w.r.t. semantics, and we can
also adapt new ones, albeit they should certainly not be completely
unexpected.

>> Maybe make this optional and only enforce it for users that do not
>> have some more powerful priv?
> 
> We could introduce a similar check as for the DELETE
> /nodes/{node}/tasks/{upid} endpoint: Users can always overrule their own
> guest tasks, and with `Sys.Modify` they can overrule any guest task (for
> guests for which they have the necessary permission).

Yeah, I think that would be a safe bet for now.

> Still, right now I think the primary motivation for this overruling
> feature is to save GUI users some frustration and/or clicks. In this
> scenario, a user will overrule only their own tasks, which is possible
> with the current check. What do you think about keeping the check as it
> is for now, and making it more permissive once the need arises?

I think that allowing users that hold the respective Sys.Modify and
VM.PowerMgmt to overrule any tasks from the start wouldn't be to much
"speculative future-proofing" but rather something expected while still
safe.

FWIW, you could also drop the $authuser then and just get it from
the RPCEnv singleton available in all API call-paths and then do
the permission check in the helper directly.
This would IMO be also a bit better w.r.t. conveying why we do it this
way.