proxmox-mirrors/pve-access-control
1
0
Fork 0
mirror of https://git.proxmox.com/git/pve-access-control synced 2025-08-10 17:22:06 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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>
This commit is contained in:
Friedrich Weber 2023-07-19 17:38:04 +02:00 committed by Wolfgang Bumiller
parent 72950c1d53
commit 032e7d6d44
1 changed files with 4 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
src/PVE/AccessControl.pm
View File

@ -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);
}