laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.26k stars 10.93k forks source link

[Proposal] Improve BCrypt hashing component. #16

Closed ziadoz closed 11 years ago

ziadoz commented 11 years ago

Seeing as other Laravel components are leveraging some of the best PHP libraries available, it seems like the BCrypt hashing component should do the same. I recommend making it leverage either Kherge/BCrypt or Password Compat.

They both have a better selection of salt generators (MCrypt, OpenSSL, DevRandom, Microsoft CryptoAPI) and select the best available in the environment, falling back to the next most secure salt generator (until eventually it'll use plain old MtRand). They also switch between BCrypt versions 2a and 2y depending on the version of PHP running because 2a has a security issue. Additionally they're well tested.

It's worth reading Anthony Ferrara's article about screwing up BCrypt as it highlights some of the common mistakes (a few of which exist in the current L4 implementation).

taylorotwell commented 11 years ago

Would prefer to use ircmaxell's Password Compat, unforuntately he hasn't tagged a stable version of specified a branch-alias on Composer. Kherge's also looks good. Do you know if it is forward compatible with PHP 5.5's password functions?

jasonlewis commented 11 years ago

It doesn't appear to be @taylorotwell, I think you're better off using Password Compat when it becomes stable enough. Perhaps @ircmaxell just needs a bit of a jump start. ;)

ziadoz commented 11 years ago

I'd prefer to use Password Compat too, and it makes sense to wait for a stable release if that's the route you want to go. I don't believe Kherge's is compatible with the PHP 5.5 API right now, but one nice thing about it is that the salts generators are decoupled and could be used to do other stuff (E.g. provide a command line tool to generate a secure L4 application key).

franzliedke commented 11 years ago

Regarding @ircmaxell's class, we should probably take into account that the whole point of the library is to not change the API. In that sense, it could probably be integrated rather soon-ish (strictly speaking, L4 isn't even beta yet). As long as Anthony can promise that it will get to a finished, complete and stable state in time for the final.

ircmaxell commented 11 years ago

All, I've just pushed 1.0.0 as a production version. It's stable. The only difference was pulling the version check. So now it's on the user to determine if it's 5.3.7 or newer (or supports $2y). If you require 5.4, it should be fine for all versions...

taylorotwell commented 11 years ago

@ircmaxell's library has been adopted! thanks all...

btw, irc's password_verify will work with hashes that were already created with L4 prior to this transition, so should be no problem upgrading.

taylorotwell commented 11 years ago

I should note that the Hash::make and Hash::check API is still the same in L4, just that ircmaxell's library is being used as the implementation under the hood.

jasonlewis commented 11 years ago

Cool cool, good stuff Taylor. On Jan 18, 2013 2:46 PM, "Taylor Otwell" notifications@github.com wrote:

I should note that the Hash::make and Hash::check API is still the same in L4, just that ircmaxell's library is being used as the implementation under the hood.

— Reply to this email directly or view it on GitHubhttps://github.com/laravel/framework/issues/16#issuecomment-12406571.