Re: [pve-devel] [PATCH pve-access-control] fix #6528: tfa: update user config on removal of TFA

["Shan Shaji" <s.shaji@proxmox.com>]

Thank you so much for the review and i will update it accordingly. 
Had some doubts which i added as inline comments. 

On Tue Jul 29, 2025 at 11:45 AM CEST, Fabian Grünbichler wrote:
> 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 <s.shaji@proxmox.com>
> > ---
> >  
> >  Tested Cases:
> >  - Case 1:
> >     - Added one TFA for a user. 
> >     - Delete with CLI `pveum user tfa delete <user>`
> >     - Verified if the UI and CLI outputs are updated. 
> >  - Case 2:
> >     - Add two TFA for a user. 
> >     - Delete with CLI `pveum user tfa delete <user>`
> >     - Verified if the UI and CLI outputs are updated. 
> >  - Case 3: 
> >     - Add two TFA for a user. 
> >     - Delete with CLI `pveum user tfa delete <user> --id <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).
So it's okay if i set `$update_user_config = 1;`
> >              }
> >              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:
So the order will the lock TFA -> lock user cfg -> update user cfg ->
update tfa cfg. 
> 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



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel