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