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

Better example of how to change hashing algorithm? [Question] #289

Closed
jheysen opened this Issue Sep 11, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@jheysen
Copy link

jheysen commented Sep 11, 2016

I've been trying to change the bcrypt that feathers-authentication uses from bcryptjs to bcrypt due to scalability issues. I did try the method discussed on #146 declaring a bcrypt param on feathers-authentication config that matches the export of bcrypt module, i.e:

var bcryptC = require('bcrypt');

var app = this;
app.configure(authentication(app.get('auth')));

And my config.json

"auth": {
    "idField": "id",
    "token": {
      "secret": "SOMESECRET"
    },
    "local": {},
    "bcrypt": "bcryptC"
}

The application, however, crashes on startup when I do this telling that crypto.genSalt is not a function.
I have added bcrypt library (https://www.npmjs.com/package/bcrypt) to my dependencies.
What am I doing wrong?
For reference, I need to do this since logging in a user using local strategy or reistering a new one takes more than 1 sec.

@jheysen jheysen changed the title Better example of how o change hashing algorithm? Better example of how o change hashing algorithm? [Question] Sep 11, 2016

@jheysen jheysen changed the title Better example of how o change hashing algorithm? [Question] Better example of how to change hashing algorithm? [Question] Sep 13, 2016

@kaiquewdev

This comment has been minimized.

Copy link

kaiquewdev commented Sep 15, 2016

You have a detailed report about the failure?

@jheysen

This comment has been minimized.

Copy link
Author

jheysen commented Sep 19, 2016

Sorry about the delay. Sadly nope, since my app was runing in Docker.
We solved this by forking the library code, changing there the dependency from bcryptjs to bcrypt, transpiling and uploading to our fork, then redirecting our app's package.json to the fork for Feathers-Authentication.
If you are curious, you'll see various reports of bcryptjs taking too long on production servers. In our case making the switch from bcryptjs to bcrypt speeded up login times from 2000ms to 150ms.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Sep 19, 2016

Crazy, I didn't think it would be that bad. We moved to bcryptJS because the authentication module installation was failing in many different environment due to the compilation step.

You should also be able to replace the hashing algorithm by passing the module reference into the configuration though:

const authConfig = Object.assign({}, app.get('auth'), {
  bcrypt: require('bcrypt')
});

app.configure(authentication(authConfig));
@jheysen

This comment has been minimized.

Copy link
Author

jheysen commented Sep 20, 2016

I noticed that the hook uses bcryptjs in a hardcoded way though :p

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Sep 20, 2016

Ah that is true. That should definitely be fixed but it is at least a little easier to add as your own hook using BCrypt than having to fork the entire library.

@ekryski ekryski modified the milestone: 1.0 Nov 21, 2016

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Dec 30, 2016

This is now possible and much more flexible with auth v1.x and it's corresponding auth providers.

You can now simply extend the verifier for feathers-authentication-local (or any other auth provider) and implementing your own _comparePassword function that uses any hashing function you would like.

You will also need to pass your own hash function as an option to the hashPassword hook.

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