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

Replace bcrypt with bcryptjs #3

Closed
wants to merge 2 commits into from
Closed

Replace bcrypt with bcryptjs #3

wants to merge 2 commits into from

Conversation

nwinch
Copy link
Contributor

@nwinch nwinch commented May 15, 2016

This branch resolves #2

Pass tests both locally and on Travis. Let's roll with it!

@davidbanham
Copy link
Contributor

Doing bcrypt in JS rather than C is likely going to be a pretty major performance hit. Are there benchmarks between the two libs?

Bcrypt does have some extra deps like gyp, but that's been a standard since like 0.8 at least. Any sane node environment should be able to build it fine.

What issues have we seen?

@nwinch
Copy link
Contributor Author

nwinch commented May 15, 2016

While bcrypt.js is compatible to the C++ bcrypt binding, it is written in pure JavaScript and thus slower (about 2.7 times), effectively reducing the number of iterations that can be processed in an equal time span.

Yep, it's slower. How much of a practical difference would that make to us using it?

What issues have we seen?

https://travis-ci.org/Prismatik/simple-password/builds/128781147

And other's from Travis I remember from a while back. @s-taylor Also reported bcrypt related issues with Docker.

@s-taylor
Copy link

s-taylor commented May 16, 2016

The error i'm getting with bcrypt on docker is Error: /opt/app/node_modules/bcrypt/build/Release/bcrypt_lib.node: invalid ELF header
It's only occuring if i've scaffolded a users controller with redbeard which is because that's when becrypt is used via simple-password. Error does not occur when not using docker.

@s-taylor
Copy link

s-taylor commented May 16, 2016

So from what i can ascertain, bcrypt does not work on any node version > 4.2. It's definitely failing on 4.4.4 for me.

@jacklynrose
Copy link

jacklynrose commented May 18, 2016

So though speed is a concern for me, security is actually higher on my list. Coping with the idea of bindings to a security library potentially having issues is something you just take as a given, but to actually replace the implementation of the library opens up attack vectors that won't be seen in the heavily scrutinised official C++ library.

There is definitely people using this on all versions of node up to v6 from what I've found, just need to fix the error now. If it's to do with docker you very likely haven't got a system dependency installed or if you're uploading the node_modules folder you're uploading the bcrypt binaries which have been built for OS X, not whatever linux distro is running in your container, in which case add npm rebuild to your Dockerfile or have it install from scratch.

@nwinch
Copy link
Contributor Author

nwinch commented Aug 25, 2017

Closing this, as, err, it probably won't be used.

@nwinch nwinch closed this Aug 25, 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

Successfully merging this pull request may close these issues.

Change bcrypt libs
4 participants