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 781E9D546 for ; Fri, 14 Jul 2023 14:22:25 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5175F2F2B3 for ; Fri, 14 Jul 2023 14:21:55 +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 14:21:54 +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 4A4EC41D90 for ; Fri, 14 Jul 2023 14:21:54 +0200 (CEST) Date: Fri, 14 Jul 2023 14:21:53 +0200 From: Wolfgang Bumiller To: Friedrich Weber Cc: pve-devel@lists.proxmox.com Message-ID: References: <20230714114950.96167-1-f.weber@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230714114950.96167-1-f.weber@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.121 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: Re: [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 12:22:25 -0000 applied, thanks We'll want a cheaper check in pve-rs for whether the user has anything defined next to querying the list soon, and the caller of `user_get_tfa` can replace the `defined($tfa_cfg)` check to use that as well - since `$tfa_cfg` now cannot be `undef` there anymore. Alternatively we could see about moving the realm-tfa check to after calling `authentication_challenge` which may allow us to avoid having to explicitly query whether 2nd factors exist altogether? On Fri, Jul 14, 2023 at 01:49:50PM +0200, Friedrich Weber wrote: > 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 >