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 7C58160262 for ; Wed, 2 Dec 2020 13:37:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6D1D41B62A for ; Wed, 2 Dec 2020 13:36:32 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (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 id D4C7A1B620 for ; Wed, 2 Dec 2020 13:36:31 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9FFF344834 for ; Wed, 2 Dec 2020 13:36:31 +0100 (CET) Date: Wed, 2 Dec 2020 13:35:56 +0100 From: Oguz Bektas To: Thomas Lamprecht Cc: Proxmox Backup Server development discussion Message-ID: <20201202123556.GE7591@gaia.proxmox.com> References: <20201119145608.16866-1-w.bumiller@proxmox.com> <20201202105650.GA7591@gaia.proxmox.com> <4c361a22-5caa-db5e-66b9-046638048fd5@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4c361a22-5caa-db5e-66b9-046638048fd5@proxmox.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-SPAM-LEVEL: Spam detection results: 0 AWL 1.473 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [RFC backup 0/6] Two factor authentication X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Dec 2020 12:37:02 -0000 On Wed, Dec 02, 2020 at 01:27:47PM +0100, Thomas Lamprecht wrote: > Hi, > > thanks for taking a look, some comments regarding your feedback. > > On 02.12.20 11:56, Oguz Bektas wrote: > > we talked with wolfgang off-list about some issues, so here are > > some recommendations for the next version: > > > > 1. increase the length of recovery codes for bruteforce mitigation > > > > most websites use 12-16 characters for the length of recovery keys. > > makes sense > > > > > 2. do not store recovery codes in cleartext (hash them instead, we thought > > hmac-sha256 is fine). the reason being that recovery codes can bypass > > other tfa methods so they shouldn't be visible > > make sense, would expect them to be hashed > > > > > 3. don't store all the tfa information in a single json file. > > > > makes no sense to me, any reason you mention below can happen to arbitrary > files, so just adds complexity while not gaining anything. > > > current version uses a single /etc/proxmox-backup/tfa.json file > > which holds all the tfa info for all the users. this is a single point > > of failure because: > > - file can be corrupted, causing tfa to break for everyone (no more logins) > > - file could get deleted, disabling/bypassing 2fa for everyone > > - file could get leaked in a backup etc., giving everyone's tfa secrets > > and/or recovery keys to attackers (bypass everything) > > > > better is to at least create a file for each user: > > /etc/proxmox-backup/tfa/.json or similar > > > > this way the damage is contained if for example the config breaks > > because of incorrect deserialization etc. > > Why would deserialisation be incorrect for one single file but magically > works if multiple files? Makes no sense. of course this can happen on arbitrary files... i don't see why it would add any complexity to use multiple files though (actually makes it simpler imo). the reasoning behind this was to avoid a single point of failure like i explained: multiple files for users -> only that user is affected by broken config, other users can log in single file for all users -> all users affected if config breaks and nobody can log in so the point wasn't to magically fix (potential) incorrect deserialization but to reduce breakage in case something like that happens. > > > > > 4. html/js injection in the description field on gui (fixed on staff > > repo already) > > > > Yeah, as always, Ext.String.htmlEncode is your friend ;) :) > > > 5. notify user if more than X failed tfa attempts (password is already > > compromised at this point, so it's important to notify) and block IP > > for certain amount of time (fail2ban?) > > we do not setup fail2ban but any admin can already if wished. Notification > can only work if the user has setup a mail in the first place - but yes, sou yes, but imo 2fa is more sensitive to bruteforcing than regular passwords so it would make sense to limit it by default > > > > > 5.b also if recovery keys are available, limit amount of TOTP attempts > > for that user > > what? > if a user sets up TOTP + recovery keys, then it would make sense to lock account in case of a lot of auth attempts with TOTP, until recovery key is entered (afaik this is a common mechanism). but maybe just notifying the user is enough as well.