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

Feature Request: Additional security checks for certain resources ( aka websudo ) #84

Closed
noxify opened this issue Feb 9, 2017 · 18 comments

Comments

@noxify
Copy link
Contributor

noxify commented Feb 9, 2017

The Idea (pasted from Slack)

I'm using the the two factor auth via app, everything is working fine in the frontend.
Now I had the idea to secure the backend/admin with the following:

  • Check that the user has the permission to see the dashboard / access to the backend
  • Check that the user has two factor enabled
  • User has to type a generated code from the app again

In confluence/jira and/or github there is something simular, but they require (only) the user password again.

Current Implementation

User Access a resource 
--> Middleware checks that the user is logged in
--> If defined, check also defined permissions (example via routes: `->middleware(['web', 'can:access-dashboard'])`)
--> All checks passed?
=> Yes: show requested resource
=> No: Show error message and redirect back

Future Implementation

User Access a resource 
--> Middleware checks that the user is logged in
--> If defined, check also defined permissions (example via routes: `->middleware(['web', 'can:access-dashboard'])`)
--> All checks passed?
--> security check defined?
---> Check which kind of security check is required
----> Password: Show additional form, where the user has to type the password again
----> 2FA: Show additional form, where the user has to type the 2FA token from the app/sms 
=> Yes: show requested resource
=> No: Show error message and redirect back

Both described implementations are simplified

How can we implement it

Via Guard / Middleware

I had the idea to create my own Guard (for example: admin guard).
Each route which should handled via the admin guard, will have a custom middleware.

Example:

`->middleware(['web', 'admin', 'can:access-dashboard'])`)

The idea was to use the existing functionality to keep the code maintainable (don't repeat yourself ;) )
This means, we can define it in the routes for each resource or via the controller "globally" for all resource actions.

Via Policy

While writing the Via Guard section, i have checked the policy documentation.
The idea here is to define, what we need in the policies.

Why? In the current implementation we have already the definitions for the resources and actions.

Example:

|        | GET|HEAD  | backend                            | rinvex.fort.backend.dashboard.home                 | Rinvex\Fort\Http\Controllers\Backend\DashboardController@home                             | web,can:access-dashboard,auth,can:access-dashboard,dashboard |
|        | GET|HEAD  | backend/abilities                  | rinvex.fort.backend.abilities.index                | Rinvex\Fort\Http\Controllers\Backend\AbilitiesController@index                            | web,can:access-dashboard,auth,can:list-abilities,abilities   |
|        | GET|HEAD  | backend/abilities/create           | rinvex.fort.backend.abilities.create               | Rinvex\Fort\Http\Controllers\Backend\AbilitiesController@create                           | web,can:access-dashboard,auth,can:create-abilities,abilities |
|        | POST      | backend/abilities/create           | rinvex.fort.backend.abilities.store                | Rinvex\Fort\Http\Controllers\Backend\AbilitiesController@store                            | web,can:access-dashboard,auth,can:create-abilities,abilities |
|        | GET|HEAD  | backend/abilities/{ability}        | rinvex.fort.backend.abilities.edit                 | Rinvex\Fort\Http\Controllers\Backend\AbilitiesController@edit                             | web,can:access-dashboard,auth,can:update-abilities,abilities |
|        | PUT       | backend/abilities/{ability}        | rinvex.fort.backend.abilities.update               | Rinvex\Fort\Http\Controllers\Backend\AbilitiesController@update                           | web,can:access-dashboard,auth,can:update-abilities,abilities |
|        | DELETE    | backend/abilities/{ability}        | rinvex.fort.backend.abilities.delete               | Rinvex\Fort\Http\Controllers\Backend\AbilitiesController@delete                           | web,can:access-dashboard,auth,can:delete-abilities,abilities |
|        | GET|HEAD  | backend/roles                      | rinvex.fort.backend.roles.index                    | Rinvex\Fort\Http\Controllers\Backend\RolesController@index                                | web,can:access-dashboard,auth,can:list-roles,roles           |
|        | POST      | backend/roles/create               | rinvex.fort.backend.roles.store                    | Rinvex\Fort\Http\Controllers\Backend\RolesController@store                                | web,can:access-dashboard,auth,can:create-roles,roles         |
|        | GET|HEAD  | backend/roles/create               | rinvex.fort.backend.roles.create                   | Rinvex\Fort\Http\Controllers\Backend\RolesController@create                               | web,can:access-dashboard,auth,can:create-roles,roles         |
|        | DELETE    | backend/roles/{role}               | rinvex.fort.backend.roles.delete                   | Rinvex\Fort\Http\Controllers\Backend\RolesController@delete                               | web,can:access-dashboard,auth,can:delete-roles,roles         |
|        | PUT       | backend/roles/{role}               | rinvex.fort.backend.roles.update                   | Rinvex\Fort\Http\Controllers\Backend\RolesController@update                               | web,can:access-dashboard,auth,can:update-roles,roles         |
|        | GET|HEAD  | backend/roles/{role}               | rinvex.fort.backend.roles.edit                     | Rinvex\Fort\Http\Controllers\Backend\RolesController@edit                                 | web,can:access-dashboard,auth,can:update-roles,roles         |
|        | GET|HEAD  | backend/users                      | rinvex.fort.backend.users.index                    | Rinvex\Fort\Http\Controllers\Backend\UsersController@index                                | web,can:access-dashboard,auth,can:list-users,users           |
|        | POST      | backend/users/create               | rinvex.fort.backend.users.store                    | Rinvex\Fort\Http\Controllers\Backend\UsersController@store                                | web,can:access-dashboard,auth,can:create-users,users         |
|        | GET|HEAD  | backend/users/create               | rinvex.fort.backend.users.create                   | Rinvex\Fort\Http\Controllers\Backend\UsersController@create                               | web,can:access-dashboard,auth,can:create-users,users         |
|        | DELETE    | backend/users/{user}               | rinvex.fort.backend.users.delete                   | Rinvex\Fort\Http\Controllers\Backend\UsersController@delete                               | web,can:access-dashboard,auth,can:delete-users,users         |
|        | PUT       | backend/users/{user}               | rinvex.fort.backend.users.update                   | Rinvex\Fort\Http\Controllers\Backend\UsersController@update                               | web,can:access-dashboard,auth,can:update-users,users         |
|        | GET|HEAD  | backend/users/{user}               | rinvex.fort.backend.users.edit                     | Rinvex\Fort\Http\Controllers\Backend\UsersController@edit                                 | web,can:access-dashboard,auth,can:update-users,users

We can use this as start point and extend the existing policy classes with our new functionality to show our forms

Alternative solution (first prototype?)

Based on "Make it work then make it better", we should create a prototype.
With this, i'm very sure, we find another way how we can implement this feature... anyway...

We extend our config file, where we can define which route needs some additional security check.

Example:

'security' => [
	'rinvex.fort.frontend.user.sessions' => 'password',
	'rinvex.fort.backend.*'				 => 'twofactor'
],

Then we're creating a new middleware (e.g. security) which will be used in the same way as the web middleware.
This middleware checks at first, if the current route is defined in the config.

If so, then we check a session variable (each route has it's own session).
If the session is valid, show the requested page.

If not, we have to redirect to our "enter password" / "enter twofactor" page.
If the confirmation was successful, we will call the original requested page again.


If you have another idea - feel free to post it :)

I hope my ideas are understandable and you can follow my thoughts.

Thanks a lot :)

@noxify
Copy link
Contributor Author

noxify commented Feb 19, 2017

I found some articles / forum posts which could be a start point for some prototypes:

I hope I can test it in the next days.

I will keep you informed :)

@noxify
Copy link
Contributor Author

noxify commented Feb 21, 2017

Gist for the first prototype (via middleware).

https://gist.github.com/noxify/90ffac363138e43bc330b74df6c9cfbc

In the next days I will try to finish this prototype.

@noxify
Copy link
Contributor Author

noxify commented Feb 21, 2017

While checking how it works in github i found the following POC for wordpress, which is exactly what we're trying to do here :)

https://github.com/trepmal/sudo-mode

@Omranic
Copy link
Member

Omranic commented Feb 22, 2017

Nice, cool progress for this POC.
Hope we can reach something more sensible than that WordPress example soon, so we can move on to implementation phase 😃

@noxify
Copy link
Contributor Author

noxify commented Feb 23, 2017

Hi everybody,

today I found some time to finish the first running prototype.
I have updated the gist (see link above).

It's currently a basic implementation without any language files and something has to be changed.
BUT .. It's working!

What do you think about it?

@noxify noxify changed the title Feature Request: Additional security checks for certain resources Feature Request: Additional security checks for certain resources ( aka websudo ) Feb 24, 2017
@noxify
Copy link
Contributor Author

noxify commented Feb 24, 2017

Open Todos:

Make it more secure

The current implementation with "checking that there is a session and the timestamp is valid" is ok for a mockup, but it's not really a good idea to secure a resource.

My idea was to use the persistences for this to check if the current session is valid or not.

Add 2FA Confirmation

Currently we're checking only that the entered password equals the current user password.
But my first idea was to use the 2FA (for me, it's more secure as a password check).

I have to check how can I call the existing functionality and understand how is it working.

Policy Implementation

Currently we're using in the prototype a new middleware to check everything.

To be honest, i have no clue if it's possible to add a parameter to the middleware to decide if the user has to enter a password or the app generated token.

I think with a policy like websudo:password or websudo:2fa we can solve the issue and using the existing implementation without creating two or more middleware to support different confirmations (password/2fa).

@noxify
Copy link
Contributor Author

noxify commented Feb 26, 2017

Hi,

the first running version with 2FA (tokenbased via app) is running.

Now it's also possible to define the session timeout per route and you can define if the session should be renewed if you're visiting it within the defined time.

You can also decide the confirmation type.

For password confirmation, you can use ->middleware(['websudo']) or ->middleware(['websudo:password']).
For 2FA confirmation, you can use ->middleware(['websudo:twofactor']).

This should complete the "Policy Implementation" part.

The "Make it more secure" part is obsolete (for me). Because Laravel handles the sessions in a secure way and I didn't saw any possibility to change the values via the browser developer tools.

What do you think?

@Omranic
Copy link
Member

Omranic commented Feb 26, 2017

Awesome, this is really moving forward.
Seems it will be one of the best features in this package 🥇

I'm looking at the code right now, ...

@Omranic
Copy link
Member

Omranic commented Feb 26, 2017

First of all, I really appreciate your efforts @noxify 😃

Well, looks pretty solid to move towards implementation phase.
Initially I'd say that the only thing I'll change in the above implementation is the config part, I prefer to define secured pages through route middleware instead of config option, and pass timeout & renewability through middleware parameters as well.

@noxify
Copy link
Contributor Author

noxify commented Feb 26, 2017

You mean something like:

->middleware(['websudo:password,60,true'])

  • 60 => Timeout
  • true => renew

The session name which is currently defined in the config could be replaced with the route name.

I will test it in the next days :)

Thanks for the new ideas.

@noxify
Copy link
Contributor Author

noxify commented Feb 26, 2017

The gist is updated and can be configured inside the routes definition.
The config file includes only the session prefix.

@Omranic
Copy link
Member

Omranic commented Jan 26, 2018

This is still one of my best feature discussions! I still like it & I'm now open for PRs and willing to merge it ASAP .. @noxify are you still interested in lending a helping hand here? 😃

@noxify
Copy link
Contributor Author

noxify commented Jan 26, 2018

Hi,

Yes for sure. Should we implement it as part of the rinvex/fort package or better as part of the cortex/fort package since there is no frontend in the rinvex/fort package anymore?

Maybe it‘s also an idea to create a new cortex module for this as additional package.

What do you think?
I can start the work ASAP with the existing snippets as startpoint (i think we have to change something but not everything)

@Omranic
Copy link
Member

Omranic commented Jan 26, 2018

Thank you in advance, appreciated 🙏
It fits cortex/fort very well as it's considered frontend feature anyway.

@noxify
Copy link
Contributor Author

noxify commented Jan 26, 2018

@Omranic - i have checked the existing packages and found something which could help to speed up the implementation.

What do you think about https://github.com/mpociot/reauthenticate ?

The difference between the idea and the package is just the "missing" 2FA verification.
And to be honest - i'm not sure if we need this really.
All sites (e.g. github) asking only for the current user password but never a 2FA token or something else.

Also I'm not sure about the enduser perspective if they have to search for their mobile phone to enter page X...

@Omranic
Copy link
Member

Omranic commented Jan 27, 2018

I reviewed the package, and it's a simple but still we need to modify it to add our own view, attach middleware automatically through service provider, and add config option for the timeout.

Accordingly I recommend implementing it on our own for a more clean feature :)

@Omranic
Copy link
Member

Omranic commented Feb 4, 2018

Merged!

@Omranic Omranic closed this as completed Feb 4, 2018
@noxify
Copy link
Contributor Author

noxify commented Feb 4, 2018

Awesome! Thank you so much

Omranic added a commit to rinvex/cortex-auth that referenced this issue Feb 11, 2018
* 'develop' of github.com:rinvex/cortex-fort:
  Clean ReauthenticationController
  Allow grouping reauthentication routes
  Clean & simplify Reauthenticate middleware
  Fix service provider conflict with recent update
  * revert type changes * removed config for session prefix to keep it simple
  Update config.php
  Implementation for rinvex/laravel-auth#84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants