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 DA908D542 for ; Fri, 14 Jul 2023 13:50:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BC71C2F052 for ; Fri, 14 Jul 2023 13:50:33 +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 ; Fri, 14 Jul 2023 13:50:32 +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 076F241EE8 for ; Fri, 14 Jul 2023 13:50:32 +0200 (CEST) From: Friedrich Weber To: pve-devel@lists.proxmox.com Cc: Wolfgang Bumiller Date: Fri, 14 Jul 2023 13:49:50 +0200 Message-Id: <20230714114950.96167-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.286 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, proxmox.com] Subject: [pve-devel] [PATCH access-control] auth: tfa: read tfa.cfg also if the user.cfg entry has no "x" marker 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: Fri, 14 Jul 2023 11:50:33 -0000 Previously, `user_get_tfa` read the `keys` user attribute from user.cfg to determine whether a user has second factors configured. `keys` could contain TOTP secrets or Yubico key IDs (for realms that require TFA), or the marker "x" to signify that second factors are defined in tfa.cfg, in which case `user_get_tfa` would additionally read tfa.cfg. However, syncing an LDAP realm with `remove-vanished=properties` erases the `keys` attribute, and thus the "x" marker (unless custom `sync_attributes` with a mapping for `keys` are defined). This would allow TFA-enabled users to log in without a second factor after a realm sync. This issue was first reported in the forum [1]. To fix this issue, `user_get_tfa` now reads tfa.cfg unconditionally, and thus independently of the value of `keys`. In other words, the "x" marker is now irrelevant for authentication. The reasoning for this change is that most current setups define second factors in tfa.cfg anyway. Special care is needed to avoid breaking realms that require TFA: In that case, `user_get_tfa` must fail authentication if neither user.cfg nor tfa.cfg define any second factors. This patch changes the behavior of a hypothetical (and not officially supported) LDAP realm setup in which `sync_attributes: keys=attr` and `remove-vanished=properties` is used to maintain `keys` in the LDAP directory. In such a setup, an admin could enable/disable TFA for a user who has an enabled second factor in tfa.cfg by editing their LDAP entry and switching between "x" and "". With this patch, TFA is always enabled for that user. This patch makes the "x" marker irrelevant for authentication, but PVE still *writes* it if the user has second factors configured in tfa.cfg. This behavior is kept for now to avoid issues in cluster upgrade scenarios, where some nodes that still rely on the "x" marker could allow logins without a second factor. [1] https://forum.proxmox.com/threads/130440/ Suggested-by: Wolfgang Bumiller Signed-off-by: Friedrich Weber --- src/PVE/AccessControl.pm | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm index 24c6618..da018e5 100644 --- a/src/PVE/AccessControl.pm +++ b/src/PVE/AccessControl.pm @@ -1998,15 +1998,17 @@ sub user_get_tfa : prototype($$$) { $realm_tfa = PVE::Auth::Plugin::parse_tfa_config($realm_tfa) if $realm_tfa; - if (!$keys) { - return if !$realm_tfa; - die "missing required 2nd keys\n"; - } - my $tfa_cfg = cfs_read_file('priv/tfa.cfg'); if (defined($keys) && $keys !~ /^x(?:!.*)$/) { add_old_keys_to_realm_tfa($username, $tfa_cfg, $realm_tfa, $keys); } + + if ($realm_tfa) { + 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