bshaffer / oauth2-server-php

A library for implementing an OAuth2 Server in php
http://bshaffer.github.io/oauth2-server-php-docs
MIT License
3.26k stars 953 forks source link

password_hash / password_verify #855

Open jonathanplong opened 7 years ago

jonathanplong commented 7 years ago

I have no idea if this is the right area to comment on this. I would hardly call this an issue, however I'd like to provide my insight and maybe get some feedback from individuals who are better versed with this library (if anyone cares to comment, that is).

First and foremost: Another user indicated that it would have been helpful if the documentation indicated (somewhere) that (by default) the passwords need to be hashed in the database. I kept getting invalid username/password errors, and I thought there was a mistake somewhere. Maybe it is somewhere and I was just to stupid to read it.

Before I get flamed - Yes, I know plaintext passwords are bad. I did keep thinking to myself that it was a bit odd that the passwords would be in plaintext.

After I figured out that I needed to store the password as an SHA1 hash - everything worked fine. I then read the comments in the files which said to use a secure hashing algorithm to store the passwords, which led to a few hours trying to better understand the best mechanism to salt the passwords (thankfully I'm on a stint of sobriety while I work on this project, ha).

The best recommendation I came across was using the password_hash() function of PHP to dynamically generate a unique salt + password hash for each user, and using password_hash() to confirm the hash matches the password.

Given my inexperience with PHP, I have no idea if this is the proper way to modify the code - but I took the following code:

protected function checkPassword($user, $password)
{
    return $user['password'] == $this->hashPassword($password);
}

and changed it to read:

protected function checkPassword($user, $password)
{
    // original line below
    // return $user['password'] == $this->hashPassword($password);
    return password_verify($password, $user['password']);
}

I think this is acceptable, but (as I mentioned previously) I would like some input on this modification.

Also, on an un-related note - if I don't want to use this as a library which is integrated into another application, but a standalone "application" - are there any concerns or issues with doing that?

bluebaroncanada commented 7 years ago

You should use checkUserCredentials.

Also, he's using sha1 encryption, and you're right, that should be return password_hash($plainTextPassword, PASSWORD_DEFAULT); and return password_verify($this->plainTextPassword, $this->user->getPasswordHash()); in the repo code. You don't actually have to deal with that. You just pass in the plain-text password. You should try to learn how to read other people's code.

You can do a few things from here: 1) Acquiesce to using the built in sha1 hashing. 2) Submit a pull request that fixes this. 3) Implement a new storage interface that implements the same interfaces. 4) Extend the storage class and override the methods that you want to.