[RFC] two-factor authentication #1545

bytehead · 2018-06-09T15:05:06Z

This is a working draft to add a two-factor authentication mechanism 2FA with TOTP for backend users.
It uses scheb/two-factor-bundle to extend the symfony security authentication mechanism, spomky-labs/otphp for generating and validating one-time passwords and bacon/bacon-qr-code to render QR codes as SVGs.

Please review and mention all missed topics.

Changes to security.yml:

security:
    firewalls:
        contao_backend:
            two_factor:
                auth_form_path: contao_backend_two_factor
                check_path: contao_backend_two_factor_check
                auth_code_parameter_name: verify

    access_control:
        - { path: ^/contao/two-factor, roles: [IS_AUTHENTICATED_2FA_IN_PROGRESS, ROLE_USER] }

Changes to AppKernel.php:

/**
     * {@inheritdoc}
     */
    public function registerBundles()
    {
        $bundles = [
  			// ...
            new Scheb\TwoFactorBundle\SchebTwoFactorBundle(),
        ];
		// ...
    }

How to enforce 2FA for backend users via config.yml:

contao:
    authentication:
        two_factor:
            enforce_backend: true    #default: false

ToDos:

Move QR code image to user profile
Unit tests
Option to enforce 2FA for all backend users
Icon in the backend list view of tl_user indicating which users have 2FA enabled

Possible new features (future PRs):

2FA also for frontend users

richardhj · 2018-06-12T10:06:00Z

🙌

I'm really looking forward to see this feature.

You aren't using the Google Authenticator already part of scheb/two-factor-bundle because it is not that generic than the implementation of spomky-labs/otphp?
We might want an icon in the backend list view of tl_user indicating which users have 2FA enabled.
We might want to force users to use 2FA.
2FA must only get activated after the user successfully confirmed the setup with a token generated on his 2FA device.

And currently im getting the Unrecognized option "two_factor" under "security.firewalls.contao_backend" error. Do I need to alter the configuration in the DI extension?

bytehead · 2018-06-12T20:31:46Z

You aren't using the Google Authenticator already part of scheb/two-factor-bundle because it is not that generic than the implementation of spomky-labs/otphp?

Yes I don't use it. It didn't work with all of my apps I've tried upfront. That's why I decided to choose a library that supports actually the RFCs. Even easier, if we decide to support HOTP in future.

We might want an icon in the backend list view of tl_user indicating which users have 2FA enabled.

I like the idea, @contao/developers what do you think about?

We might want to force users to use 2FA.

IMHO the users should enable 2FA by themselves.

2FA must only get activated after the user successfully confirmed the setup with a token generated on his 2FA device.

I will implement this, already discussed 👍

And currently im getting the Unrecognized option "two_factor" under "security.firewalls.contao_backend" error. Do I need to alter the configuration in the DI extension?

Did you miss to enable the SchebTwoFactorBundle?

leofeyer · 2018-06-13T06:46:13Z

We might want an icon in the backend list view of tl_user indicating which users have 2FA enabled.

I like the idea, @contao/developers what do you think about?

Go for it.

We might want to force users to use 2FA.

IMHO the users should enable 2FA by themselves.

I also think we should not force them.

fritzmg · 2018-06-13T08:00:03Z

We might want to force users to use 2FA.

IMHO the users should enable 2FA by themselves.

I also think we should not force them.

I think @richardhj meant that you should optionally be able to enforce 2FA for one Contao Installation for everyone. Not that it should be enforced in general. This makes sense imho.

richardhj · 2018-06-13T08:03:58Z

I think @richardhj meant that you should optionally be able to enforce 2FA for one Contao Installation for everyone. Not that it should be enforced in general. This makes sense imho.

Optionally for some users or user groups, in order to comply with some TOMs :-)
Fore sure it is not necessary.

bytehead · 2018-06-13T08:28:31Z

I think @richardhj meant that you should optionally be able to enforce 2FA for one Contao Installation for everyone. Not that it should be enforced in general. This makes sense imho.

I don't get this one.

If somebody else enforces 2FA in general the question will be how the users will initially setup the app in this case?

aschempp · 2018-06-13T08:34:49Z

We might want an icon in the backend list view of tl_user indicating which users have 2FA enabled.

I like the idea, @contao/developers what do you think about?

I like it as well. Maybe a badge on the user/admin icon like the page publishing options?

If somebody else enforces 2FA in general the question will be how the users will initially setup the app in this case?

Something like forcing the setup after login attempt?

leofeyer · 2018-06-13T08:42:54Z

Please don't do this. None of the apps I am using 2FA with does it – and I am using a lot of them.

patrickjDE · 2018-06-13T21:44:21Z

As an option to enforce 2FA upon users would be, well.. optional, I don't see a problem there. It'd work analogous to "Password change required". Upon first/next login the user is required to change his password/setup 2FA, whatever the admin's reasons for that decision might be.

If 2FA becomes a core feature, so should the option to enforce its usage upon all or specific users.

fritzmg · 2018-06-14T07:38:27Z

I agree with @patrickjDE . A company should ideally be able to decide whether to force all editors of their website two use 2FA in order to increase security.

@leofeyer you are probably talking about a front end context there, right? But this is about the back end functionality. If your website has 2 administrators for the back end and only one uses 2FA, the other account is still (maybe) more vulnerable. Hence you might want to decide that every back end user (or specific back end user groups - or just administrators) should use 2FA.

bytehead · 2018-06-18T07:46:41Z

We might want an icon in the backend list view of tl_user indicating which users have 2FA enabled.

@frontendschlampe do you want to create an icon for it? 😎

Toflar · 2018-06-19T07:12:42Z

src/Resources/contao/dca/tl_user.php

+				continue;
+			}
+
+			$GLOBALS['TL_DCA']['tl_user']['palettes'][$palette] = str_replace


Why not use the PaletteManipulator here? Would make things easier 😄

Definitely! 😎

Toflar · 2018-06-19T07:18:18Z

src/Resources/contao/dca/tl_user.php

+			// Generate 1024 bit secret
+			$secret = random_bytes(128);
+
+			$this->Database->prepare("UPDATE tl_user SET secret=? WHERE id=?")


Shouldn't that be something like this?

$user = BackendUser::getInstance(); $user->secret = random_bytes(128); $user->save();

Yep. Can't see the forest for the trees.

Toflar · 2018-06-19T07:19:14Z

src/Resources/contao/dca/tl_user.php

+<div class="' . $class . ' widget">
+  <div id="ctrl_' . $dc->field . '" class="">
+    <h3><label for="ctrl_' . $dc->field . '">' . $GLOBALS['TL_LANG']['tl_user']['2faQrCode'][0] . '</label></h3>
+    <img src="data:image/svg+xml;base64,' . base64_encode($twoFactorAuthenticator->getQrCode($this->User, $request)) . '" />


Can we use BackendUser::getInstance() everywhere instead of $this->User that's stored somewhere in nowhere?

Toflar · 2018-06-19T07:19:38Z

src/Resources/contao/dca/tl_user.php

+			throw new Exception($GLOBALS['TL_LANG']['ERR']['invalidTwoFactor']);
+		}
+
+		return 1;


Imho true should work.

Toflar · 2018-06-19T07:21:18Z

src/Security/TwoFactor/ContaoTwoFactorAuthenticator.php

+     */
+    public function validateCode(User $user, string $code): bool
+    {
+        // The 2FA app from Google (Google authenticator) does not strictly confirm to RFC 4648 [1] (they confirm to the old RFC 3548 [2]).


private function getUpperUnpaddedScretForUser(User $user): string

Then you need to add the google comment only once :-P

frontendschlampe · 2018-06-19T13:56:20Z

@frontendschlampe do you want to create an icon for it?

yes ... :-) Today is icon day :-D

frontendschlampe · 2018-06-19T22:11:12Z

What do you think?

richardhj · 2018-06-19T22:12:24Z

Love it.
Should be on the left side since it is no action button though.

bytehead · 2018-06-20T05:18:06Z

@frontendschlampe Yeah, I like! I'll place it on the left as @richardhj already stated.

bytehead · 2018-06-20T05:34:03Z

Here is the implemented version:

frontendschlampe · 2018-06-20T06:26:54Z

yeah ... no problem. I‘ve added the icon to my PR yesterday with the other icons. :-)

aschempp · 2018-07-11T19:22:20Z

With the change in 4e36d00, so we actually still need a different route for the 2FA or can we just apply 2FA on the login route?

bytehead · 2018-07-11T19:23:50Z

@aschempp see here: #1545 (comment)

…heck route.

aschempp · 2018-07-12T07:56:55Z

src/Security/TwoFactor/Provider.php

+            return false;
+        }
+
+        if (!$this->enforceTwoFactor && !$user->useTwoFactor) {


So if the user does not have a secret (which is the default, if he never edited his data), then we don't enforce 2FA?

This seems correct to me. Otherwise the user would be asked to enter their verification code although they never had a chance to set up 2FA.

In fact that's wrong. If there is no secret and 2FA is enforced, a secret should be generated, so the user gets the QR code to verify on the login screen.

Fixed in eac180e.

…ser.

…e yet.

bytehead · 2018-07-12T08:58:27Z

src/Security/TwoFactor/Provider.php

-            return false;
+        // Generate a secret if 2FA is enforced and the user has not yet enabled it
+        if ($this->enforceTwoFactor && !$user->secret) {
+            $user->secret = random_bytes(128);


Isn't here a $user->save(); missing?

IDK. Can we be sure that $context->getUser() returns an object with a save() method? Also it seems to work without save().

Yes we can, it returns a Contao\User object.

Added in 9f3b5df.

bytehead

👍

leofeyer · 2018-07-12T10:00:13Z

src/Security/TwoFactor/Provider.php

+        }
+
+        // 2FA is now confirmed, save the user flag
+        if ($this->enforceTwoFactor && !$user->confirmedTwoFactor) {


Why do we only set this if 2FA is enforced?

Because this only happens if 2FA is enforced by config. Otherwise it gets saved here.

…he secret.

leofeyer · 2018-07-12T10:23:54Z

Thank you big time @bytehead!

Description ----------- This is what I meant in #1545 (comment) Commits ------- 3b35e23 Drop the unnecessary 2FA login route 2da1351 Also adjust the tests and the firewall and rename the check route. 2398caa Merge branch 'master' into feature/no-2fa-route

Description ----------- - improve icon modules.svg - add icons for contao/core-bundle#1202 - add icon for contao/core-bundle#1545 Commits ------- abb6003b add/improve some icons f14694e4 improve size of 2fa icons (same height like user icons) d37484eb improve module icon again a86c900d delete single 2fa-icons and add user/admin icon with lock aeaf343b improve size units of some icons 3922030e improve position of icon elements 0c427ba5 Run the gulp task.

Description ----------- This is what I meant in contao/core-bundle#1545 (comment) Commits ------- 3b35e23d Drop the unnecessary 2FA login route 2da1351d Also adjust the tests and the firewall and rename the check route. 2398caa0 Merge branch 'master' into feature/no-2fa-route

BoozieBadd · 2022-03-12T04:33:30Z

#1545

leofeyer assigned bytehead Jun 9, 2018

leofeyer added the feature label Jun 9, 2018

leofeyer added this to the 4.6.0 milestone Jun 9, 2018

bytehead mentioned this pull request Jun 11, 2018

Subpalettes won't work on users profile page #1552

Closed

bytehead force-pushed the feature/two-factor branch 4 times, most recently from 935034f to 6203d91 Compare June 11, 2018 22:15

bytehead force-pushed the feature/two-factor branch from a73d12f to 12517ea Compare June 12, 2018 16:58

bytehead force-pushed the feature/two-factor branch from 51833b6 to 378e260 Compare June 16, 2018 08:13

bytehead force-pushed the feature/two-factor branch from 17fa0a8 to 9bec2f7 Compare June 18, 2018 15:04

Toflar suggested changes Jun 19, 2018

View reviewed changes

frontendschlampe mentioned this pull request Jun 19, 2018

add/improve some icons #1575

Merged

bytehead added 2 commits July 11, 2018 18:19

Added removal notice for dummy controller

8d520a2

Fix save callback logic

5683059

bytehead force-pushed the feature/two-factor branch from 8d30ffe to 5683059 Compare July 11, 2018 16:36

leofeyer added 2 commits July 12, 2018 09:10

Fix the icon logic and coding style.

17bea1a

Also redirect the user if they manually call the /contao/two-factor-c…

168a620

…heck route.

leofeyer force-pushed the feature/two-factor branch from c39a5c5 to 168a620 Compare July 12, 2018 07:18

Disable the "use two factor" checkbox if 2FA is enforced.

81b6977

aschempp reviewed Jul 12, 2018

View reviewed changes

bytehead and others added 3 commits July 12, 2018 10:05

Fixed test

ec32642

Correctly clear the 2FA data if an admin disables 2FA for a regular u…

b65d316

…ser.

Correctly generate the secret if 2FA is enforced and the user has non…

eac180e

…e yet.

bytehead commented Jul 12, 2018

View reviewed changes

leofeyer added 2 commits July 12, 2018 11:20

Save the user after generating the secret.

9f3b5df

Add a test for the contao_backend_two_factor_check route.

bbdb154

bytehead commented Jul 12, 2018

View reviewed changes

leofeyer reviewed Jul 12, 2018

View reviewed changes

Do not copy the 2FA data when a record is duplicated and never show t…

fb869b4

…he secret.

leofeyer merged commit b6ce152 into contao:master Jul 12, 2018

bytehead deleted the feature/two-factor branch July 12, 2018 10:25

aschempp mentioned this pull request Jul 13, 2018

Drop the unnecessary 2FA login route #1619

Merged

leofeyer added a commit to contao/contao that referenced this pull request Jan 11, 2019

Remove a leftover class from merging contao/core-bundle#1545

c76dd63

leofeyer added a commit that referenced this pull request Jan 11, 2019

Remove a leftover class from merging #1545

3645f8a

richardhj mentioned this pull request Nov 13, 2019

Explain 2FA for back end users contao/docs#165

Merged

[RFC] two-factor authentication #1545

[RFC] two-factor authentication #1545

Conversation

bytehead commented Jun 9, 2018 • edited Loading

ToDos:

Possible new features (future PRs):

richardhj commented Jun 12, 2018

bytehead commented Jun 12, 2018 • edited Loading

leofeyer commented Jun 13, 2018

fritzmg commented Jun 13, 2018 • edited Loading

richardhj commented Jun 13, 2018

bytehead commented Jun 13, 2018

aschempp commented Jun 13, 2018

leofeyer commented Jun 13, 2018

patrickjDE commented Jun 13, 2018

fritzmg commented Jun 14, 2018 • edited Loading

bytehead commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frontendschlampe commented Jun 19, 2018

frontendschlampe commented Jun 19, 2018

richardhj commented Jun 19, 2018

bytehead commented Jun 20, 2018

bytehead commented Jun 20, 2018

frontendschlampe commented Jun 20, 2018

aschempp commented Jul 11, 2018

bytehead commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bytehead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leofeyer commented Jul 12, 2018

BoozieBadd commented Mar 12, 2022

bytehead commented Jun 9, 2018 •

edited

Loading

bytehead commented Jun 12, 2018 •

edited

Loading

fritzmg commented Jun 13, 2018 •

edited

Loading

fritzmg commented Jun 14, 2018 •

edited

Loading