public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Oguz Bektas <o.bektas@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>
Cc: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
Date: Wed, 2 Dec 2020 14:35:11 +0100	[thread overview]
Message-ID: <20201202133511.GI7591@gaia.proxmox.com> (raw)
In-Reply-To: <fca82b83-e77e-947b-cf64-b2a79fe41c6d@proxmox.com>

On Wed, Dec 02, 2020 at 02:07:25PM +0100, Thomas Lamprecht wrote:
> On 02.12.20 13:35, Oguz Bektas wrote:
> > On Wed, Dec 02, 2020 at 01:27:47PM +0100, Thomas Lamprecht wrote:
> >>> 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
> 
> See that almost as anti-feature, it's actually better if such a thing happens
> that it's broken for all, as then one gets admin attention and can actually
> look for the underlying root cause - which at that point is probably memory or
> disk corruption/failure - or where does wolfgangs serializer breaks for all in one
> but not for split??
> 
> 
> > 
> > so the point wasn't to magically fix (potential) incorrect deserialization but to
> > reduce breakage in case something like that happens.
> 
> 
> like "what" happens? There's no such thing as one serialization is fine and the
> other not - if you start assuming that transient error model you cannot do anything
> at all anymore!

as i explained already, it's not about if one serialization is fine and
the other isnt; if we have one big mess of a json file holding all the
secrets of everyone's tfa config, and at any point there's some bug in
the serializer or any other component that interacts with this, then
this can lead to DOS of ALL accounts on the server (or compromise of ALL
secrets in that file). the model is different than the normal authentication
mechanism with pam/pbs realms, since the 2fa configuration has
(untrusted) user input that gets serialized and added into a root-owned
file during the setup. letting any user on any realm do this is IMO
bad practice.

furthermore we could easily add a check during auth to see if the
tfa.json parses to correct json, and if not pop up an error message like
"2FA configuration invalid, please contact administrator" etc. and even
automatically send an email to root@pam ...

> 
> I rather have it corrupt for all files as then the admin needs to fix it and we
> get notified, as some "magic" bug that only happens if it's a Tuesday and full moon.
> 
> So no I do *not* want to have user.cfg, token.cfg, shadow.json with all info in
> one file, and then start to split TFA for every user, because of an error model
> which just assumes whatever one wishes.
> 
> >>
> >>> 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
> 
> why is it more sensitive? I need both, so it's the same? If I get leaked shadow
> and tfa, I need to break both, only one has no use - that's the idea of TFA...

it's more sensitive to bruteforcing; because of limited
keyspace, as in it's easier to bruteforce a 6
digit numerical passcode than a regular passphrase in most
circumstances. if attacker cracks/steals a password and is presented
with a 2fa screen, it should be unlikely for them to bypass/crack that.
if i get unlimited tries to crack a 6-digit code you'll eventually get
it right.

that's why i think attempts should be limited by default and not reliant
on fail2ban, because there's no use case where anyone tries to enter a
totp code a thousand times for any legitimate reason... (however you
could forget/lose your password easily so it's more acceptable to let
someone keep trying in the regular auth case)

> 
> 
> >>
> >>>
> >>> 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.
> 
> and why do you place more trust onto the fixed recovery keys than another TFA
> option? 
the same reason i explained above, this would only kick in when the TOTP
is disabled because of too many auth failures. if a user has set up
recovery keys then they can be already used instead of TOTP (the option
is there regardless). so it's not placing more trust on the recovery
keys.

the flow could be something like this:
1. user sets up 2fa, TOTP and recovery keys
2. attacker login with stolen password
3. attacker attempts to crack 2fa totp code
4. fails after 3/5/X attempts, user gets notified and TOTP is disabled
5. at this point user can only log in with password + recovery code. (which they
could anyway, even if TOTP is present)

> Which services/programs/websites do that, can you name a few examples?

afaik some "secure" email providers like protonmail/tutanota etc. use
this kind of mechanism (account password + mailbox is encrypted with
password, and recovery keys in case all else is lost/locked).

i'm sure there are other examples as well


> 
> 




  reply	other threads:[~2020-12-02 13:35 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19 14:56 Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 1/6] add tools::serde_filter submodule Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 2/6] config: add tfa configuration Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 3/6] api: tfa management and login Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 4/6] depend on libjs-qrcodejs Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 5/6] proxy: expose qrcodejs Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 6/6] gui: tfa support Wolfgang Bumiller
2020-11-24  9:42   ` Wolfgang Bumiller
2020-11-24  9:51     ` Thomas Lamprecht
2020-12-02 10:56 ` [pbs-devel] [RFC backup 0/6] Two factor authentication Oguz Bektas
2020-12-02 12:27   ` Thomas Lamprecht
2020-12-02 12:34     ` Thomas Lamprecht
2020-12-02 12:48       ` Oguz Bektas
2020-12-02 12:59         ` Wolfgang Bumiller
2020-12-02 13:08           ` Oguz Bektas
2020-12-02 12:35     ` Oguz Bektas
2020-12-02 12:51       ` Wolfgang Bumiller
2020-12-02 13:15         ` Thomas Lamprecht
2020-12-02 13:07       ` Thomas Lamprecht
2020-12-02 13:35         ` Oguz Bektas [this message]
2020-12-02 14:05           ` Thomas Lamprecht
2020-12-02 14:21             ` Oguz Bektas
2020-12-02 14:29               ` Wolfgang Bumiller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201202133511.GI7591@gaia.proxmox.com \
    --to=o.bektas@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal