Use bcrypt for passwords instead of SHA1

[dicksonlaw583]

This changes the password hashing method to bcrypt, which is much more resistant to mass computation and now the industry standard for storing passwords.

[zerocrates]

Replacing the home-grown SHA-1 hashing/salting with password_hash is definitely a good idea.

There's at first certainly the wrinkle of this requiring PHP 5.5, though obviously extensive old-version support is less relevant these days than it used to be. But it probably informs us on what kind of release we'd put this in minimally. We could also use ircmaxell's password_compat library to extend compatibility to our current 5.4 lower limit... part of the reasoning behind choosing 5.4 when increasing previously was to both ensure Composer compatibility and get beyond the early 5.3 versions that had a problem with their bcrypt implementation.

As for how it actually is implemented, it looks pretty good to me. Reusing the salt column with just the content "bcrypt" is a clever solution to being able to tell the difference between the formats for "free" without having to add another column.

In terms of some specific nits:

I think I'd basically reverse the order of your checks when seeing if you need to "reset" the password: check for $salt === 'bcrypt' first as it would become the usual expectation going forward, then $salt !== null to check the previous format, then just the hash check for the ancient format. This also results in fewer redundant checks against what the salt is or isn't than the order you're doing it now.

Along those same lines, we might as well build in password_needs_rehash support while doing this at the same time. And it then probably makes sense to just use PASSWORD_DEFAULT instead of PASSWORD_BCRYPT then, and just bite the bullet and use a 255-character hash column as the PHP manual suggests (and we could go ahead and collate it as latin1_bin while we're at it since we really don't need UTF-8 there).

[zerocrates]

On your last commit, I think just leave to us whether we want to just bump the version to 5.5 or go through the trouble of pulling in the password_compat polyfill. It does various other things to try to get "better" randomness for the salt that are worth getting vs. the mt_rand thing you're doing there.

So you could revert that. I don't think you need to actually make any changes to accommodate my concern about the PHP verisons.

[zerocrates]

What do you think about my suggestion about PASSWORD_DEFAULT and the larger column size?

[dicksonlaw583]

I have some doubts about PASSWORD_DEFAULT as the description on the PHP Manual says that its format and algorithm can change over time. Using that would make it difficult for us to stay consistent across different PHP versions going forward. With PASSWORD_BCRYPT, we know exactly what to expect out of it. But having a larger column size is definitely doable for future-proofing purposes.

[zerocrates]

Hmm... I suppose in a future version if they changed PASSWORD_DEFAULT it could cause compatibility issues if you ran an installation for a while on that newer version then wanted to move back to an older one which didn't have support for the algorithm. How realistic that is of a concern, I'm not sure (one imagines only an algo that has been supported for quite a long time would ever be switched to be the new default).

Worth noting that we're already using PASSWORD_DEFAULT for Omeka S... but also the difference is currently academic as there's never been a different default so far.

[dicksonlaw583]

@zerocrates

With PHP 5.4 and PHP 5.5 dropped for 3.1, I think now would be a good time to revisit this PR as the bcrypt compatibility issue won't apply going forward. I have just rebased the PR branch awaiting your review. What do you think?

[zerocrates]

Thanks for continuing to work on this; I'll take a look soon.

I agree with you that the higher minimum PHP version of 3.1 makes the decisions here easier.