Enable OTP 2FA #3798

j0k3r · 2018-12-02T16:39:11Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Documentation	needed
Translation	missing
CHANGELOG.md	no
License	MIT

Fixes #2822

Moved stuff from the "reset area" to a dedicated tab. Might be better.

Enable OTP 2FA:

Update SchebTwoFactorBundle to version 3
Enable Google 2fa on the bundle
Disallow ability to use both email and google as 2fa
use $this->addFlash shortcut instead of $this->get('session')->getFlashBag()->add
update admin to be able to enable/reset the 2fa

Here is a gif when enabling OTP app:

Backup codes
Hash backup codes
Replace "Google Authenticator"
Ability to test a generated code

Kdecherf

Nice 👍

However I have two notes:

We must provide backup codes for when the device is lost or unavailable, see https://github.com/scheb/two-factor-bundle/blob/master/Resources/doc/backup_codes.md
We should limit our use of the term 'Google Authenticator' as it could be misleading. This authenticator uses a standard OTP mechanism afaik, the documentation and help lines should talk about other authenticator apps like Duo mobile and FreeOTP too

tcitworld

Agreed with @Kdecherf and these two extra remarks:

Put the QR code more upfront (people should use it more than the text code)
From an UX PoV most services which offer 2FA will ask you to confirm 2FA is working with a code generated by the 2FA app, is this doable here ?

virtadpt · 2018-12-02T22:45:06Z

This is probably a dumb question, but will this interfere with existing app integrations (e.g., Android) if we upgrade to get this functionality and turn it on?

tcitworld · 2018-12-03T07:12:52Z

@virtadpt It will indeed.

j0k3r · 2018-12-03T08:04:37Z

@virtadpt the email 2fa is already available. You can give a try.
We added the OTP option in that PR.

j0k3r · 2018-12-06T19:41:35Z

@Kdecherf done
@tcitworld I moved the QR code upfront. About the code validation I agree, but I don't really know how to better integrate it. I think I'll let it for later and create an issue about that.

I have question regarding the OTP secret & backup codes: should the administrator be able to view them? Like:

Kdecherf · 2018-12-07T10:15:16Z

@j0k3r imo administrators must not see secret keys and backup codes. Also, backup codes must not be saved somewhere: they are shown only once.

Here is a side-note on the text of checkbox for OTP: we should avoid "Google" as well here.

j0k3r · 2018-12-07T10:33:34Z

Backup codes are saved in the database, so we can check if the given code is valid (and then remove it from the backup codes).
Ok I agree about secret & codes from the admin.

Kdecherf · 2018-12-07T10:41:24Z

Oh, well, I saw it differently in my mind :D You're right.

tcitworld · 2018-12-07T15:09:41Z

Backup codes are saved in the database, so we can check if the given code is valid (and then remove it from the backup codes).

Is there any reason why we don't hash them inside database ?

j0k3r · 2018-12-07T15:14:43Z

@tcitworld None. We can hash backup code.
But the secret code generated for the OTP app can't be hashed. Which means, an admin can still configure its OTP app with the secret from a given user using the secret code. 😕

noplanman · 2019-01-14T02:05:40Z

Just popping in to ask about the status on this. Would love to see this merged 😊

j0k3r · 2019-01-14T08:34:01Z

I need to get back on this to:

remove info for the admin POV
add a better OTP validation process

j0k3r · 2019-01-15T09:07:22Z

Rebased against the updated 2.4 branch.

src/Wallabag/UserBundle/Controller/ManageController.php

j0k3r · 2019-01-18T21:59:40Z

I've updated how it works now.
We can verify a generated code before enabling it.

Just one thing left: hashing backup codes.

j0k3r · 2019-01-19T19:08:31Z

Regarding my last comment I'm wondering if we really should hash backup code in the database? 🤔

tcitworld · 2019-01-19T21:57:50Z

Regarding my last comment I'm wondering if we really should hash backup code in the database? 🤔

My opinion would be to check how popular libraries / softs handle this.

j0k3r · 2019-01-19T22:40:54Z

GitHub allow you to view your backup codes. So I guess they didn't hash them.

techexo · 2019-01-19T23:15:19Z

I have at least two services (Ubisoft and I think Microsoft) telling me when activating otp "this is your code and don't lose them". So they probably hash them.

Kdecherf

LGTM

As for the hashing of backup codes, we could ignore it as for now considering that the rest of the database is not encrypted.

tcitworld

It seems quite easy to me to have backups codes hashed, so I'd still vote for this before merging it.

The whole point of having backup codes is not being able to see them again once connected, so that if someone gets access to your session on a computer you just left, he's not able to get access to them.

- Update SchebTwoFactorBundle to version 3 - Enable Google 2fa on the bundle - Disallow ability to use both email and google as 2fa - Update Ocramius Proxy Manager to handle typed function & attributes (from PHP 7) - use `$this->addFlash` shortcut instead of `$this->get('session')->getFlashBag()->add` - update admin to be able to create/reset the 2fa

Replace “Google Authenticator” by “Google Authenticator, Authy or FreeOTP” in all text. Translate how to use the code / qr code.

Also remove the forced `server_version` from dbal config to avoid an hard overriding across all database.

And add a step to validate a generated code from the OTP app

j0k3r · 2019-01-23T12:32:45Z

Rebased because of conflicts.

BackupCodes are now hashed

Kdecherf · 2019-01-23T23:16:07Z

It seems quite easy to me to have backups codes hashed, so I'd still vote for this before merging it.

The whole point of having backup codes is not being able to see them again once connected, so that if someone gets access to your session on a computer you just left, he's not able to get access to them.

I didn't think about that vector, you make a point.

Kdecherf · 2019-01-23T23:20:33Z

I'll review and test the workflow tomorrow

Kdecherf

"tomorrow" 6 days ago 👀

looks good 👍

j0k3r added the Ready for review label Dec 2, 2018

j0k3r added this to the 2.4.0 milestone Dec 2, 2018

j0k3r requested review from Kdecherf and tcitworld December 2, 2018 16:39

Kdecherf requested changes Dec 2, 2018

View reviewed changes

tcitworld requested changes Dec 2, 2018

View reviewed changes

j0k3r force-pushed the update-two-factor-bundle branch from b276da4 to 111fd61 Compare January 15, 2019 09:07

noplanman reviewed Jan 15, 2019

View reviewed changes

src/Wallabag/UserBundle/Controller/ManageController.php Show resolved Hide resolved

j0k3r force-pushed the update-two-factor-bundle branch from 111fd61 to 0ad4436 Compare January 18, 2019 15:55

j0k3r requested review from tcitworld and Kdecherf January 19, 2019 19:08

Kdecherf previously approved these changes Jan 20, 2019

View reviewed changes

tcitworld previously requested changes Jan 23, 2019

View reviewed changes

j0k3r added 13 commits January 23, 2019 13:28

Create a dedicated tab to reset data

acd4412

Fix test for custom version of the tidy extension

edc79ad

Fix tests

2dfbe9e

Cleanup

43ccf4b

Better translations

6e4fc95

Replace “Google Authenticator” by “Google Authenticator, Authy or FreeOTP” in all text. Translate how to use the code / qr code.

Add backup codes

dfd0a7b

Add SQLite & PG migration

842af5c

Also remove the forced `server_version` from dbal config to avoid an hard overriding across all database.

Fix PG & Travis drop/create database

6df8b9c

Update translation

e073090

Remove secret from admin

4c0e747

Change the way to enable 2FA

a0c5eb0

And add a step to validate a generated code from the OTP app

CS

c416ed4

j0k3r dismissed Kdecherf’s stale review via c416ed4 January 23, 2019 12:32

j0k3r force-pushed the update-two-factor-bundle branch from 38b14fc to c416ed4 Compare January 23, 2019 12:32

j0k3r added 2 commits January 23, 2019 13:47

Revert PG on Travis about drop/create the database

7485a27

Hash backup codes in the database using password_hash

4654a83

j0k3r requested review from tcitworld and Kdecherf January 23, 2019 13:54

tcitworld approved these changes Jan 23, 2019

View reviewed changes

Kdecherf approved these changes Jan 30, 2019

View reviewed changes

Kdecherf merged commit 2e5b3fa into 2.4 Jan 30, 2019

Kdecherf deleted the update-two-factor-bundle branch January 30, 2019 00:02

j0k3r mentioned this pull request May 29, 2019

Merge 2.4 into master #3984

Merged

j0k3r mentioned this pull request Jun 5, 2019

TOTP as 2FA #1492

Closed

j0k3r mentioned this pull request Oct 25, 2020

Prepare 2.4.0 release #4361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable OTP 2FA #3798

Enable OTP 2FA #3798

j0k3r commented Dec 2, 2018 •

edited

Loading

Kdecherf left a comment

tcitworld left a comment

virtadpt commented Dec 2, 2018

tcitworld commented Dec 3, 2018

j0k3r commented Dec 3, 2018

j0k3r commented Dec 6, 2018

Kdecherf commented Dec 7, 2018

j0k3r commented Dec 7, 2018

Kdecherf commented Dec 7, 2018

tcitworld commented Dec 7, 2018

j0k3r commented Dec 7, 2018 •

edited

Loading

noplanman commented Jan 14, 2019

j0k3r commented Jan 14, 2019 •

edited

Loading

j0k3r commented Jan 15, 2019

j0k3r commented Jan 18, 2019

j0k3r commented Jan 19, 2019

tcitworld commented Jan 19, 2019

j0k3r commented Jan 19, 2019

techexo commented Jan 19, 2019

Kdecherf left a comment

tcitworld left a comment

j0k3r commented Jan 23, 2019

Kdecherf commented Jan 23, 2019

Kdecherf commented Jan 23, 2019

Kdecherf left a comment

Enable OTP 2FA #3798

Enable OTP 2FA #3798

Conversation

j0k3r commented Dec 2, 2018 • edited Loading

Kdecherf left a comment

Choose a reason for hiding this comment

tcitworld left a comment

Choose a reason for hiding this comment

virtadpt commented Dec 2, 2018

tcitworld commented Dec 3, 2018

j0k3r commented Dec 3, 2018

j0k3r commented Dec 6, 2018

Kdecherf commented Dec 7, 2018

j0k3r commented Dec 7, 2018

Kdecherf commented Dec 7, 2018

tcitworld commented Dec 7, 2018

j0k3r commented Dec 7, 2018 • edited Loading

noplanman commented Jan 14, 2019

j0k3r commented Jan 14, 2019 • edited Loading

j0k3r commented Jan 15, 2019

j0k3r commented Jan 18, 2019

j0k3r commented Jan 19, 2019

tcitworld commented Jan 19, 2019

j0k3r commented Jan 19, 2019

techexo commented Jan 19, 2019

Kdecherf left a comment

Choose a reason for hiding this comment

tcitworld left a comment

Choose a reason for hiding this comment

j0k3r commented Jan 23, 2019

Kdecherf commented Jan 23, 2019

Kdecherf commented Jan 23, 2019

Kdecherf left a comment

Choose a reason for hiding this comment

j0k3r commented Dec 2, 2018 •

edited

Loading

j0k3r commented Dec 7, 2018 •

edited

Loading

j0k3r commented Jan 14, 2019 •

edited

Loading