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

Bcrypt #868

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Bcrypt #868

wants to merge 10 commits into from

Conversation

danastasio
Copy link

@danastasio danastasio commented Jan 24, 2020

This is my first pull request!

I have modified the code to allow for bcrypt based password hashing. In order to allow current users to continue connecting to the server, the authentication mechanism looks at the hash to see if it starts with $2y$ and if it does, it uses password_verify. If it does not, then it uses the existing auth mechanisms.

In order to keep digest method, I added a drop down on the settings page that allows the user to choose between basic and bcrypt authentication, along with a note that states if the setting is changed, the password must be changed.

Passwords will be unaffected by default. If set to use bcrypt, passwords will only be changed the next time they are changed. Passwords will authenticate no matter which hash they are using.

This will close #514

Anyway, I have no idea how I'm supposed to end this or even if I'm rambling at this point. Thanks for the opportunity to help!

@MalteKiefer
Copy link

Will this be integrated to increae the security of baikal? @ByteHamster

@danastasio
Copy link
Author

@ByteHamster I submitted this PR a while ago, I know you're busy but is there any chance you could look it over and let me know if you have any thoughts? Thanks!

@ByteHamster
Copy link
Member

Sorry for ignoring you for that long. I was busy with university. I want to merge #835 first but that PR needs some additional work. That's why I did not reply yet. Most of my free-time activites got cancelled because of Corona, so chances are that I will find more time to work on Baikal in the next few weeks :)

@danastasio
Copy link
Author

@ByteHamster I would still like to see this get merged, but I don't want to work on it if you don't have any plans to include this.

@ByteHamster
Copy link
Member

I just had a look at the changes. What is the advantage of using bcrypt? To me, it looks like it makes authentication less strong because passwords are sent in plain text (in contrast to digest auth, where they are hashed).

@danastasio
Copy link
Author

danastasio commented Aug 22, 2020

MD5 hashing has been deprecated for a long time now and straight up isn't secure.. Bcrypt is pretty much the de-facto standard in password storage these days. Passwords are sent in cleartext, but any internet facing server (that cares about security) needs to have an HTTPS connection anyway, so this is not susceptible to MITM attacks. Also, I wrote my changes so you choose between the existing auth mechanism and the updated, modern one. So people that don't want to change for whatever reason don't have to.

https://www.sitepoint.com/why-you-should-use-bcrypt-to-hash-stored-passwords/

@ByteHamster
Copy link
Member

any internet facing server (that cares about security) needs to have an HTTPS connection anyway

I have seen quite a few Baikal installations that do not use https. A possible reason for this is that some cheap hosting services do not support it or charge extra.

I totally agree that passwords should generally be stored with a better hashing algorithm than md5. I just fear that having an option that requires basic auth could encourage users to come up with a configuration than is more vulnerable than without the option.

This doesn't mean I am strongly against it. I am just not sure what would be better for a majority of users.

@phil-davis @DeepDiver1975 Do you have an opinion about this?

@MichaIng
Copy link

MichaIng commented Oct 12, 2020

While bcrypt is still fine, since PHP7.2 there is Argon2i (since PHP7.3 Argon2id) available as algorithm with the password_hash function. When already start using this, I'd at least prepare the code a way that Argon2 can be implemented easily later on, if not directly:

As example see how Nextcloud implemented it: https://github.com/nextcloud/server/blob/master/lib/private/Security/Hasher.php

Take care that there are options when using Argon2 which should be reasonably adjusted based on the system, at least using parallelism that matches the CPU core count. The time_cost options is correctly explained in the RFC, but from all I found wrong in the PHP function documentation, see: nextcloud/server#23319

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.

Use proper password hashing algorithm
4 participants