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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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

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