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

Move to bcryptjs #112

Closed
daffl opened this Issue Mar 22, 2016 · 9 comments

Comments

Projects
None yet
4 participants
@daffl
Copy link
Member

daffl commented Mar 22, 2016

There is a pure js version of bcrypt that we might want to try out because compiling bcrypt is causing a lot of issues (see feathersjs/feathers-chat#1 and feathersjs/feathers#273 for example).

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Mar 22, 2016

Yeah. It can really be a huge pain to get bcrypt to build. I ran into that with Travis. Let's just be sure to call it out in the readme of the modules that use it. If bcrypt being 2.7x slower means they have to use 3x more machines for scaling then they might want to know about other options.

@daffl

This comment has been minimized.

Copy link
Member Author

daffl commented Mar 22, 2016

Ah yes, that's another specific problem for bcrypt, it only runs on newer versions of GCC that many Linux distributions don't have.

The performance to compare passwords might be interesting. But usually you only do that once to create a new user and to get a token which probably doesn't happen hundreds of times a second.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Mar 22, 2016

Yeah. Seems like now It will only be a few weeks before somebody shows us. :)

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 22, 2016

I'm less concerned with the performance but more the memory overhead. The bcrypt algorithm is fixed to a certain amount of time to compare passwords in order to prevent timing attacks and you don't compare passwords enough for performance to really have an impact. We should check to see if switching to the JS one doesn't raise memory a bunch.

Sorry I'm being a stickler but IMHO this a pretty low priority given everything else we have on the go. I don't want to monkey with a bunch of the auth stuff until we have good test coverage in place.

I also think, as a general rule, that just because a couple people have issues doesn't mean we should change something for everyone. We've only had 2 issues created and both were from people that are new to node and on windows.

@daffl

This comment has been minimized.

Copy link
Member Author

daffl commented Mar 22, 2016

I think it is. Besides those two issues it came up in other conversations before and it does make things much harder to use in Windows.

I already spent at least hour myself trying to figure out why my Travis CI tests didn't pass (it didn't come with the right version of GCC to compile bcrypt) and anybody who is using Travis (or more conservative Linux distributions) for their projects will also run into the same problem.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Mar 22, 2016

We ought to add a section to the FAQ docs on Working with Travis

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 22, 2016

@marshallswain probably not our concern there really. We only use Travis for plugins and I think the generator generally does a good job of scaffolding out stuff. We can always help people if they want to transfer repos to us.

@daffl alright, feel free to add it after I get #109 merged later today. Personally, I'd like to have at least a rough idea of what the CPU/Memory effects of switching are going to be given how core this module is turning out to be, but if you think it's top priority then shrug.

Like I said, it is not a very high priority for me.

@beeplin

This comment has been minimized.

Copy link

beeplin commented Mar 22, 2016

I would say that as a windows user it is reeeeeally painful to install the whole bunch of visual studio thing (around 9G) just for get a convenient c++ compiler to build feathers-authentication. Would be nice if this got solved.

来自 魅族 MX5

-------- 原始邮件 --------
发件人:Eric Kryski notifications@github.com
时间:周三 3月23日 00:19
收件人:feathersjs/feathers-authentication feathers-authentication@noreply.github.com
主题:Re: [feathers-authentication] Move to bcryptjs (#112)

@marshallswain probably not our concern there really. We only use Travis for plugins and I think the generator generally does a good job of scaffolding out stuff. We can always help people if they want to transfer repos to us. @daffl alright, feel free to add it after I get #109 merged later today. Personally, I'd like to have at least a rough idea of what the CPU/Memory effects of switching are going to be given how core this module is turning out to be, but if you think it's top priority then shrug. Like I said, it is not a very high priority for me. —You are receiving this because you are subscribed to this thread.Reply to this email directly or view it on GitHub

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 28, 2016

Closed by #137.

@ekryski ekryski closed this Mar 28, 2016

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.