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

Added a "Forgotten password" maker #359

Open
wants to merge 1 commit into
base: master
from

Conversation

@romaricdrigon
Copy link
Collaborator

commented Jan 31, 2019

Hello,

Having a password reset feature ("forgotten password") is a common feature of a web application. Maker bundle already has a User registration maker, it makes sense imo to add this too. Otherwise users are left either to code it manually (long), or to ditch Maker commands for FOSUserBundle.

I started implementing such a maker. It is heavily inspired from other Registration maker regarding style, and from FOSUserBundle process. Right now this is a work in progress, but I wanted to propose the idea & to discuss code style.

To do:

  • first working POC
  • add (minimal) view
  • add documentation describing the command
  • add tests - I got those are functional tests over the generated code?
  • squash & rebase everything

Points to be discussed:

  • is the controller right now looking good? I'm trying to have something minimalistic doing the job right, and easily modifiable. Not having too many services/helpers.
  • should the e-mail body be from a template, or does an heredoc looks sufficient to you?
  • which options would make sense for the command? for instance, redirecting or authenticating the user after having changed his password?
  • how to handle missing password setter/User class without an e-mail field? Display a warning in console?

Screenshots:

image

image

image

image

Thank you for this great bundle :)

@romaricdrigon romaricdrigon force-pushed the romaricdrigon:feat-forgotten-password branch 6 times, most recently from 593718a to 59805d7 Feb 1, 2019
@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 1, 2019

Views were added, and the POC if fully functional (manually tested).
From now on I will stop amending my first commit, if people want to review/comment code.
I'm considering making the "request password" form (first screenshot) work only by e-mail address, as recommended here.

@sstok

This comment has been minimized.

Copy link

commented Feb 1, 2019

I strongly suggest you read this 👍 https://paragonie.com/blog/2016/09/untangling-forget-me-knot-secure-account-recovery-made-simple

Almost all password reset systems are "horrible" insecure, tld;dr explanation: by using a single token (basically a random username) it's possible for an attack to guess the existing token by using a timing attack. When you provide a token, the database will use the index to look for an existing record, each character is checked against a binary-tree until it finds a record. When a character has a hit this means the time takes longer (because it continues the traversing of the tree), when there is no hit the time takes shorter because it bails out. Now using this technique an attack can use this information to try as many combinations as possible until all characters matches!

This is similar to a brute-force attack to guess a username/ID, but with a major difference. There is no password, and therefor access is directly granted.

By using the SplitToken you have selector (random username) and a verifier (random password in a hashed format), meaning that an attacker first needs to guess the selector, and then when there is a positive match, try to guess the verifier. BUT! Once you provide a wrong verifier for the selector, you simple revoke the token and no further tries are possible, even then guessing the verifier is nearly impossible as you compare the hashes in constant time (always takes the same amount of time) which leaks no timing information, making guessing nearly impossible.

I actually created a library that uses this technique https://github.com/rollerworks/split-token (MPLv2.0 licensed because it depends on HiddenString) but for most applications using the technique descried in the linked article is already a good step.

@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 1, 2019

@sstok thank you for the article, but I'm not sure implementing the suggestions from the article would be useful for most users The goal is to offer a basic feature, done right, for beginners. I'm pretty sure developers having tighter security requirements won't be using a maker command.
Hence I'm re-using a process from FOSUserBundle, which proved since a few years good enough for most, and follow some reasonable best practices (https://www.troyhunt.com/everything-you-ever-wanted-to-know/).

I like your approach, though. Did you consider make a "SplitTokenBundle"?

@gsylvestre

This comment has been minimized.

Copy link

commented Feb 7, 2019

This PR is a really good idea, and would be very usefull. I tried it and I love very much the generated code.

Some really small problems:

  • The generated twig files and folders do not follow coding stardards in their camelCased form (they should be snake_cased).
  • In the controller, method request(), is there any reason that you inject Twig_Environment instead of using the simpler approch of $this->renderView() ?
  • The suffix Action should be removed on controller's method checkEmailAction().

Also, I understand completely that you want to keep things simple and basic, and I agree. I just wonder if we could kinda incorporate some of @sstok suggestions by passing along the user email in the reset link. That would allow us to hash the reset token in the database, without adding much generated code and no other field in the entity. We even already use the UserPasswordEncoderInterface in the reset() method...

@gsylvestre

This comment has been minimized.

Copy link

commented Feb 7, 2019

In fact, maybe that those should be command options:

The reset password system need to store random tokens for your user. Do you want to store those:

  1. In your User entity
  2. In a separate entity

Would you like to hash the stored tokens (more secure)?

  1. Yes
  2. No

Would you like to add an extra "selector" field to protect against timed attack (more secure)?

  1. Yes
  2. No

Obviously, that is a lot of work to imlement, but that keep things simple for the end user.

@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 7, 2019

Thank you for the review @gsylvestre
I fixed the small problems, regarding \Twig_Environment at some point I wanted to render blocks independently, but I dropped this approach as it uses internal methods.

Regarding going further, I'm indecisive for now. I would like to get more feedbacks on this PR before going further regarding advanced features, about if and how it can integrates Maker bundle.

@sstok

This comment has been minimized.

Copy link

commented Feb 7, 2019

FYI. I don't have any plans for a bundle to integrate SplitToken, in practice you can register as many SplitToken factories as you want.

The only thing that might be interesting is a FormType to handle this format native, I actually did this for Park-Manager . But that's something for later as it's still experimental.

@weaverryan

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Sorry for the delay. I really want this - and it's on my list to review - but haven't had time yet. Thanks for getting it started!

@weaverryan

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@gsylvestre I was interested in getting more info from your above comments:

In fact, maybe that those should be command options:

The reset password system need to store random tokens for your user. Do you want to store those:

  1. In your User entity
  2. In a separate entity

I don't think we should do this - that's a level of customization that you can do if you want to, but I don't see any advantage to adding this flexibility (or is there a security implication?)

Would you like to hash the stored tokens (more secure)?

  1. Yes
  2. No

I saw your other comment before this, but can you explain this a bit more? If we only had 1 field in the database, how is the token hashed and how does that improve security?

Would you like to add an extra "selector" field to protect against timed attack (more secure)?

  1. Yes
  2. No

Maybe ;). I do think this is something to keep in mind as we get closer - it's probably not that much more work to implement, if indeed this is an industry-standard way of making more secure reset passwords.

@weaverryan

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Oh, I understand now about the hashed tokens: if we passed the email address in the reset URL, then we could look up the User by that email address, and then do a timing-safe check to see if the hash in the URL matches what we expect in the database.

Of course, then this also in theory allows for a timing attack on that "reset" URL, because you could use it to detect timing differences in return to determine if the email you entered is an email in the system :).

In general, I think we should be looking at other popular implementations (including FOSUserBundle, Laravel, Rails, whatever) and making sure we have the same safeguards at least as they do.

Copy link
Member

left a comment

Feedback started! I ran out of time to go through everything. But WOW! This is a crazy awesome and huge start! Let's work together and get this awesome command to the people :)

src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
@gsylvestre

This comment has been minimized.

Copy link

commented Mar 2, 2019

@weaverryan yes that's what I meant: if we hash the token in the database, we then need some kind of identifier to retrieve it. As you say, if we use the email, then an attacker can use timed attacks to find out if it's in the db... Really not a big concern imo, but still.

I was not proposing the "separate entity" option for security implication, not directly. But now that I have looked around and thought about it, I believe that we should always store the token in a new entity, instead of inside properties in the user class. See below...

Something like :

class PasswordResetToken 
{
    $id
    $selector //(crypto secure string, shorter)
    $token //(crypto secure string, longer)
    $requestedAt //(datetime)
    $user //(OneToOne unidir relation)
}

Pros:

  • We do not mess with User class by adding new properties and methods. It is often already clogged, and I feel it is intrusive.
  • We can then more freely add more fields and methods to our Token entity, like the generated "selector", or ie. if the developper want, he/she can add an expireAt field easily, or maybe use this entity for email confirmation purposes.
  • Indirectly more secure because we can now add the "selector" field
  • Until a password reset request is made, no token are generated. And no fields are sitting there being NULL and sad in the user table.
  • Better model structure, added flexibility

Cons:

  • Extra entity and database table are generated
  • more complicated?

In fact, is it really more complicated for the developper to understand what is going on? I believe even beginners will get this new entity easily.
And we are doing a really secure password reset. Why settle for less? We expect Symfony to provide top secure features.

BTW, Laravel is storing reset tokens in a new table, but use the email as the selector.

I can implement these changes next week, if the coding work is a problem.

@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 2, 2019

Thank you for your inputs both of you, and for your review @weaverryan, that's great to see things moving. Let's make this maker happen!

@gsylvestre I like that last idea you had, having a separate entity. It is not making things overly complex, but it both adds some security from what I got & code is a little bit cleaner in my opinion (it prevents User entity from growing too big...). I can work on it while fixing the other comments, but I would appreciate your review.

@sstok

This comment has been minimized.

Copy link

commented Mar 4, 2019

Of course, then this also in theory allows for a timing attack on that "reset" URL, because you could use it to detect timing differences in return to determine if the email you entered is an email in the system :).

See the linked article for full details ;) https://paragonie.com/blog/2016/09/untangling-forget-me-knot-secure-account-recovery-made-simple

The main focus is that you have two factors for authenticating, a selector (like a username, email address or random token) and a verifier that acts like a password (with the same security requirements for verifying). Using only a random value (token) as-is (as the FOSUserBundle is doing) makes that your token is nothing more then a random id that can be guessed, I can't make it more clear than this 👍

@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 5, 2019

I pushed commits regarding the first review comments.
On top of that, I prefixed both form classes names by Password, I find PasswordRequestFormType and PasswordResettingFormType to be more explicit.

Now I will work on having a separate entity, and then on testing everything.

@stof

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Note that if I were building FOSUserBundle today, I would probably use a separate table to store the reset tokens instead of putting things in the User class.

@sstok the issue of using the email address as the selector is that it allows using a timing attack to identify whether the email exists in the DB, allowing to leak the user base. That's why using a random selector instead is better if you try to protect the system against timing attacks.
Another option is to use the primary key of the user table...

@gsylvestre

This comment has been minimized.

Copy link

commented Mar 5, 2019

@stof, would you rather go with the random selector or the primary key?
I feel like the id would seem unsafe (for users and developpers to see the easily guessable user id in the url), even if it's mostly the same security level... It is one less field to generate on the other hand.

@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 7, 2019

I feel like having an extra "selector" field is not that bad, so for now I'm going with a selector separate from the ID.
One issue I'm having though: how to hash "token" without hardcoding a reference to the hash algorithm & without adding a dependency to an extra library?
For now, I'm considering getting the "UserPasswordEncoder", it feels a little bit hacky though.

@romaricdrigon romaricdrigon force-pushed the romaricdrigon:feat-forgotten-password branch 5 times, most recently from f8cffc9 to b2294a6 Mar 12, 2019
@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

Thanks for your offer, I've been kinda swamped last days due to personal reasons + flight issues which got me wandering in the airport instead of being at home :D

Right now, the biggest blocker is Travis failing. I don't understand why. And it's kinda a blocker. If you can help me with that, that would be immensely helpful!

@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

I opened another PR (#469), and it looks like Travis is just crashing on every MakerBundle build (even on master). So I will mostly ignore it :(

@Pierstoval

This comment has been minimized.

Copy link

commented Sep 5, 2019

Let's try to figure it out, I'll open a PR and work on that tonight 😃

@Pierstoval

This comment has been minimized.

Copy link

commented Sep 6, 2019

Well I spent a part of the night, and... All I could find was that the PHP commands take very very long to execute, so I think linking their output to PHPUnit's one could be a good start, and separate some tests in "groups" in order to execute phpunit once for each group could be a second start, but it might be more than that 😕

@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 6, 2019

Damn, that's complex :(
Functional tests take a very long time to start. I suspect loading "fixtures" to disk is too heavy or suboptimal.

@Pierstoval

This comment has been minimized.

Copy link

commented Sep 6, 2019

I investigated, it's not the fixtures, it's the maker commands. A symfony/skeleton project is created, and functional tests consists in copy/pasting the default skeleton, alter it a little for the sake of the test, and execute maker commands in order to check the generated output is the right one.

I wonder whether we could make this "enhanced unit tests" instead to make them a bit faster...

@weaverryan

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Yea, the tests are super slow... I wish we could make them faster (and maybe we can), but they're meant to be realistic - creating a fresh Flex project, running composer require on dependencies, and actually running the commands. Indeed, better output on failure might help debugging this stuff easier. Btw, I usually run ./vendor/bin/simple-phpunit --filter= and then the name of the test case in FunctionalTest that I specifically want to run.

Travis is just crashing on every MakerBundle build (even on master)

That's another issue (feature!) about the test suite. Because they're so realistic, if something changes upstream (e.g. a recipe change) it could break our tests. That's actually what we want... because it means that some assumption that MakerBundle was making might now be incorrect... but it also causes more frequent "sudden" test failures.

@Pierstoval

This comment has been minimized.

Copy link

commented Sep 17, 2019

A solution to make the tests faster would be to execute them in parallel instead. Since each test relies on a specific directory, it could be achieved by using the Process component or something similar. Implementation might be a bit complex, but to me it's probably the best option to make it faster.

@romaricdrigon romaricdrigon force-pushed the romaricdrigon:feat-forgotten-password branch from 4fa9886 to c989caf Sep 19, 2019
@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 19, 2019

About making tests faster, we could install a "full" Symfony app with Flex only at the beginning of the test suite, and then copy/reset it after each it. It should be faster than having as many composer require / Flex install as there are tests (those 2 take about 50% of test time, and a lot of memory).
Anyhow, that requires further investigation, and likely an issue/a PR to discuss it.

I'm resuming my work on this PR, and I will finish it this afternoon. Sorry for the long time it took!
Since I implemented several similar tokens systems, à la @sstok, I may also improve a little bit interfaces.

@romaricdrigon romaricdrigon force-pushed the romaricdrigon:feat-forgotten-password branch 13 times, most recently from 7fb97e0 to e01c353 Sep 19, 2019
@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2019

The PR is now good regarding feedback from @bocharsky-bw and @weaverryan !
I added a few tests regarding new features (retry TTL...), test are green on Travis / 7.3 and AppVeyor. I tested the Maker on a fresh Symfony project, I like the generated code, and I believe it will improve many applications :)

Also thank you @stof for your comments and @Pierstoval for your help.
There are still 2 comments open for discussion, which are either personal preference either to be done later I think.

@romaricdrigon romaricdrigon force-pushed the romaricdrigon:feat-forgotten-password branch from e01c353 to df78338 Sep 20, 2019
@Pierstoval

This comment has been minimized.

Copy link

commented Sep 20, 2019

🚢

😄

@romaricdrigon romaricdrigon force-pushed the romaricdrigon:feat-forgotten-password branch from df78338 to b57f3cf Sep 20, 2019
@romaricdrigon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 30, 2019

@bocharsky-bw and @weaverryan could you please check how I implemented modifications you requested?
I still hope it can get merged soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.