Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add trusted devices for 2FA #559

Merged
merged 65 commits into from Jan 18, 2020
Merged

Conversation

@bytehead
Copy link
Member

bytehead commented Jul 7, 2019

This is a first draft of a trusted devices implementation for the Contao 2FA.
It uses the built-in stuff from the scheb/two-factor-bundle and I would consider this as a poor man implementation because it only sets a versioned cookie and does not care about user agent or the like.

My advice would be to store a GUID in a cookie or in local storage AND use the User-Agent header to identify and store the device server-side as a trusted device. As of this we can show a list of trusted devices to the user where he easily can remove the ones he wants (or even all).

We should send an email when a new device is flagged as a trusted device. If the user didn't add it, or if he doesn't recognize the device information, he can clear his trusted devices immediately.

ToDo:

  • Decide if cookie or local storage
  • Store a list of trusted devices server-side (e.g. User-Agent with cookie GUID or the like)
  • Send an e-mail if a new trusted device is added
  • Implement it as well for frontend users when #363 is merged
  • Tests

2FA login screen:
Bildschirmfoto 2019-09-02 um 15 59 21

2FA backend module:
Bildschirmfoto 2019-09-02 um 16 00 39

2FA frontend module:
Bildschirmfoto 2019-09-02 um 22 32 05

@bytehead bytehead added the feature label Jul 7, 2019
@bytehead bytehead self-assigned this Jul 7, 2019
@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jul 7, 2019

See #265 (comment) as well.

@bytehead bytehead added this to the 4.9 milestone Jul 7, 2019
@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jul 8, 2019

Local storage and server side sounds like a super strange combination to me?! 😄

@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jul 8, 2019

Why? We can use a combination of both to identify trusted devices.

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Jul 8, 2019

How is the server supposed to validate data in the local storage?

@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jul 8, 2019

🤦‍♂ Was too early in the morning 😂

@bytehead bytehead changed the title POC: 2FA Trusted Devices [POC] 2FA Trusted Devices Jul 9, 2019
@leofeyer leofeyer force-pushed the contao:master branch 2 times, most recently from b2174a9 to 8728020 Jul 10, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-trusted-devices branch 2 times, most recently from 9cc1497 to a780be2 Jul 10, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-trusted-devices branch from a780be2 to 879e33c Sep 2, 2019
@bytehead bytehead changed the title [POC] 2FA Trusted Devices [RFC] 2FA Trusted Devices Sep 3, 2019
@bytehead bytehead marked this pull request as ready for review Sep 3, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-trusted-devices branch 2 times, most recently from 1567d04 to 75cff33 Sep 3, 2019
@leofeyer leofeyer force-pushed the contao:master branch from 2ea6ebd to 91d73a0 Sep 26, 2019
@leofeyer leofeyer mentioned this pull request Nov 11, 2019
@richardhj

This comment has been minimized.

Copy link
Member

richardhj commented Nov 17, 2019

It uses the built-in stuff from the scheb/two-factor-bundle and I would consider this as a poor man implementation because it only sets a versioned cookie and does not care about user agent or the like.

A cookie can be set/modified client-side, but the same is for the User-Agent. Why is checking for both cookie and user-agent considered as more secure then?
If you update your Firefox from version 70 to 72, is this still considered as the same trusted device?

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Nov 18, 2019

I would assume the cookie is sort of signed to prevent modification. As you can see in the screen shot, only the browser name is shown but not the version, so that should not be a problem?

@bytehead bytehead force-pushed the bytehead:feature/2fa-trusted-devices branch 2 times, most recently from 1080920 to 4b7cb34 Nov 30, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-trusted-devices branch from 4b7cb34 to c081695 Dec 7, 2019
@bytehead bytehead changed the title [RFC] 2FA Trusted Devices [WIP] 2FA Trusted Devices Dec 9, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-trusted-devices branch 2 times, most recently from 8a8f331 to 064f590 Dec 23, 2019
aschempp and others added 10 commits Jan 17, 2020
CS
CS
CS
@bytehead bytehead requested a review from leofeyer Jan 17, 2020
leofeyer added 2 commits Jan 18, 2020
@leofeyer leofeyer changed the title [RFC] 2FA Trusted Devices Add trusted devices for 2FA Jan 18, 2020
@leofeyer leofeyer merged commit b6d81c4 into contao:master Jan 18, 2020
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.2
Details
PHP 7.3
Details
PHP 7.4
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project No report found to compare against
Details
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 18, 2020

Thanks a lot for your hard work @bytehead.

@bytehead bytehead deleted the bytehead:feature/2fa-trusted-devices branch Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.