Re: [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured

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

I think this is https://bugzilla.proxmox.com/show_bug.cgi?id=3739 ;)

@wolfgang could you take a look at this?

On September 14, 2022 3:42 pm, Dominik Csapak wrote:
> With change to our new tfa mechanism, we now lock the tfa config
> when verifying the second factor and when creating the challenge for it
> that makes sense, since at least one tfa type can change the config
> (recovery keys must be deleted from there).
> 
> The downside is that we cannot authenticate users anymore without quorum
> (since locking requires write access to pmxcfs), even for users without
> tfa configured (and also for clusters without any tfa configured at all).
> 
> With this patch, we loosen that restriction a bit, by checking if the
> user has tfa configured outside the lock. We still query it again
> inside the lock to get the current config inside the lock again.
> (slightly out of the diff context)
> 
> There is a minimal (IMHO unimportant) race here:
> if a user is logging in and for this user tfa is configured
> simultaneously, it may happen that during the first check tfa is still
> not present.
> 
> This is not a real problem though, since a logged in user will not be
> logged out when a tfa is configured, so it's the same as when the user
> would have logged in before the tfa is being configured.
> 
> There were quite a bit confused users that ran into that, and preventing
> them from logging in at all because of a not quorate server (when
> there is not a good technical reason for it) seems bad
> 
> It's still not possible to login when tfa is enabled though, but at
> least for simple setups it should be a bit better
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/AccessControl.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index c32dcc3..52ad84b 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -790,6 +790,12 @@ sub authenticate_2nd_old : prototype($$$) {
>  sub authenticate_2nd_new : prototype($$$$) {
>      my ($username, $realm, $otp, $tfa_challenge) = @_;
>  
> +    my ($tfa_cfg) = user_get_tfa($username, $realm, 1);
> +
> +    if (!defined($tfa_cfg)) {
> +	return undef;
> +    }
> +
>      my $result = lock_tfa_config(sub {
>  	my ($tfa_cfg, $realm_tfa) = user_get_tfa($username, $realm, 1);
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>