Replace bcrypt with bcryptjs #2053

OskarsPakers · 2019-12-06T20:36:20Z

It is a nightmare to run this project on Windows as it uses bcrypt -> node-gyp which requires Python and Windows build tools.

… Windows

changeset-bot · 2019-12-06T20:36:23Z

🦋 Changeset is good to go

Latest commit: 3eba7ab

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

emmatown

I think doing this is good because removing native Node modules is Good(since it’s annoying pretty much everywhere) and a nice side benefit is that this lets us deploy to environments that are not Node.

From https://github.com/kelektiv/node.bcrypt.js/wiki/bcrypt-vs-bcrypt.js, it seems like the JS version is ~30% slower which I think is acceptable though the JS version does everything on the main thread while the native version doesn’t block the main thread which isn’t ideal.

I’m a 👍 but someone who is more familiar with bcrypt/password things should review this as well

timleslie · 2019-12-08T21:34:49Z

@molomby I think you're best placed to review this and make an executive decision.

sarneaud · 2019-12-09T00:24:01Z

I like the "just works" feature of using bcryptjs. The 30% performance loss isn't such a big deal, but the fact that it blocks the main thread is going to be a problem for some sites.

It would be great if we supported both.

gautamsi · 2019-12-09T02:22:59Z

Usually the deployment to cloud is on linux based servers (cheaper than windows counterparts)

Dev environments on windows may use the instructions rather than the loss in performance.

Native modules are higher performance and usually non blocking. Nodejs is single threaded traditionally, we should avoid adding overhead. bcrypt is like standard used in this scenario.

We already have multiple overhead in process like ODM (knex/mongoose), graphql, keystone.

OskarsPakers · 2019-12-10T16:38:30Z

Maybe somebody should contribute to bcryptjs and make it not blocking the main thread if that is possible?

No doubt native modules are more performant.
Where ia the border line when NOT to use a native module?
Here the cost of this performance gain is that I (perhaps some more people) almost lost interest in keystone project.
Is it worth it or not?

gautamsi · 2019-12-10T17:19:43Z

I have two internal sites running on windows, no issue with that.

Just install windows build tools and run cmd with admin while installing dependencies or use developer mode in win10 to help you with windows setup.

MadeByMike · 2019-12-11T04:39:11Z

@OskarsPakers any reason for not using WSL. It's pretty much built-into Windows and makes these things a lot less painful. It's why WSL exists.

MadeByMike · 2019-12-11T04:39:36Z

I say this as a long time Windows user.

Vultraz · 2019-12-11T10:23:39Z

FTR, I'm also using Windows to run KS and I've had no issues (though granted, I do have a dev environment set up). I'm not using WSL.

OskarsPakers · 2019-12-13T22:36:46Z

Perhaps I was doing something really wrong, but that was a journey to the successful npm install.

So I did npm install

> [email protected] install C:\Users\o.pakers\Downloads\temidio\node_modules\bcrypt
> node-pre-gyp install --fallback-to-build

node-pre-gyp WARN Using needle for node-pre-gyp https download
node-pre-gyp WARN Tried to download(404): https://github.com/kelektiv/node.bcrypt.js/releases/download/v3.0.7/bcrypt_lib-v3.0.7-node-v72-win32-x64-unknown.tar.gz
node-pre-gyp WARN Pre-built binaries not found for [email protected] and [email protected] (node-v72 ABI, unknown) (falling back to source compile with node-gyp)

 Error: Could not find any Python installation to use

Found out windows build tools can help with Python (installed from elevated PowerShell)
npm i windows-build-tools -g
npm install again

C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\Common7\IDE\VC\VCTargets\Microsoft.Cpp.WindowsSDK.target
s(46,5): error MSB8036: The Windows SDK version 10.0.17763.0 was not found. Install the required version of Windows SDK
 or change the SDK version in the project property pages or by right-clicking the solution and selecting "Retarget solution". [C:\Users\o.pakers\Downloads\temidio\node_modules\bcrypt\build\bcrypt_lib.vcxproj]
gyp ERR! build error
gyp ERR! stack Error: `C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\MSBuild\15.0\Bin\MSBuild.exe` failed with exit code: 1

Found out that some build tools I had were not the with the correct version.
Installed Microsoft visual studio community 2017 with Workload "Desktop development with C++" https://www.visualstudio.com/thank-you-downloading-visual-studio/?sku=Community&rel=15 of size 1.58 GB

npm i

> [email protected] install C:\Users\o.pakers\Downloads\temidio\node_modules\bcrypt
> node-pre-gyp install --fallback-to-build

node-pre-gyp WARN Using needle for node-pre-gyp https download
node-pre-gyp WARN Tried to download(404): https://github.com/kelektiv/node.bcrypt.js/releases/download/v3.0.7/bcrypt_lib-v3.0.7-node-v72-win32-x64-unknown.tar.gz
node-pre-gyp WARN Pre-built binaries not found for [email protected] and [email protected] (node-v72 ABI, unknown) (falling back to source compile with node-gyp)
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  blowfish.cc
  bcrypt.cc
  bcrypt_node.cc
  win_delay_load_hook.cc
     Creating library C:\Users\o.pakers\Downloads\temidio\node_modules\bcrypt\build\Release\bcrypt_lib.lib and object C:\Users\o.pakers\Downloads\temidio\node_modules\bcrypt\build\Release\bcrypt_lib.exp
  bcrypt_lib.vcxproj -> C:\Users\o.pakers\Downloads\temidio\node_modules\bcrypt\build\Release\\bcrypt_lib.node
  Copying C:\Users\o.pakers\Downloads\temidio\node_modules\bcrypt\build\Release\/bcrypt_lib.node to C:/Users/o.pakers/Downloads/temidio/node_modules/bcrypt/lib/binding\bcrypt_lib.node
          1 file(s) copied.

Success!

And I am not arguing that it is not possible to install keystone on Windows, but this is far from a simple npm install. And as bcrypt is the problem, switching to bcryptjs would make it easier.

@MadeByMike I actually have subsystem for Linux and I use it, however, not all of my colleagues have it. Also switching to Linux is just running away from the problem.

molomby

I think this is a good idea.

Pro: Native JS
This is a real win. I've seen a stack of issues with node-gyp over the years, not just on Windows but also with differences between MacOS dev and Linux production environments.

Con: Performance
The ~30% performance penalty (more like 0-20% in my testing) is a bit of a red herring. It shouldn't really contribute to production load due to the tune-ability of the workFactor but it does reduce the security of the hashes in that, for a given workFactor, brute-forcing a hash will be easier. But.. with a decent work factor and password complexity, that amount of compute needed to brute-force a hash is millions of CPU years. On that kind of scale we're worried about orders of magnitude, not double digit percentages.

Con: Thread Blocking
This is a problem, especially for a function that, by design, will usually take 100-1000 milliseconds to run. Remember, this penalty is paid both when generating and verifying hashes, making it even easier to (intentionally or otherwise) DoS a site by hammering on an auth mutation. It's also pretty hard currently rate limit auth mutations, exacerbating this risk (but we're working on it!).

So.. the ease of the native JS solution is great for small apps and better developer experience but, for larger or particularly security conscious sites, the compiled version is better. @sarneaud sums it up:

It would be great if we supported both.

Let's do that ☝🏼

These commits should be kept but let's also add a useCompliedBcrypt option to the Password field which reverts to requiring bcrypt. Eg..

keystone.createList('User', {
  fields: {
    email: { type: Text },
    password: { type: Password, useCompliedBcrypt: true },
    // ...
  },
});

The bcrypt package doesn't need to be included in the package.json; it can still be required if installed manually by the app developer. The docs would also need to be updated to cover the new option.

I think this makes sense and gives us the best of both worlds.. Does anyone else have thoughts on this proposal?

timleslie · 2020-03-15T22:26:20Z

Another issue with using bcrypt arises when trying to deploy to an AWS lambda. Deployment to a lambda requires building the node_modules directory locally and then supplying it to the lambda function to use. If the build is done on OSX and the lambda is deployed on a linux system then the bcrypt package will not work, as OSX uses a different binary file format to linux.

timleslie · 2020-03-16T00:09:58Z

Closed by #2523

OskarsPakers added 3 commits December 6, 2019 22:16

[master]: Switch to bcryptjs

f9b3db5

[master]: Switch to bcryptjs to avoid issues on packaging node-gyp on…

088dc9c

… Windows

[master]: Switch to bcryptjs to avoid issues on packaging node-gyp on…

45519c3

… Windows

OskarsPakers changed the title ~~Replce bcrypt with bcryptjs~~ Repalce bcrypt with bcryptjs Dec 6, 2019

OskarsPakers changed the title ~~Repalce bcrypt with bcryptjs~~ Replace bcrypt with bcryptjs Dec 6, 2019

MadeByMike requested review from emmatown, molomby and jesstelford December 8, 2019 21:02

emmatown reviewed Dec 8, 2019

View reviewed changes

timleslie assigned molomby Dec 8, 2019

Merge branch 'master' into master

3eba7ab

molomby requested changes Dec 19, 2019

View reviewed changes

AllanFerencz mentioned this pull request Jan 15, 2020

Add FAQ for windows users failed install #2239

Closed

molomby mentioned this pull request Jan 29, 2020

Password auth strategy should check a hash exists before comparing #2301

Closed

timleslie added the merge conflict label Feb 24, 2020

timleslie mentioned this pull request Mar 15, 2020

Add support for conditional bcrypt/bcryptjs usage #2523

Merged

timleslie closed this Mar 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace bcrypt with bcryptjs #2053

Replace bcrypt with bcryptjs #2053

OskarsPakers commented Dec 6, 2019

changeset-bot bot commented Dec 6, 2019 •

edited

Loading

emmatown left a comment

timleslie commented Dec 8, 2019

sarneaud commented Dec 9, 2019

gautamsi commented Dec 9, 2019 •

edited

Loading

OskarsPakers commented Dec 10, 2019 •

edited

Loading

gautamsi commented Dec 10, 2019

MadeByMike commented Dec 11, 2019

MadeByMike commented Dec 11, 2019

Vultraz commented Dec 11, 2019

OskarsPakers commented Dec 13, 2019

molomby left a comment

timleslie commented Mar 15, 2020

timleslie commented Mar 16, 2020

Replace bcrypt with bcryptjs #2053

Replace bcrypt with bcryptjs #2053

Conversation

OskarsPakers commented Dec 6, 2019

changeset-bot bot commented Dec 6, 2019 • edited Loading

🦋 Changeset is good to go

emmatown left a comment

Choose a reason for hiding this comment

timleslie commented Dec 8, 2019

sarneaud commented Dec 9, 2019

gautamsi commented Dec 9, 2019 • edited Loading

OskarsPakers commented Dec 10, 2019 • edited Loading

gautamsi commented Dec 10, 2019

MadeByMike commented Dec 11, 2019

MadeByMike commented Dec 11, 2019

Vultraz commented Dec 11, 2019

OskarsPakers commented Dec 13, 2019

molomby left a comment

Choose a reason for hiding this comment

timleslie commented Mar 15, 2020

timleslie commented Mar 16, 2020

changeset-bot bot commented Dec 6, 2019 •

edited

Loading

gautamsi commented Dec 9, 2019 •

edited

Loading

OskarsPakers commented Dec 10, 2019 •

edited

Loading