Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:89016
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.54 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <0d0b01d10b7f$5ecf4890$1c6dd9b0$@belski.net>
References: <CAAyV7nGLiGC_E1YYc21chGSKdUJm1KpZV9V3_rZn5eHOmyEarA@mail.gmail.com>
	<0d0b01d10b7f$5ecf4890$1c6dd9b0$@belski.net>
Date: Fri, 30 Oct 2015 11:57:34 +0100
Message-ID: <CAAyV7nEVU+13iL5R7nMSW6bf4ocrRYis7FLw+_J5fK_=2VVA4A@mail.gmail.com>
To: Anatol Belski <anatol.php@belski.net>
Cc: "internals@lists.php.net" <internals@lists.php.net>, Kalle Sommer Nielsen <kalle@php.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Password_hash salt generation refactor
From: ircmaxell@gmail.com (Anthony Ferrara)

All,

On Tue, Oct 20, 2015 at 11:35 PM, Anatol Belski <anatol.php@belski.net> wro=
te:
> 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 agai=
nst
>> 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 n=
ot 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 (whe=
re
>> prior it would simply make a "best effort".
>>
>> Thoughts?
>>
> As much as it could be an improvement of the password_hash(), one would b=
etter 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_ra=
ndom_bytes() that side effect free.
>
> Could php_random_bytes() be extended with a flag that would tell exceptio=
ns to pass? Or maybe exceptions could be moved out from the php_random_byte=
s() and thrown directly in the new functions that was undergoing the RFC bu=
t not password_hash() ? I'd be actually for the second variant.
>
> That ways FAILURE would be returned, but a decision about what to do abou=
t 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