Skip to content
This repository has been archived by the owner on Dec 2, 2021. It is now read-only.

Integrate Two-Factor Authentication Into Security Layer #73

Closed
scheb opened this issue Sep 8, 2016 · 36 comments
Closed

Integrate Two-Factor Authentication Into Security Layer #73

scheb opened this issue Sep 8, 2016 · 36 comments

Comments

@scheb
Copy link
Owner

scheb commented Sep 8, 2016

Excerpt from the documentation:

Limitations

After the initial login happened, the user is already fully authenticated to the Symfony security layer. The bundle then prevents access to secured and non-secured content by intercepting any request and showing the two-factor authentication form instead.

If you execute code based on the authentication status, make sure to take the two-factor status into account. This can be done by checking access with isGranted (security voter has to be registered, see configuration).

Warning: Just doing a getUser on security.token_storage (or the old security.context) is not secure. You will get a user object even when two-factor authentication is not complete yet.

Overall, the current implementation causes some issues, which cannot really be solved, as long as two-factor authentication doesn't become part of the actual security layer.

@scheb
Copy link
Owner Author

scheb commented Sep 8, 2016

There is a experimental "proof-of-concept" implementation in branch firewall-integration. In the following I'll describe what I did and where the problems are. Hopefully someone can point me into the right direction.

Two-factor authentication becomes a real authentication provider, which has to be enabled in the firewall configuration.

On login, the authenticated token (e.g. a UserPasswordToken) is wrapped with a TwoFactorToken. The TwoFactorToken is always unauthenticated and has no roles at all. The contained authenticated token and its priviledges are not exposed, therefore the security layer handles the user like not being logged in at all (similar to AnnonymousToken).

Wrapping the authenticated token with a TwoFactorToken is currently done by decorating the AuthenticationProviderManager. I don't like that solution, but so far this was the only way how I could make sure that the authenticated token is wrapped away early enough.

In the latest implementation there is a compiler pass to decorate all authentication providers with some logic to wrap the security token. When an authentication provider returns a token, which requires two-factor authentication, the decorator wraps the original token with a TwoFactorToken.

The TwoFactorToken has the disadvantage, that it's already interpreted as "fully authenticated" (role IS_FULLY_AUTHENTICATED passes), because it is not an AnonymousToken. Extending the AnonymousToken didn't work, because an AnonymousToken is not written to the session (that's hardcoded somewhere in the security layer). Therefore the solution (which is better anyways) was to replace the original AuthenticationTrustResolver with a custom implementation. So we have a TwoFactorToken now, which is unauthenticated and doesn't expose any priviledges to the security layer, that the user should have yet. So priviledges issues solved.

Although there is a AuthenticationTrustResolverInterface for that case, replacing it comes with a twist. The symfony/security-acl package depends on AuthenticationTrustResolver instead of AuthenticationTrustResolverInterface. Pull request issued: symfony/security-acl#26. Update: Pull request was merged.

The TwoFactorToken will trigger the TwoFactorListener authentication listener, which redirects the user to the authentication form (Controller and route need to be configured).

User enters authentication code and submits the form. TwoFactorListener checks the code. If the code is correct, the authenticated token is unwrapped from the TwoFactorToken and returned instead. Authentication complete and user is authenticated with a normal token.

@scheb
Copy link
Owner Author

scheb commented Sep 8, 2016

Wrapping the authenticated token with a TwoFactorToken is currently done by decorating the AuthenticationProviderManager. I don't like that solution, but so far this was the only way how I could make sure that the authenticated token is wrapped away early enough. Alternative solution needed.

@stof Since you already commented on the firewall issue. Do you have an idea how to solve that? Or can you point me to someone who could? Thanks!

@scheb scheb added the BC break label Sep 8, 2016
@sebastianblum
Copy link

hello @scheb,

I would like to help you with the problem. At the moment, I don't understand how to reproduce the problem in unit tests. Maybe we can work together on a solution.

@scheb
Copy link
Owner Author

scheb commented Sep 10, 2016

@sebastianblum I'm not sure which problem you mean. The problem with the master request (issues #70 & #71) cannot be reproduced with unit tests, because it only appears when the bundle interacts with the security layer in a real working environment.

This issue here is about integrating the bundle in Symfony's security layer. As a proof-of-concept I've put together an implementation, which can be seen in that firewall-integration branch. None of that integration already has tests, it's just quickly put together, so see if it would work at all. So you cannot test anything of that new implementation through tests - yet. Although the current implementation would work, I'm not sure about certain solutions, especially the way the token is wrapped. So some advice from a security-component expert would be really helpful.

@scheb
Copy link
Owner Author

scheb commented Sep 11, 2016

Update:

Wrapping the authenticated token with a TwoFactorToken is currently done by decorating the AuthenticationProviderManager. I don't like that solution, but so far this was the only way how I could make sure that the authenticated token is wrapped away early enough. Alternative solution needed.

This is no longer the case.

In the latest implementation there is a compiler pass to decorate all authentication providers with some logic to wrap the security token. Best solution so far.

@walva
Copy link

walva commented Sep 14, 2016

Hello,

Since you want to create a firewall "type", I guess you will keep the login form, and add the "two facto" form for google, email, sms and so on. It also can be merged into one form which I think is a very bad idea. If we stick with the first plan, what role would have the user on the second form?
IS_AUTHENTICATED_FULLY ?
IS_AUTHENTICATED_REMEMBERED ?
IS_AUTHENTICATED_IN_PROGRESS or something else?
Or event IS_AUTHENTICATED_ANONYMOUSLY?

To be honest, I think it's time for a new role :D

@scheb
Copy link
Owner Author

scheb commented Sep 14, 2016

The user would only have IS_AUTHENTICATED_ANONYMOUSLY when the two-factor authentication has not been completed.

Adding a new role like IS_AUTHENTICATED_IN_PROGRESS is hardly possible because the interface all security classes depend on - AuthenticationTrustResolverInterface - doesn't have a method to check for that new status and therefore the callers wouldn't even know that it's possible to check for such a role. So I ended up with the solution to treat the intermediate state (two-factor not completed) like as IS_AUTHENTICATED_ANONYMOUSLY.

@walva
Copy link

walva commented Sep 15, 2016

I understand your point of you. I'll explain my expected usage:
I'm implementing a marketplace where there is : Members (buyers and sellers) and Admin (operation).
We decided to force our admin to have two factor authentication. If they are not two factor authenticated, they only have access to the member section. We don't plain to ask/force two factor for our member yet. But we might enable this functionality "on demand" (flag in the database?) in order to require two factor for critical operation (buy or sell, data modification, etc...).

One solution that would be viable is to consider "IS_AUTHENTICATED_FULLY" like "IS_AUTHENTICATED_REMEMBERED" and create a more trusted status like "IS_MULTIPLE_FACTOR_AUTHENTICATED". I believe this is not easy.

Anyway, I wanted to share this situation with you in order to build a strong vision of what you want to achieve. I think it might require some change in the security component, I don't know if it is very open to such improvement. I'll try to help as much as possible.

@sebastianblum
Copy link

@SulivanDotEu I understand what you want, but i think your solution is to complicated.

The 2 Factor Authentication (in the case of google authenticator) is enabled, if a secret in stored in the user object. So you can have both, users with and without 2 factor authentication.

The #13 issue is a proposal, that the 2factor can configured on individual firewalls and that you should solve your problem. You need 2 firewalls like main and admin and only for the admin firewall you enable the 2 factor authentication.

@walva
Copy link

walva commented Sep 15, 2016

@sebastianblum It's complicated I agree. But those kind of solution would allow developer to configure the access_control like this:
access_control:
- { path: ^/user/login$, role: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/user/register, role: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/user/resetting, role: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/member, role: ROLE_USER }
- { path: ^/admin, role: IS_MULTIPLE_FACTOR_AUTHENTICATED }
- { path: ^/member/buy, role: IS_MULTIPLE_FACTOR_AUTHENTICATED }

As you mention, I use the "secret is empty or not" to active for specific users.

@eman1986
Copy link

is there any new progress with this process?

@scheb
Copy link
Owner Author

scheb commented Feb 27, 2017

No, not really. There a the proof-of-concept branch, which is working to some degree (some features are missing), but overall I'm not satisfied with the implementation. Too many things that feel hacky and unstable. I wanted to get advise from someone with deeper knowledge on the security component, but didn't get a reaction from the Symfony core people. Also, I have a pull request pending for the security-acl component, which is laying around for months now. Kind of disappointing. And then, since I couldn't get any progress, lost interest in it, especially since I myself am no longer using the bundle actively.

@eman1986
Copy link

Ok, I'm more of using this a reference on how to apply two factor to silex, which has been helping a lot so I thank you for that. 😄

kinda surprised symfony isn't more interested in baking it in, seeing that their security component is so robust and 2FA is a growing demand for login security.

Depending on how successful I get with it, maybe I'll try to offer some help to this repo in exchange for it helping me get a good bounce-off point.

@Lozik
Copy link

Lozik commented Mar 29, 2017

@scheb I completely understand your situation. I'm thankful for your bundle, even that I would love to see a proper integration into the security layer with, as @SulivanDotEu asked for it, proper roles connected to it. Like @eman1986 remarked, it's extremely surprising that Symfony didn't think of integrating such things in the core. It seems that Symfony may be sort of powerful in that domain but not flexible.

The 2 factor authentication should also adapt to the firewalls, as described above. Why should it block access to public pages when only the user password has been entered and the one-time password hasn't yet? I guess it's simply a technical restriction.

Two factor authentication would be so easy to implement and configure; the 'only' hard thing is to integrate it in a proper way into Symfony and collect it into a bundle. I'm tempted to call handlers in each controller where authentication is required. Not clean at all, I know. 😞

@eman1986
Copy link

I'm close to a working solution, though I'm using symfony guard which does make things a bit easier and give you a bit more control.

I have more work to do as its not 100% functional yet but I'm very close.

@Lozik
Copy link

Lozik commented Mar 31, 2017

@eman1986 That's great news! Looking forward to it.

By the way, anti brute force has not been foreseen by the Symfony-creators either. (But that should be quite easy to implement.)

@sebastianblum
Copy link

great @eman1986 :)

@flo-sch
Copy link

flo-sch commented Apr 6, 2017

Very nice to hear @eman1986!

@DamienHarper
Copy link

@eman1986 Do you finally have a working solution?

@eman1986
Copy link

I still have one kink to work out but I've tabled it to work on a different project. I'm looking to get it working fully once I get a chance to get back to it. With guard, its somewhat easy to do, just need to get one small thing figured out and I'll have it working fully.

I should note, I'm using Silex on my project so once I get it fully working, it may require some extra work for those on a full symfony stack (mainly the wire-up portion as they do it completely different)

@cordoval
Copy link
Contributor

@eman1986 if you have it working on silex should be a bliss to move it to symfony, thanks so much 👍

@eman1986
Copy link

I think someone who has symfony experience would be able to port it over pretty easily. I'll make a gist of it once I get back to working on it. The only hiccup I'm hitting is its going into an endless login loop, I think I may know what it is, but that's what I'm stuck on with it before I can call it a success. I may be taking another look at it soon as my current project could use it too.

@scheb
Copy link
Owner Author

scheb commented Jul 22, 2017

@eman1986 Would be great if you could share your solution with us. I think everyone here is curious.

@scheb
Copy link
Owner Author

scheb commented Jul 22, 2017

Btw. my pull request symfony/security-acl#26 was merged recently. But no release yet that includes the commit.

@cordoval
Copy link
Contributor

the PR is good @scheb but what would be nice is to describe how to tab into that Interface

@cordoval
Copy link
Contributor

@dunglas told me that the reason that it was delayed was that there is not assigned maintainer for the ACL component. But he did merge it 👍

@eman1986
Copy link

I'm going to be working on this again, once I get all the kinks out, I'll make a gist that everyone can look at to get an idea of how to do this.

@scheb
Copy link
Owner Author

scheb commented Nov 18, 2017

Since Symfony 4 is coming, I give it another try to get in contact with the Symfony folks. I still think it would be a way better bundle if it was properly integrated with the security layer.

@scheb
Copy link
Owner Author

scheb commented Jan 28, 2018

So this is finally happening. Created a 3.x branch where I'm currently reworking the old "firewall-integration" branch for Symfony 3.4+

@scheb scheb added this to the 3.x milestone Jan 30, 2018
@scheb
Copy link
Owner Author

scheb commented Mar 1, 2018

Ping @sebastianblum @bytehead

The current dev-master (v3.0.0-beta1 release for your convenience) version is now feature-complete for the 3.x version. Feel free to test it. Upgrade instructions can be found here: https://github.com/scheb/two-factor-bundle/blob/master/UPGRADE.md

The only "feature" missing is a proper implementation for the controller.

Things I'd like to get feedback on:

  • Is it working for you? :D
  • Are the upgrade instructions complete or did I miss something?
  • Any ideas how to build a proper controller? One which can be customized. To me the biggest open question is how customly built two-factor providers can display with their own templates. Should we e.g. have a renderForm method in the TwoFactorProviderInterface?

@bytehead
Copy link
Contributor

bytehead commented Mar 2, 2018

Thank you!
I'll test the next days.

Any ideas how to build a proper controller? One which can be customized. To me the biggest open question is how customly built two-factor providers can display with their own templates. Should we e.g. have a renderForm method in the TwoFactorProviderInterface?

Exactly one of my questions and needs I'll have. 😄

@scheb
Copy link
Owner Author

scheb commented Mar 2, 2018

Proposed solution for form rendering:

TwoFactorProviderInterface::getFormRenderer(): TwoFactorFormRendererInterface
TwoFactorFormRendererInterface::renderForm(Request $request, array $templateVars): Response

@stephanvierkant
Copy link
Contributor

\R\U2FTwoFactorBundle\Security\TwoFactor\Prodiver\U2F\TwoFactorProvider::requestAuthenticationCode requires injecting authenticationData into the template, so implementing a custom render method that allows using the Authenticator

@scheb
Copy link
Owner Author

scheb commented Mar 5, 2018

v3.0.0-beta2 now has a working controller and form renderer implementation. Please note that you need to update the route config if you've been on beta1 before.

@scheb
Copy link
Owner Author

scheb commented Mar 11, 2018

And there it is: 3.0.0 out now. Thanks to everyone contributing during the development process! Special thanks to @chalasr who pointed me into the right directions in the beginning.

@scheb scheb closed this as completed Mar 11, 2018
@bytehead
Copy link
Contributor

Thanks a lot!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants