[2.2][Security] Outsource all the BCrypt heavy lifting to a library #7247
Conversation
|
seems like a good idea. /cc @ircmaxell |
|
-1, (out of principle) I'm not in favor of adding a dependency to the standard framework that defines functions in the global namespace, even if it is purely for BC. password-compat is at it's core a compatibility plugger, not a Bcrypt "heavy-lifter", and it's mostly made redundant by the core SecureRandom classes. |
|
I agree with @gerryvdm. We spent quite some time on the SecureRandom class, with many eyes on it. |
|
@fabpot, the library uses the PHP 5.5 simple password hashing API when available and falls back to Are you sure we need to keep dealing with all that base64 stuff ourselves? Also we have to decide which bcrypt algorithm to use based on the PHP version. It feels like a hack. Why not rely on a de facto library for that? Also, my bundle uses the library and I hoped I could tell its users they can abandon the bundle in favor of the built-in encoder, but without this commit I don't feel like it. |
|
@fabpot I just took a look at the "SecureRandom" class, and it leaves a bit to be desired. Check out these two blog posts for some reasons why: http://blog.astrumfutura.com/2013/03/predicting-random-numbers-in-php-its-easier-than-you-think/ http://phpsecurity.readthedocs.org/en/latest/Insufficient-Entropy-For-Random-Values.html I'll open a new ticket for that which details specific issues and recommendations. However, I disagree with @gerryvdm on two points. First off, that reply is basically the definition of NIH syndrome. Second, this is not a "whatever you want to call it". It's a polyfill. If you use it, the encoder can simply not care what version of PHP you're on, and simply call the API. As it is now, your encoder has two unique execution paths (<=5.4 and >=5.5). Leveraging the polyfill will allow you to only maintain one path, and let the other fall to the 3pd maintainer. It's not like you're doing something different from password_compat. In fact, you're calling the same API if it's available... My $0.02 at least... |
|
@fabpot, please reconsider. |
|
What is the benefit of the lib? Crypt works fine? And the comments on SecureRandom, shouldn't we fix that class instead? |
|
The benefit is to get a higher abstraction of working with BCrypt. The lower level we go, the higher chance there is of doing something wrong. I thought the PHP community got cured of the NIH syndrome. 😉 |
|
has @ircmaxell expanded anywhere on his "leaves a bit to be desired" comment? |
| @@ -19,7 +19,8 @@ | |||
| "php": ">=5.3.3", | |||
| "doctrine/common": "~2.2", | |||
| "twig/twig": "~1.11", | |||
| "psr/log": "~1.0" | |||
| "psr/log": "~1.0", | |||
| "ircmaxell/password-compat": "1.0.*" | |||
There was a problem hiding this comment.
it's not a requirement, only for testing, so it should be moved to the require-dev section.
|
@ircmaxell I've decided to actually used your library in Symfony (see #7853). You mentioned some problems with our SecureRandom class, can you open a ticket for this? Thanks. Closing in favor of #7853 |
This PR was merged into the master branch. Discussion ---------- [Security] Outsource all the BCrypt heavy lifting to a library | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | License | MIT The [BCrypt bundle](https://github.com/elnur/ElnurBlowfishPasswordEncoderBundle) is already using the library. This is a working implementation of #7247 Commits ------- c83546d [Security] tweaked previous commit b2e553a Outsource all the BCrypt heavy lifting to a library
The BCrypt bundle is already using the library.