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

[Proposal] Improve BCrypt hashing component. #16

Closed
ziadoz opened this issue Jan 13, 2013 · 8 comments
Closed

[Proposal] Improve BCrypt hashing component. #16

ziadoz opened this issue Jan 13, 2013 · 8 comments

Comments

@ziadoz
Copy link
Contributor

ziadoz commented Jan 13, 2013

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

ziadoz commented Jan 13, 2013

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
Copy link
Contributor

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
Copy link

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor

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//issues/16#issuecomment-12406571.

taylorotwell pushed a commit that referenced this issue Feb 2, 2019
gonzalom pushed a commit to Hydrane/tmp-laravel-framework that referenced this issue Oct 12, 2023
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

No branches or pull requests

5 participants