]> git.proxmox.com Git - pve-access-control.git/commit
auth: tfa: fail if realm requires TFA but no challenge is generated
authorFriedrich Weber <f.weber@proxmox.com>
Wed, 19 Jul 2023 15:38:04 +0000 (17:38 +0200)
committerWolfgang Bumiller <w.bumiller@proxmox.com>
Thu, 20 Jul 2023 08:53:41 +0000 (10:53 +0200)
commit032e7d6d441f89a48cadfd7f47e957c8a561c022
treea585e0d471249200bbeb54db32388866dd4c57b8
parent72950c1d531595ad1c4a895e415cbcba408e6d03
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>
src/PVE/AccessControl.pm