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

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

hi,

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.

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

3. don't store all the tfa information in a single json file.

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.

4. html/js injection in the description field on gui (fixed on staff
repo already)

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?)

5.b also if recovery keys are available, limit amount of TOTP attempts
for that user


review still going on, but i figured it's good to have a feedback loop
:)












On Thu, Nov 19, 2020 at 03:56:02PM +0100, Wolfgang Bumiller wrote:
> This series is a first RFC for two factor authentication.
> 
> Notes:
> * Backend contains support for TOTP, Webauthn, Recovery keys and U2F.
> * The webauthn-rs crate introduces new dependencies we need to package:
>   - `half`
>   - `serde-cbor`
>   For our internal rust packaging I pushed the current debcargo-conf
>   changes to my `staff/w.bumiller/debcargo-conf` repo, branch `webauthn-rs`.
> 
>   some extra (already-packaged) deps will be pulled in along with them:
>     $ cargo update
>           Adding getrandom v0.1.14
>           Adding half v1.6.0
>           Adding nom v4.2.3
>           Adding ppv-lite86 v0.2.6
>           Adding rand v0.7.2
>           Adding rand_chacha v0.2.2
>           Adding rand_core v0.5.1
>           Adding rand_hc v0.2.0
>           Adding serde_bytes v0.11.5
>           Adding serde_cbor v0.11.1
>           Adding thiserror v1.0.15
>           Adding thiserror-impl v1.0.15
>           Adding webauthn-rs v0.2.5
> 
> * I wrote u2f before webauthn and left it in there unused because:
>   * we may want to move the code out to be integrated to PVE and PBS as
>     well for webauthn
>   * if we do: the webauthn-rs crate doesn't seem to provide a way
>     forward to using existin u2f credentials, so to not break those
>     we'll need the u2f backend.
> 
> * The GUI does not use U2F (see above). (I do have code for it if for
>   some reason we do want that).
> 
> * The GUI code is probably super weird. Some windows might look clunky,
>   but for me they always do currently since I'm working with
>   non-standard dpi monitor settings... so I can't really tell :-P
> 
> * I introduced some `async` code into the GUI because the webauthn api
>   uses Promises and extjs doesn't seem to have issues with that...
> 
> * The TFA configuration is currently a single json file.
> 
> * While writing this mail I realized I still want to change the way
>   webauthn credentials are being serialized, but that's not important
>   for a first draft to look through ;-)
> 
> Wolfgang Bumiller (6):
>   add tools::serde_filter submodule
>   config: add tfa configuration
>   api: tfa management and login
>   depend on libjs-qrcodejs
>   proxy: expose qrcodejs
>   gui: tfa support
> 
>  Cargo.toml                      |    1 +
>  debian/control.in               |    1 +
>  src/api2/access.rs              |  171 ++++--
>  src/api2/access/tfa.rs          |  567 +++++++++++++++++
>  src/bin/proxmox-backup-proxy.rs |    1 +
>  src/config.rs                   |    1 +
>  src/config/tfa.rs               | 1017 +++++++++++++++++++++++++++++++
>  src/server.rs                   |    2 +
>  src/server/rest.rs              |    5 +-
>  src/server/ticket.rs            |   77 +++
>  src/tools.rs                    |    1 +
>  src/tools/serde_filter.rs       |   97 +++
>  www/LoginView.js                |  323 +++++++++-
>  www/Makefile                    |    6 +
>  www/OnlineHelpInfo.js           |   36 --
>  www/Utils.js                    |   59 ++
>  www/config/TfaView.js           |  322 ++++++++++
>  www/index.hbs                   |    1 +
>  www/panel/AccessControl.js      |    6 +
>  www/window/AddTfaRecovery.js    |  211 +++++++
>  www/window/AddTotp.js           |  283 +++++++++
>  www/window/AddWebauthn.js       |  193 ++++++
>  www/window/TfaEdit.js           |   92 +++
>  23 files changed, 3357 insertions(+), 116 deletions(-)
>  create mode 100644 src/api2/access/tfa.rs
>  create mode 100644 src/config/tfa.rs
>  create mode 100644 src/server/ticket.rs
>  create mode 100644 src/tools/serde_filter.rs
>  create mode 100644 www/config/TfaView.js
>  create mode 100644 www/window/AddTfaRecovery.js
>  create mode 100644 www/window/AddTotp.js
>  create mode 100644 www/window/AddWebauthn.js
>  create mode 100644 www/window/TfaEdit.js
> 
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>