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

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

[Shan Shaji]

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;
             } else {
-                $tfa_cfg->remove_user($userid);
+                my $needs_user_saving = $tfa_cfg->remove_user($userid);
+                $update_user_config = $needs_user_saving;
             }
             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);
+            });
+        }
         return;
     },
 });
-- 
2.39.5



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

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

[Fabian Grünbichler]

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

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

[Shan Shaji]

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

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

[Fabian Grünbichler]

On July 29, 2025 12:26 pm, Shan Shaji wrote:
> 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:
>> > +
>> > +        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. 

yes, unless you find another code path that does the inverse (lock user
first, then lock TFA while the lock is held) - in that case we need to
settle on one of the two variants ;)

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


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

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

[Shan Shaji]

superseeded by v2: https://lore.proxmox.com/pve-devel/20250729110229.118959-1-s.shaji@proxmox.com/T/#u

On Tue Jul 29, 2025 at 12:40 PM CEST, Fabian Grünbichler wrote:
> > So the order will the lock TFA -> lock user cfg -> update user cfg ->
> > update tfa cfg. 
>
> yes, unless you find another code path that does the inverse (lock user
> first, then lock TFA while the lock is held) - in that case we need to
> settle on one of the two variants ;)

I think it's not possible to do the other way around as when i checked
the implementaion of `lock_tfa_config` in PVE::AccessControl if the
user config is locked it's not possible to acquire a tfa lock. Also
there is a comment that mentions about locking of the files are only
allowed together in one order. 



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