Re: [pve-devel] [PATCH access-control] auth: tfa: read tfa.cfg also if the user.cfg entry has no "x" marker

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

applied, thanks

We'll want a cheaper check in pve-rs for whether the user has anything
defined next to querying the list soon, and the caller of `user_get_tfa`
can replace the `defined($tfa_cfg)` check to use that as well - since
`$tfa_cfg` now cannot be `undef` there anymore.
Alternatively we could see about moving the realm-tfa check to after
calling `authentication_challenge` which may allow us to avoid having to
explicitly query whether 2nd factors exist altogether?

On Fri, Jul 14, 2023 at 01:49:50PM +0200, Friedrich Weber wrote:
> Previously, `user_get_tfa` read the `keys` user attribute from
> user.cfg to determine whether a user has second factors configured.
> `keys` could contain TOTP secrets or Yubico key IDs (for realms that
> require TFA), or the marker "x" to signify that second factors are
> defined in tfa.cfg, in which case `user_get_tfa` would additionally
> read tfa.cfg.
> 
> However, syncing an LDAP realm with `remove-vanished=properties`
> erases the `keys` attribute, and thus the "x" marker (unless custom
> `sync_attributes` with a mapping for `keys` are defined). This would
> allow TFA-enabled users to log in without a second factor after a
> realm sync. This issue was first reported in the forum [1].
> 
> To fix this issue, `user_get_tfa` now reads tfa.cfg unconditionally,
> and thus independently of the value of `keys`. In other words, the "x"
> marker is now irrelevant for authentication. The reasoning for this
> change is that most current setups define second factors in tfa.cfg
> anyway.
> 
> Special care is needed to avoid breaking realms that require TFA: In
> that case, `user_get_tfa` must fail authentication if neither user.cfg
> nor tfa.cfg define any second factors.
> 
> This patch changes the behavior of a hypothetical (and not officially
> supported) LDAP realm setup in which `sync_attributes: keys=attr` and
> `remove-vanished=properties` is used to maintain `keys` in the LDAP
> directory. In such a setup, an admin could enable/disable TFA for a
> user who has an enabled second factor in tfa.cfg by editing their LDAP
> entry and switching between "x" and "". With this patch, TFA is always
> enabled for that user.
> 
> This patch makes the "x" marker irrelevant for authentication, but PVE
> still *writes* it if the user has second factors configured in
> tfa.cfg. This behavior is kept for now to avoid issues in cluster
> upgrade scenarios, where some nodes that still rely on the "x" marker
> could allow logins without a second factor.
> 
> [1] https://forum.proxmox.com/threads/130440/
> 
> Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
>  src/PVE/AccessControl.pm | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index 24c6618..da018e5 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -1998,15 +1998,17 @@ sub user_get_tfa : prototype($$$) {
>      $realm_tfa = PVE::Auth::Plugin::parse_tfa_config($realm_tfa)
>  	if $realm_tfa;
>  
> -    if (!$keys) {
> -	return if !$realm_tfa;
> -	die "missing required 2nd keys\n";
> -    }
> -
>      my $tfa_cfg = cfs_read_file('priv/tfa.cfg');
>      if (defined($keys) && $keys !~ /^x(?:!.*)$/) {
>  	add_old_keys_to_realm_tfa($username, $tfa_cfg, $realm_tfa, $keys);
>      }
> +
> +    if ($realm_tfa) {
> +	my $entries = $tfa_cfg->api_list_user_tfa($username);
> +	die "missing required 2nd keys\n"
> +	    if scalar(@$entries) == 0;
> +    }
> +
>      return ($tfa_cfg, $realm_tfa);
>  }
>  
> -- 
> 2.39.2
>