PasswordEncoder should support BCrypt

[fabpot]

see discussion here:

https://groups.google.com/group/symfony-devs/browse_thread/thread/92ae24eafd13e29e?hl=en

[stof]

@elnur maybe you should simply contribute your encoder to the core for 2.2 ?

[elnur]

Yea, no problem. I'll do it on this weekend.

[theoreticaLee]

@elnur you want to pair up on this one? I can tie up what we already have in a bow and submit the pull request.

Here is the port on your very nice code:
https://github.com/theoreticaLee/symfony/commit/41281dcddcaa717bf358bbc611a8d931d1f7fb25

I want to review it one more time, and get your go ahead and I'll submit a pull request

[elnur]

@theoreticaLee, thanks, but I better do it on my own.

First of all, I want to rename the encoder and at least one variable. Then, it's not just the encoder that needs to be ported, but the configuration stuff, tests, documentation and changelog need to be added as well.

So, just give me a couple of days and I'll do it. ;)

[theoreticaLee]

ok, removed commit.

[mvrhov]

https://gist.github.com/4040680 :)

[bzikarsky]

If you convert the ElnurBlowfishPasswordEncoderBundle into a Symfony2 default, can you have a look at PERBILITY\BCryptBundle. It's pretty much the same as your bundle, but provides support for a) a global salt which can be defined at application level and b) the possibility to bind the hash to additional user-data (such as email).

[mvrhov]

@TerjeBr: it was all already taken care of in my gist. Which is also faster as it uses standard base64.

[TerjeBr]

@mvrhov I can see at least 2 errors in your getSalt() function. First it requires 18 random bytes instead of 16.
Second, if the varible $this->useOpenSsl is false or the openssl_random_pseudo_bytes function fails to give a strong result, you fall back to using a base64 encoded sha256 hash. But here you have forgotten to replace the '+' character with '.' as you did further up in the function.

[mvrhov]

Now the salt is indeed a bit longer. I'm just making sure that we are not short and at the same time too greedy.
The fallback is to hex encoded salt, so this is actually not a bug.

[TerjeBr]

@mvrhov Have you studied your own code?

return substr(hash('sha512', uniqid(mt_rand(), true), true), 0, $nbBytes);

When $nbBytes is 16, this will return 16 raw bytes not encoded as anything. That is one error.
The second error, is why on earth do you first get data from mt_rand, then run it through uniqid and then through hash_sha512? You are just wasting entropy everywhere here, and making the possible different retrunvalues less.

Your third error is in the above comment. You cannot return the value hex encoded. It will just be interpreted as base64 on the crypt routine all the same, but again you limit the entropy. Only those base64 values that also happen to be valid hex will be used.

If you are going to use mt_rand as a fallback, it is best to just use that value directly in your code, as I have done.
That will give the best entropy in the result.

Also, why not use /dev/urandom as a source if the system has an /dev/urandom? That is the first choice in my code.

(And as I said before, please feel free to copy any chunks of code from me.)

[stof]

Well, when submitting the encoder, the generation of the salt should use the SecureRandom class anyway instead of trying to duplicate it (and it provides a secure fallback when SSL is disabled)

[TerjeBr]

Here is my latest version of the code

@stof Be aware that the random generation is only one part of my complaint with the code as it stands.
Please have another look at my implementation of the function gensalt_blowfish($random)