From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.weber@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Wed, 19 Jul 2023 17:38:30 +0200 (CEST)
From: Friedrich Weber <f.weber@proxmox.com>
To: pve-devel@lists.proxmox.com
Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <w.bumiller@proxmox.com>
Signed-off-by: Friedrich Weber <f.weber@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