search

jembi / openhim-core-js

The Open Health Information Mediator core component. OpenHIM Support: Post your query on OpenHIE Discourse using the #openhim tag https://discourse.ohie.org/
http://openhim.org
Mozilla Public License 2.0
70 stars 70 forks source link

Passwords for BasicAuth should be stored as salted hash #27

Closed HeinrichFilter closed 10 years ago

HeinrichFilter commented 10 years ago

A hashing function designed for password (e.g. bcrypt) should be used and not a general hashing function like MD5.

rcrichton commented 10 years ago

When an application is saved if they have a password it should be hashed using bcrypt or scrypt before it is stored. Also when passwords are being matched during authentication it should hash the incoming password and compare it to the stored hash. If the hashes match then the application is authenticated.

Scrypt seems to be the up-and-coming next best password hashing function, perhaps we should make use of it. Here is a node module that allows enables the use of scrypt: https://www.npmjs.org/package/scrypt

hnnesv commented 10 years ago

So after some discussion we've settled on supporting both bcrypt and SHA hashing.

Although bcrypt is far better for password use, not all implementers may be happy with the performance hit that comes with using it. A minimum workload added a bump of ~300ms (possibly blocking) to the transaction time during testing.

Note that we'll also have to use the bcrypt-nodejs module as the better maintained C++ driver node.bcrypt.js doesn't support Node 0.11.x yet. Tests using this module added ~100ms minimum using a low work factor.

By supporting both options, we'll give implementers the choice: either slow/secure bcrypt or quick/not-as-secure SHA.

hnnesv commented 10 years ago

I ended up going a bit further than just SHA, and added support for any algorithm in the standard crypto library: http://nodejs.org/api/crypto.html, since it's simple enough to support. Implementers can then just pick the algo they want.