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 support for bcrypt #22

Closed
2 tasks done
j3k0 opened this issue Apr 21, 2017 · 10 comments
Closed
2 tasks done

Add support for bcrypt #22

j3k0 opened this issue Apr 21, 2017 · 10 comments
Assignees

Comments

@j3k0
Copy link
Owner

j3k0 commented Apr 21, 2017

As we're migrating from stormpath, we'll retrieve encrypted bcrypt passwords.

We need to:

  • set password from the bcrypt string (not from a plain text)
    • suggestion: add hashedPassword data in body as an alternative to password when creating/editing accounts.
  • detect that the password is bcrypt-ed or not, use the right password validation method.

Ref: https://stormpath.com/export#use-pw

@j3k0
Copy link
Owner Author

j3k0 commented Apr 21, 2017

(for now, I'm assuming we only have bcrypt-ed password, no "stormpath hash" -- which they say is unlikely)

@j3k0
Copy link
Owner Author

j3k0 commented Apr 21, 2017

@elmigranto urgent, so take this task only if you have time today or tomorrow. Otherwise I'll handle it. Please let me know.

@j3k0 j3k0 added the ready label Apr 21, 2017
@elmigranto
Copy link
Collaborator

Will work on this today.

@elmigranto
Copy link
Collaborator

elmigranto commented Apr 21, 2017

Took a look, here's the scope with bit more specifics:

  • LoginsUsers#login() should be aware of pbkdf2 vs bcrypt;
  • CreatesUsers#create() when accepting something that looks like bcrypt should react appropriately[1];
  • ChangesPasswords#change() is not affected — user is already logged in at that point, and password received is plain text which we can hash however we want.

[1] What if someones password starts with $2, guess we write more specific regex; or probably would be better to use your suggestions. Given how we can only receive bcrypt-hash when importing old stuff (./bin/import-json), that should work. So, stuff to do:

  • LoginsUsers#login() runs different verification depending on hash type;
  • LoginsUsers#hashPassword() passes already hashed passwords as is;
  • CreatesUsers#create() accepts either password prop or passwordHash prop; in former case it hashes itself, in latter one it just saves hash as is into DB;
  • ./bin/import-json should take into account password hash already present in JSON exported from Stormpath;
  • directory.router should try to either pull password or passwordHash from req.body.

Let me know if any of this sounds off to you.

@elmigranto
Copy link
Collaborator

Correct me if I'm wrong, password hash will only be on users ported using import-json? For now it sets account password to random hex string, but will actually use w/ever there is in Stormpath exported JSON (see #3).

What's the format of that with password hash included?

@elmigranto
Copy link
Collaborator

If that's the case, I wonder whether it would be easier to just replace password's hash directly in DB after creating a user (from import script that is): we assemble payload, remember user ID, send request, and replace db doc with hash field changed to bcrypt hash.

@j3k0
Copy link
Owner Author

j3k0 commented Apr 21, 2017

Yes it's only meant to be used with import-json, we can add support for a hash field in the input json document with a hashed password.

Also, to make detectVerifier more robust, we can prefix our hash with bcrypt (so the test becomes hash.startsWith('bcrypt$'). Seems useful?

I'd say, let's go for the easier. Maybe it's also simple to test the current plaintext password for a condition like password.length > 16 && detectVerifier(password), then consider it already hashed.

@elmigranto
Copy link
Collaborator

elmigranto commented Apr 21, 2017

hash.startsWith('bcrypt$')

I'd rather not, let's keep w/ever spec/lib does.

password.length > 16 && detectVerifier(password)

Well, looks like the simplest would be to make LoginsUsers#hashPassword() detect if password passed is hash already and return that. Though, here, we should make stricter check than just startsWith, true, — some regex or something.

@j3k0
Copy link
Owner Author

j3k0 commented Apr 21, 2017

the simplest would be to make LoginsUsers#hashPassword() detect if password passed is hash already

Sounds good. startsWith $2 might not be a condition good enough...

From https://en.wikipedia.org/wiki/Bcrypt :

The prefix "$2a$" or "$2b$" (or "$2y$") in a hash string in a shadow password file indicates that hash string is a bcrypt hash in modular crypt format.[4] The rest of the hash string includes the cost parameter, a 128-bit salt (base-64 encoded as 22 characters), and 184 bits of the resulting hash value (base-64 encoded as 31 characters)

@elmigranto
Copy link
Collaborator

It will be good enough to detect what kind of hash passed in when verifying plain text password against it (LoginsUsers#login()). It wouldn't be good enough to detect whether we have plain text password vs hash (LoginsUsers#hashPassword()). Though, I guess, we can as well make both more robust.

@j3k0 j3k0 closed this as completed May 15, 2017
@j3k0 j3k0 removed the ready label May 15, 2017
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