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

Support local signin #36

Closed
jackycute opened this issue Oct 4, 2015 · 20 comments
Closed

Support local signin #36

jackycute opened this issue Oct 4, 2015 · 20 comments
Labels
feature Wants to add a new feature
Milestone

Comments

@jackycute
Copy link
Member

There might be some users not using any services we used to do a third-party auth.
So we need a local signin solution, currently we're using passport as the auth middleware.
Then using passport-local to support this will be the best.

@jackycute jackycute added this to the 0.3.4 milestone Oct 5, 2015
@jackycute jackycute added the feature Wants to add a new feature label Oct 5, 2015
@jackycute jackycute removed this from the 0.3.4 milestone Nov 29, 2015
@ccoenen
Copy link
Contributor

ccoenen commented Oct 15, 2016

I would like to request putting this back onto an agenda (i.e. assign a milestone to it). I believe this to be of major importance for a self-hosted tool.

@jackycute
Copy link
Member Author

OK, I would like to add this back in track, but only for self-hosted usage.

Because holding password is a high risky and complex behavior, official service won't enable this feature.

@jackycute jackycute added this to the 0.4.6 milestone Oct 16, 2016
@ccoenen
Copy link
Contributor

ccoenen commented Oct 16, 2016

"self hosted only" is fine with me, that's exactly my use-case :)

Can I somehow help? Ideally you'd store not the password but a bcrypt (more widely known) or scrypt (more recent, supposed to be stronger) hash. That way, If anything did happen to the database, there's very little value in the password hashes themselves.

@jackycute
Copy link
Member Author

We use passport.js to auth our users.
So this package will be needed to register and login https://github.com/jaredhanson/passport-local
Also need some UI for login and config to enable/disable this feature.

@ccoenen
Copy link
Contributor

ccoenen commented Oct 16, 2016

I'm trying to find documentation on how they store passwords, their bcrypt examples seem to have vanished. At the moment it looks to me as if they'd actually store them in plain text which would be extremely bad. But I might well be wrong, given that I've never used their code before.

@jackycute
Copy link
Member Author

jackycute commented Oct 16, 2016

I found that too, plaintext won't be acceptable.
But I also find few examples using bcrypt or scrypt to store password (expected using salt too).
Seems like we need to build our own way to save them safely.

http://blog.robertonodi.me/node-authentication-series-email-and-password/
https://github.com/saintedlama/passport-local-mongoose
https://gist.github.com/debrouwere/a46dec73629c71706809

@ccoenen
Copy link
Contributor

ccoenen commented Oct 16, 2016

Aah! Looking at them now!

One of the nice things about the two functions is, that they implement salting along the way! No need to add this yourself. You get a long twisted thing back, that also contains information on how strong it has been hashed and its salt value. There's a more detailed answer.

Example 1 uses pbkdf2 which is a little bit dated, but better than nothing.

Example 3 hashes passwords here: https://gist.github.com/debrouwere/a46dec73629c71706809#file-passport-js-L44
and verifies passwords here: https://gist.github.com/debrouwere/a46dec73629c71706809#file-passport-js-L53-L59

This looks like the way to go, to me.

@timonsku
Copy link

I would also like to express my support for this feature. An absolute necessity for locally hosted instances.

@jackycute
Copy link
Member Author

Supported in a73d9ce

@Joe136
Copy link

Joe136 commented Dec 7, 2016

I have removed all social-auth methods and set "email": true. Then there was no Sign in-button anymore.

@jackycute
Copy link
Member Author

Hi @Joe136,
Oops, missed that, thanks for heads up.
Fixed in ecb956f

@Joe136
Copy link

Joe136 commented Dec 9, 2016

Thanks

@Joe136
Copy link

Joe136 commented Dec 13, 2016

Hi,
when I type my email and password and press enter while the focus is in the password field, he tries to 'Register' me; normally I expect to 'Sign in'.

@jackycute
Copy link
Member Author

As you wish in 00e2845

@Joe136
Copy link

Joe136 commented Dec 14, 2016

Thanks, rotating them looks great as well.

@marmistrz
Copy link

What's the problem storing a sha256sum (or any other hash function we believe is preimage-resistant) of the password?

@SISheogorath
Copy link
Contributor

@marmistrz The reasoning is simple: You can't steal data we don't have.

So when logins are out sourced to GitHub, Twitter, … the only thing an attacker can steal are session tokens. Not more.

But please notice how old this issue is. CodiMD supports local logins and stored passwords using scrypt. But if you are interested in contributing, local users still need a lot of work especially when it comes to management. So feel free to have a look at #272 and #314. PRs are welcome :)

@ccoenen
Copy link
Contributor

ccoenen commented Jun 30, 2018

@marmistrz please do not ever store passwords just as a hashed value. This is quite simple to circumvent. Sha256 (and others) are designed to be very fast, so you can also run them in parallel on things like graphics cards. Brute forcing something as short as most passwords will cost you a few hundred bucks on AWS or similar services.

bcrypt and scrypt are intentionally designed to be slower.

Topic you might want to read up on:

  • salting / peppering
  • rainbow tables

This article here is a good start: https://www.wired.com/2016/06/hacker-lexicon-password-hashing/

@marmistrz
Copy link

@ccoenen I thought that the password ought to be salted is so obvious that it needn't be stated :)
If you add 128 bits of nonce as salt, that should be enough to stop even sha256 rainbowing, I think.

@ccoenen
Copy link
Contributor

ccoenen commented Jun 30, 2018

In my experience, it is very dangerous to rely on obviousness in security. I once opened an issue in a password manager, where the passwords were "encrypted" to the email address (which was stored in the same database).

Please forgive me, if this came off a little strong.

Long story short: why are we even talking about this? This issue has been closed/implemented 18 months ago. With a better solution (scrypt was specifically designed for passwords, and mitigates attacks that would work on sha* hashes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Wants to add a new feature
Projects
None yet
Development

No branches or pull requests

6 participants