Re: [pbs-devel] [RFC backup 0/6] Two factor authentication

[Oguz Bektas <o.bektas@proxmox.com>]

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/<username>.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.