[hotfix] Fixes db seed password bug

[mleanos]

Fixes the database seeding bug with the password not passing the owasp
test. Related to #908 & #909

Adds a UserSchema method that generates a random password that passes
the owasp test.

Performed minor refactoring of the database seed configuration to
implement the new UserSchema method.

[lirantal]

I'm not too fond of the brute forcing there.
If we have a package for testing owasp compatibility then we can have one to generate them too.

I suggest we look into a solid password generating package like https://www.npmjs.com/package/password-generator and work with that.

[mleanos]

@lirantal That's a very good point. I'll integrate a password generator package.

Are you fine with the addition of the model instance method for generating the password? I thought that would be a nice thing to have, and it's more testable in that fashion.

[lirantal]

Sure, that's ok.

[mleanos]

@lirantal I made some changes, but it was quite difficult to avoid the forced password generation. It's less brute now. I'm not sure how else we would go about randomly generating a password, given the fact that it must adhere to the strict OWASP guidelines.

I couldn't find a suitable npm package that would generate a password, given our password requirements. I tried using the package you suggested, since it supports regular expressions. However, our requirements proved too complex for the limitations of that package.

See this issue I submitted to the project.. bermi/password-generator#10
I ended up using a simplified version of the suggested workaround.

I'm open to suggestions if this doesn't seem like a viable solution. I've also added model tests, and have done some stress tests locally. Running millions of iterations of the password generation function. It seems fast, and reliable. Returning a promise really helps in cases like these.

[jloveland]

OWASP strength test accepts a passphrase as an alternative. What do you think about generating a random passphrase using multiple memorable passwords? https://www.npmjs.com/package/password **

** the password package was recommended by the diceware package https://www.npmjs.com/package/diceware to generate passphrases on node.js

[mleanos]

@jloveland I like that idea. I didn't realize that about the owasp strength test package. I just tested this with the Change Password feature. It worked beautifully.

The password generator package will generate a readable password as well. We could combine a few to create the passphrase. I'm going to go with 4 readable passwords. I noticed the owasp test will accepts a passphrase with spaces, and no spaces. I say we generate one without spaces. Does that sounds good to you?

Thanks for the feedback. No more brutish force :)

[mleanos]

@jloveland I went with the password package you recommended. I've got it working quite nicely. However, I'm concerned about the requirement for the owasp passphrase to be at least 20 characters. I ran some tests that showed me that it's possible to have 4 word passphrases that are less than 20 characters; even with the spaces. (Which I'm thinking now that we should keep).

We could modify the configuration for owasp to decrease the minimum length of a passphrase. https://github.com/nowsecure/owasp-password-strength-test#configuring

One problem is that I've seen one letter words generated by the password package. Could add to the issue.

Any ideas?

[lirantal]

I'm not very impressed by https://www.npmjs.com/package/password, let's continue re-thinking this to see if we can come up with something solid.

[lirantal]

@mleanos https://github.com/brendanashworth/generate-password doesn't seem to have a lot of npm installs but it looks like it's supporting our use case and quite simple. Can you please take a look and integrate? I want to move on with this PR so I can continue working on tests for the SeedDB feature.

[mleanos]

@lirantal Good find. Thank you for helping out. I'll integrate this today.

[jloveland]

good suggestion, looks like generate-password uses crypto which is better than Math.random

[mleanos]

@lirantal @jloveland I've tested this, and it appears that this package (in it's current state) won't work for our use case.

Either the options of the generate method are misleading, or there is a flaw in the implementation. I ran some tests, and the passwords generated aren't always meeting the requirements; even when setting all the options to true.

If you examine this line, you'll notice that the available characters (pool) are correctly set given the provided options. However, it randomly choose from that collection of characters based on the length provided. It's not requiring that each of the options provided are actually included in the password.
https://github.com/brendanashworth/generate-password/blob/master/src/generate.js#L48

This package was last updated about 9 months ago, but it's so close to what we need. Perhaps, it's a good idea to fork generate-password & extend it.

[mleanos]

Update: I'm currently working with the owner of the generate-password project, to see if we can come up with a viable solution that would fit our needs.

See this for reference: brendanashworth/generate-password#4

More eyes on this would be much appreciated.

[mleanos]

@lirantal @jloveland Can you review this approach?

So after looking at the OWASP strength test requirements again, I realized that when a password is 20 or more characters it is considered a passphrase. Passphrases are not subject to the same scrutiny as passwords. Only the required tests are ran against a passphrase.

See here for the owasp strength test guidelines: https://github.com/nowsecure/owasp-password-strength-test#features

We can utilize this passphrase feature, by ensuring that we generate a passphrase of 20 or more characters. The only condition we must account for is the possibility of 3 or more repeating characters being generated. To handle this limitation, I am checking for 3 or more repeated characters, and removing them from the generated passphrase.

See here: https://github.com/meanjs/mean/pull/921/files#diff-240057bbb9109acf3b1745506df4b781R192

I think this is as clean & simple as we'll get, given the lack of available npm packages out there that work for our use case.

I have submitted a PR to the generate-password package, to add an option for a repeating characters limit. I like this package, as it's simple and the implementation is very solid. If this is added to the package, then we no longer have to handle the repeat characters issue on our end.
brendanashworth/generate-password#5

[mleanos]

Just found an issue with this.. don't merge.

[mleanos]

I was running some stress tests against the generateRandomPassphrase method, and realized that it was possible to send back an invalid passphrase.

The case was where the 3 consecutive repeating characters were removed, and the result ended up having a new occurrence of 3 consecutive repeating characters...

Example:
"f3dirssuuus48cDd343Dspqw" will become "f3dirsss48cDd343Dspqw" after uuu is removed.

To solve this issue, I added a new condition to the while to test for the occurrence of the repeating characters.
https://github.com/meanjs/mean/pull/921/files#diff-240057bbb9109acf3b1745506df4b781R182

[lirantal]

@mleanos how long do you think this is going to take because right now we indeed have a bug on the seed functionality in master. I'm contemplating whether to revert back to your original brute forcing loop with crypto until we find a more proper solution ( @ilanbiala @codydaig WDYT?)

[mleanos]

@lirantal Do you not prefer my current approach over the brute force that I had before?

I think what I have now is a pretty good solution, given the limitations of the supporting packages out there. I'm open to suggestions. However, this may be as close to a gentle solution as we can get due to our strict requirements.

The last option, is to manually set the passwords in the seed configuration; using this here https://github.com/Gym/mean/blob/master/config/lib/seed.js#L12

Lastly, I've ran this current implementation through tens of millions of iterations (stress tests), and haven't encountered any issues. This is fast, and reliable.

[lirantal]

I'm ok with that too, but just need to keep in mind that this needs to be refactored later on.

[mleanos]

@lirantal I agree, about the refactoring. With this approach, we can easily implement a npm package that suits our needs; once we find one that can handle our use case.

There's just really no way to handle the strict requirements for our passwords, other than to handle it in this way. On the surface, it may not seem like a clean approach. However, at least we can offload the bulk of the password generation to the generate-password package for now.

[codydaig]

LGTM. @lirantal Do you plan to do the refactor with the rest of your refactor?

[lirantal]

I am not sure @codydaig
I wanted to refactor it to to begin with about the test-ability of it, although @mleanos really changed how it looks like so I'll have more work to do now. I want to merge this PR quickly though so I can get down to business there ASAP (because of the bug that is happening now in production), and before the code grows too much there.

[mleanos]

@lirantal If you'd like, I can revert the changes in the seed configuration to how they were before; changing these lines to just use the new Schema method..
https://github.com/meanjs/mean/blob/master/config/lib/seed.js#L38
https://github.com/meanjs/mean/blob/master/config/lib/seed.js#L56
https://github.com/meanjs/mean/blob/master/config/lib/seed.js#L72

I probably shouldn't have changed that file so much. I just thought it would be easier to test; and I didn't know what your refactors looked like.

[mleanos]

@lirantal I submitted a new PR, that should make things much simpler for you. You can either choose this one, or #931. I didn't mean to complicate things for you by changing the db seed configuration so much.

[lirantal]

@ilanbiala I really want to continue with this already to add tests. Do you mind the native promise approach?

[codydaig]

@lirantal @mleanos I've looked at this PR as well as #931. I like them both. @lirantal It's up too you which one is going to make your refactor easier. Whichever that one is, go ahead and merge it.

[ilanbiala]

@lirantal native is fine.

[lirantal]

I'll go ahead and merge this one, if I'll need some help refactoring it for tests I'm sure you guys will be happy to help :)

Thanks!

[mleanos]

@lirantal Thank you. I will be sure to help, if you need it.