From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 6F5DDEACD for ; Wed, 19 Jul 2023 17:38:31 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4F670A424 for ; Wed, 19 Jul 2023 17:38:31 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 19 Jul 2023 17:38:30 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8463E417C2 for ; Wed, 19 Jul 2023 17:38:30 +0200 (CEST) From: Friedrich Weber To: pve-devel@lists.proxmox.com Cc: Wolfgang Bumiller Date: Wed, 19 Jul 2023 17:38:04 +0200 Message-Id: <20230719153804.1537780-1-f.weber@proxmox.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.269 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [accesscontrol.pm] Subject: [pve-devel] [PATCH access-control] auth: tfa: fail if realm requires TFA but no challenge is generated X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Jul 2023 15:38:31 -0000 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 Signed-off-by: Friedrich Weber --- 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