From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 4ACA61FF16B for ; Tue, 29 Jul 2025 11:44:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 368ADCA8F; Tue, 29 Jul 2025 11:45:58 +0200 (CEST) Date: Tue, 29 Jul 2025 11:45:52 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20250729083010.39570-1-s.shaji@proxmox.com> In-Reply-To: <20250729083010.39570-1-s.shaji@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1753781854.l4zxsjc0e1.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753782345794 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.046 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH pve-access-control] fix #6528: tfa: update user config on removal of TFA 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On July 29, 2025 10:30 am, Shan Shaji wrote: > When removing TFA from a user via the command line, the change was not > reflected in the GUI or in the output of `pveum user list`. Both > continued to show that TFA was enabled for the user. Fixed the issue > by updating the user configuration file. > > Signed-off-by: Shan Shaji > --- > > Tested Cases: > - Case 1: > - Added one TFA for a user. > - Delete with CLI `pveum user tfa delete ` > - Verified if the UI and CLI outputs are updated. > - Case 2: > - Add two TFA for a user. > - Delete with CLI `pveum user tfa delete ` > - Verified if the UI and CLI outputs are updated. > - Case 3: > - Add two TFA for a user. > - Delete with CLI `pveum user tfa delete --id ` > - Verified if the UI and CLI outputs are updated. > > src/PVE/CLI/pveum.pm | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/src/PVE/CLI/pveum.pm b/src/PVE/CLI/pveum.pm > index 0645a6d..87e68df 100755 > --- a/src/PVE/CLI/pveum.pm > +++ b/src/PVE/CLI/pveum.pm > @@ -141,16 +141,28 @@ __PACKAGE__->register_method({ > > my $userid = extract_param($param, "userid"); > my $tfa_id = extract_param($param, "id"); > + my $update_user_config; > > PVE::AccessControl::lock_tfa_config(sub { > my $tfa_cfg = cfs_read_file('priv/tfa.cfg'); > if (defined($tfa_id)) { > - $tfa_cfg->api_delete_tfa($userid, $tfa_id); > + my $has_entries_left = $tfa_cfg->api_delete_tfa($userid, $tfa_id); > + $update_user_config = !$has_entries_left; this one is nice split like that, but > } else { > - $tfa_cfg->remove_user($userid); > + my $needs_user_saving = $tfa_cfg->remove_user($userid); > + $update_user_config = $needs_user_saving; these two lines could be combined IMHO, or we could even unconditionally set $update_user_config here - if we (are asked to) remove all TFA entries for a particular user, we want to clear the 'x' entry even if no TFA entries existed in the first place (e.g., because the configs ran out of sync). > } > cfs_write_file('priv/tfa.cfg', $tfa_cfg); > }); > + > + if ($update_user_config) { > + PVE::AccessControl::lock_user_config(sub { > + my $user_cfg = cfs_read_file('user.cfg'); > + my $user = $user_cfg->{users}->{$userid}; > + $user->{keys} = undef; > + cfs_write_file('user.cfg', $user_cfg); > + }); > + } the locking here is incomplete - in the meantime, another invocation could have added a TFA entry again, and potentially the 'x' marker is dropped afterwards making user.cfg wrong/out of sync.. PVE::API2::TFA::delete_tfa() has the same issue, but in PVE::API2::TFA::add_tfa_entry we have a nested call: lock_tfa_config() { set_user_tfa_enabled() { lock_user_config() { add 'x' entry to user.cfg } } add tfa entry to tfa.cfg } if that lock order is used everywhere else as well, the two deletion paths (API and CLI) should also be done like that, to avoid races. there might be more code paths missing proper nested locking, I haven't done a complete analysis. > return; > }, > }); > -- > 2.39.5 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel