auth: tfa: fail if realm requires TFA but no challenge is generated

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@proxmox.com>
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index c38a1e5a02bf3fce26b3967fd12f6aedabbf1dea..cc0f00b16c581e2100b71ec19dcde3b1cff82e69 100644 (file)
--- 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);
 }