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

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

[Friedrich Weber]

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

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

[Wolfgang Bumiller]

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
>