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
