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

[Friedrich Weber <f.weber@proxmox.com>]

Thanks for the review!

On 04/04/2024 17:20, Thomas Lamprecht wrote:
> Am 30/01/2024 um 18:10 schrieb Friedrich Weber:
> 
> Maybe start of with "Add a helper to abort all tasks from a specific
> (type, user, vmid) tuple. It will be used ...

Will do.

>> This helper is used to abort any active qmshutdown/vzshutdown tasks
>> before attempting to stop a VM/CT (if requested).
>>
>> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
>> ---
>>
>> Notes:
>>     no changes v1 -> v2
>>
>>  src/PVE/GuestHelpers.pm | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
>> index 961a7b8..bd94ed2 100644
>> --- a/src/PVE/GuestHelpers.pm
>> +++ b/src/PVE/GuestHelpers.pm
>> @@ -416,4 +416,22 @@ sub check_vnet_access {
>>  	if !($tag || $trunks);
>>  }
>>  
>> +sub overrule_tasks {
> 
> hmm, overruling is the thing you want to do now, but one might
> be use this for other reasons, so maybe naming it about what it
> does would be a bit better compared to what is used for (now).
> 
> From top of my head that could be "abort_guest_tasks", but maybe
> you got a better idea.

Yeah, that's true, I'll rename it to `abort_guest_tasks` or some variation.

>> +    my ($type, $user, $vmid) = @_;
>> +
>> +    my $active_tasks = PVE::INotify::read_file('active');
>> +    my $res = [];
>> +    for my $task (@$active_tasks) {
>> +       if (!$task->{saved}
>> +           && $task->{type} eq $type
>> +           && $task->{user} eq $user
> 
> This also means that e.g. root@pam cannot overrule a task started by
> apprentice@pam, which might be something admins what to do (they like
> overruling users after all ;-P). Or some automation triggering the
> shutdown (using a token with a separate user) might be also a good
> examples of things I'd like to be able to overrule. 

Good point. I see the usecase for being able to override other user's
tasks. My original motivation for making the user check that tight was
to avoid privilege escalation.

> 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.

> 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).

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?

>> +           && $task->{id} eq $vmid
>> +       ) {
> 
> meh, the pre-existing $killit param is way too subtle for my taste...
> Some %param that takes a `kill => "reason"` property for this would be
> much more telling.
> 
> But changing that is a bit out of scope, a comment would be great for
> now though.

Makes sense, will add a comment.

> 
>> +           PVE::RPCEnvironment->check_worker($task->{upid}, 1);
>> +           push @$res, $task->{upid};
> 
> renaming $res to $killed_tasks would also help in reading this code out
> of further context.

Makes sense, will rename $res.