Re: Password_hash salt generation refactor

All, On Tue, Oct 20, 2015 at 11:35 PM, Anatol Belski <anatol.php@belski.net> wrote: > Hi Anthony, > >> -----Original Message----- >> From: Anthony Ferrara [mailto:ircmaxell@gmail.com] >> Sent: Monday, October 19, 2015 1:00 AM >> To: internals@lists.php.net >> Subject: [PHP-DEV] Password_hash salt generation refactor >> >> All, >> >> With PHP 7 comes random_bytes and random_int. This duplicates some of >> the logic internally that password_hash uses to generate its salt. >> >> I would like to refactor this to unify generation. I've opened a PR against >> master: https://github.com/php/php-src/pull/1585 >> >> I don't feel comfortable pulling against 7 this far into RC status. >> Perhaps wait until after it goes gold? Or should this target 7.1? It's not a big >> deal in either direction. Though it does add a side-effect, where if it can't >> gather enough entropy it will throw an exception and return failure (where >> prior it would simply make a "best effort". >> >> Thoughts? >> > As much as it could be an improvement of the password_hash(), one would better avoid any > behavior change at this stage. I was thinking about it and came to an idea that maybe could work for > 7.0 - one should just make php_random_bytes() that side effect free. > > Could php_random_bytes() be extended with a flag that would tell exceptions to pass? Or maybe > exceptions could be moved out from the php_random_bytes() and thrown directly in the new functions > that was undergoing the RFC but not password_hash() ? I'd be actually for the second variant. > > That ways FAILURE would be returned, but a decision about what to do about it would be made > outside. IMHO it were also useful as the API is intended to be exported. So for the cases where > php_random_bytes() came to use as a library function outside immediate PHP userspace (for example in > SAPI), it should not throw exceptions from itself. > > Do this suggestions sound eligible? I have created a PR for this: https://github.com/php/php-src/pull/1614 It changes the public API of the internal php_random_bytes function to have a third "should_throw" parameter, as well as defining two macros: php_random_bytes_throw(dest, len) and php_random_bytes_silent(dest, len). If this looks acceptable, it can be pulled into 7.0, and then the move for password_hash can be done at a later date. Thanks Anthony