[pve-devel] [PATCH access-control] auth: tfa: fail if realm requires TFA but no challenge is generated

[Friedrich Weber]

Before 0f3d14d6 ("auth: tfa: read tfa.cfg also if the user.cfg entry
has no "x" marker"), `user_get_tfa` failed if the realm required TFA,
but the user's `keys` attribute was empty. Since 0f3d14d6,
`user_get_tfa` fails if the realm requires TFA, but neither user.cfg
nor tfa.cfg define any second factors for that user.

However, both before and after 0f3d14d6, a realm that requires TOTP
allows a user to login without a second factor if they have at least
one configured factor in tfa.cfg and all factors are disabled -- for
example if they have only a disabled TOTP factor. This behavior is
unwanted, as users can then circumvent the realm-mandated TFA
requirement by disabling their own TOTP factor.

This happens because a user with a disabled TOTP factor in tfa.cfg
passes the check in `user_get_tfa`. Hence, `authenticate_2nd_new_do`
proceeds to call `authentication_challenge`, which does not generate a
challenge (and returns undef) because the user has no enabled factors.
Consequently, `authenticate_2nd_new_do` returns undef and allows login
without a second factor.

Note that this does not happen for realms that require Yubico TFA,
because for these realms, `authenticate_2nd_new_do` does not call
`authentication_challenge` and instead generates a challenge in any
case, regardless of whether the user has enabled Yubico factors or
not.

This patch fixes the issue by moving the check out of `user_get_tfa`,
and instead letting `authenticate_2nd_new_do` fail if the realm
requires TFA but `authentication_challenge` generates no challenge
(returns undef). This also saves a call to `api_list_user_tfa` that
was introduced in 0f3d14d6.

This patch still allows users to login with a recovery key to a realm
that requires TFA , which is the intended behavior.

Suggested-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
Signed-off-by: Friedrich Weber <f.weber at proxmox.com>
---

Notes:
    The challenge generation step could still be improved by making sure
    that the generated challenge matches the realm's TFA type (TOTP or
    Yubico), while also taking recovery keys into account. However, the
    verification step checks this anyway, so the only advantage would be a
    slightly more streamlined user experience (e.g. for a realm that
    requires TOTP, users wouldn't be asked for a Yubico factor that
    wouldn't be accepted anyway). The disadvantage would be some code
    duplication between the challenge generation and verification steps,
    so I decided to keep things simple for now and only do the simple
    check described above.

 src/PVE/AccessControl.pm | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index c38a1e5..cc0f00b 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -808,6 +808,10 @@ sub authenticate_2nd_new_do : prototype($$$$) {
 	$tfa_challenge = undef;
     } else {
 	$tfa_challenge = $tfa_cfg->authentication_challenge($username);
+
+	die "missing required 2nd keys\n"
+	    if $realm_tfa && !defined($tfa_challenge);
+
 	if (defined($tfa_response)) {
 	    if (defined($tfa_challenge)) {
 		$tfa_done = 1;
@@ -2006,13 +2010,6 @@ sub user_get_tfa : prototype($$$) {
 	add_old_keys_to_realm_tfa($username, $tfa_cfg, $realm_tfa, $keys);
     }
 
-    if ($realm_tfa) {
-	# FIXME: pve-rs should provide a cheaper check for this
-	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