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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 0F8A59242E for ; Fri, 5 Apr 2024 15:13:29 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E3F9B13D4F for ; Fri, 5 Apr 2024 15:13:28 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 5 Apr 2024 15:13:28 +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 12707464DB for ; Fri, 5 Apr 2024 15:13:28 +0200 (CEST) Message-ID: <3106df5e-8b31-41f2-b66a-70c433faa4c1@proxmox.com> Date: Fri, 5 Apr 2024 15:13:27 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , Proxmox VE development discussion References: <20240130171057.438025-1-f.weber@proxmox.com> <20240130171057.438025-2-f.weber@proxmox.com> Content-Language: en-US From: Friedrich Weber In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.122 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [guesthelpers.pm] Subject: Re: [pve-devel] [PATCH guest-common v2 1/6] guest helpers: add helper to overrule active tasks of a specific type 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: Fri, 05 Apr 2024 13:13:29 -0000 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 >> --- >> >> 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.