[5.6] Start adding argon hashing support.

[morloderex]

Hello everyone.

With the release of php 7.2 which is due to be released at the end of november Argon 2i password hashing was added. rfc for reference

So i have added support of both bcrypt and Argon2i password hashing. Simply by adding a new class. And returning a password manager within the serviceProvider.

Note: Argon2i password hashing is only availiable if a speciel confuration flag is passed during php compile time.

Edit: If ypu are on a mac and is using homebrew i added the ability to compile php 7.2 with the correct configuration flag. use the fallowing command brew install php72 --with-argon2 if you want to test it.

[paulofreitas]

Seems a good starting point. 👍

[abhimanyu003]

Interesting 👍

[juukie]

I would at a dot at the end of this line

[taylorotwell]

The bcrypt helper would need to be forcing the Bcrypt implementation.

[Miguel-Serejo]

Would it make sense to also add a hash() helper that uses whatever driver is set as default?

[m1guelpf]

@36864 hash() is already a PHP function. Also, another name for a hash helper has been widely discussed, but we didn't get to anything.

[Miguel-Serejo]

I guess it didn't make much sense if it was going to use bcrypt anyway. While most people will probably choose one or the other and use the appropriate helpers, if they change their minds in the future they'll potentially have to make changes in a lot of places. Just spitballing, but what if these helper functions were prefixed with 'hash_', so we could have hash_bcrypt(), hash_argon() and maybe hash_default()? I'm not sure if that adds readability or unnecessary complexity (besides being a BC-break). Maybe I'm overthinking this and/or overestimating how much use this will have. I think most people will continue using bcrypt for the foreseeable future and not even know about argon. However, if in the far future bcrypt is found to be broken and people have to switch over to argon, using something like hash_default() and just changing a config variable would be far better than having to go through every file and switch bcrypt() to argon(). And if there's a BC break, people may choose to use hash_default instead of hash_bcrypt, either because they don't know/care which algorithm to use, or because it will be easier to change in the future.

…

On Tue, Oct 31, 2017 at 6:21 PM, Miguel Piedrafita ***@***.*** > wrote: @36864 <https://github.com/36864> hash() is already a PHP function. Also, another name for a hash helper has been widely discussed, but we didn't get to anything. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21885 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGzCJHtnExkukNb6i7C2Phdw4-bh96C3ks5sx2UvgaJpZM4QL9eJ> .

-- Cumprimentos, Miguel Serejo

[morloderex]

I’ll leave that up to @taylorotwell to decide.

[paulofreitas]

Some discussions on this topic just for further discussion:

• rename bcrypt to hasher and allow null value (PR #21502)
  Some new helpers ideas emerged.
• Add a str_hash() helper (PR #20169)
  Found out the str_ prefix does not match with "password hashing".
• Rename bcrypt() helper (PR #20155)
  Favored [5.5] bcrypt changed to Hash::make and renamed to str_hash :D #20152.
• bcrypt changed to Hash::make and renamed to str_hash (PR #20152)
  First proposal, ideas started emerging.
• Use Hash::make() instead of a direct call to bcrypt in ResetsPasswords.php (internals issue #695)
  Where discussions started.

The only thing decided is that, if helpers have to change at some point, the bcrypt() helper would still be included and deprecated in the next major release following the deprecation promise. 👍

[taylorotwell]

I don't really want to add any new helpers, I just want to force bcrypt to the Bcrypt implementation.

[morloderex]

It should already be forcing the correct implementation if it’s not can you point me in the right direction?

I’ll remove the new helper I added.

[morloderex]

Removed the helper.

[taylorotwell]

Do you see that you have failing tests?

[OwenMelbz]

The travis environments look like they would need to have argon enabled on the php install - is that something the community can adjust or only @taylorotwell ?

[morloderex]

@taylorotwell The failing tests on travis should now have been resolved.

[taylorotwell]

Why did you make another binding for bcrypt? Is that really the way to solve the issue?

[morloderex]

Maybe i was thinking of this wrong the wrong way. But sense the manager before just returned whatever was set as the default driver. Forcing bcrypt in the bcrypt helper made it very difficult. Spilting up the bindings makes this a lot easier. But if you know any other way i could do this without another binding some directions will be helpful :)

[taylorotwell]

Why would the helper not do app('hash')->driver('bcrypt')... am I missing something?

[taylorotwell]

I think everything else looks pretty fine if we can work out this binding issue.

[taylorotwell]

Fixed the broken tests you were having.

[BenExile]

I added Argon2 hashing support for 5.5 (with a dependency on libsodium) in #21524 and this was closed. PHP 7.2 is approaching stable, but it would be great to see backwards compatibility with >= 7.0.

[ericdowell]

The command to install PHP 7.2 with argon hashing for MacOS (homebrew) listed in the PR description is missing the number 2 in the with flag.

The command is: brew install php72 --with-argon2

[bolajipemipo]

A little light needed here @taylorotwell @morloderex

For an existing application that uses bcrypt, which means the passwords were hashed using bcrypt, won't changing the config to argon (for newer signups) affect the decrypting of the earlier "bcrypted" passwords?

[BenExile]

The ArgonHasher::check() method uses PHP's built-in password_verify function, so users with passwords using the bcrypt algorithm will still be able to log in. You would need to check at the time of login whether the hash used the correct algorithm and options, and rehash if required:

// The user credentials have been verified prior to this step

if (Hash::needsRehash($hashedPassword)) {
    $user->password = Hash::make($plainTextPassword);
    $user->save();
}

[bolajipemipo]

@BenExile, thanks. Will check it out.