RE: [PHP-DEV] Password_hash salt generation refactor

Hi Anthony, > -----Original Message----- > From: Anthony Ferrara [mailto:ircmaxell@gmail.com] > Sent: Friday, October 30, 2015 11:58 AM > To: Anatol Belski <anatol.php@belski.net> > Cc: internals@lists.php.net; Kalle Sommer Nielsen <kalle@php.net> > Subject: Re: [PHP-DEV] 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: �}�a; > ���G��%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. > IMHO this looks fine for 7.0. The API is new and was not exported, so no ABI breach and no behavior change when used for internal refactoring. Regards Anatol