Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120598
Message-ID: <224526d9-8ba5-0080-2b79-7b185436e450@bastelstu.be>
Date: Thu, 15 Jun 2023 21:17:31 +0200
MIME-Version: 1.0
Content-Language: en-US
To: internals@lists.php.net
References: <CAH5C8xWAsRV+8h-tZ2Xq0=tn753kbSQX=KSEYsE2fWEtQVMrew@mail.gmail.com>
 <CAMs36HfmXuUJWJ0uih+Pj_vui4wG4vy3OTMmmspGmpP9h_gvgw@mail.gmail.com>
 <9ea3a5af-679d-ad63-f9c2-e0d8d148db3f@bastelstu.be>
 <CABTDVhDPLwBHDDzfXcHhs4VZNMHswWqum9muKgZcA63sgMCZdg@mail.gmail.com>
 <5ca4e382-8284-1cd0-696f-0dfd693523f8@bastelstu.be>
 <CAH5C8xWnAjep7kkTSg3=koBd7U0XuBkwF0-k61Mvv1J-m5yRiQ@mail.gmail.com>
 <CALKiJKrP5n=i_MLqny_+bxVyUopQZYvKYrO0wgCvvFDtnji_=Q@mail.gmail.com>
 <5604f988-5ca5-6aca-72ee-8b45320a9836@bastelstu.be>
 <4cf80ee8-d1df-a021-78db-db9b68e8eef9@gmail.com>
In-Reply-To: <4cf80ee8-d1df-a021-78db-db9b68e8eef9@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [RFC] [Discussion] PHP 8.3 deprecations
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)

Hi

Thank you.

On 6/15/23 20:24, Rowan Tommins wrote:
>> Looping back to the beginning of my email: The recommended replacement
>> is random_int() which is available for years, but the "organic"
>> migration did not really work.
> 
> 
> I think that's partly because, rightly or wrongly, random_int() is not
> generally viewed as a universal replacement for rand()/mt_rand(). For
> instance, consider the opening description in the manual for random_int():

For the explanation of the new OO API I took great care to document 
Random\Engine\Secure (which uses the same underlying randomness as 
random_int and random_bytes) as the *default* choice. See the 
documentation quotes in my reply to Nikita.

Due to needing to prioritize I did not yet get around to bringing up the 
existing documentation fully up to shape, except for necessary cleanup work.

Writing a good high-level description is especially hard (that's also 
what the Randomizer methods are still lacking). My goal here was not to 
have some "okay-ish" documentation that needs multiple revisions due to 
being wrong, but great documentation.

>   > Generates cryptographic random integers that are suitable for use
> where unbiased results are critical, such as when shuffling a deck of
> cards for a poker game.

Agreed, this is not ideal to sell random_int() as the default choice for 
all things being equal. I've made an attempt for a more neutral / 
inclusive wording in https://github.com/php/doc-en/pull/2528. Parts of 
the description have been copied as-is from the explanation of the 
Secure engine.

> And the Caution on the manual for rand() and mt_rand():
> 
>   > This function does not generate cryptographically secure values, and
> /must not/ be used for cryptographic purposes, or purposes that require
> returned values to be unguessable.
> 
> Note that both talk about using random_int() *in particular situations*,
> not as a universal replacement.

This case is a little more complicated, because there are multiple 
issues and the existing description of mt_rand() is not particularly 
good in the first place [1]:

- They are not cryptosafe. This is mentioned.
- The randomness is not particularly good even outside of cryptographic 
applications. The main reason for this is the abysmal seeding, which 
*is* mentioned in the *s*rand docs. Perhaps the Caution note in 
mt_srand() should also be applied to mt_rand() itself? At that point the 
documentation turns into a giant warning though, which technically isn't 
wrong, but should probably be considered to be an indicator that 
mt_rand() is broken beyond repair.

[1] It starts by describing something about "libc randomness", using 
terms the average developer knows nothing about.

> Add to that the scary fact that random_int() can fail with an exception
> (the technical detail of how unlikely that is probably goes over the
> head of the majority of PHP programmers), and the perception that it's
> significantly slower (which may or may not be true, or relevant to most
> users), and many people will be actively choosing not to use it when
> they don't need its guarantees.
> 

Yeah, I get behind that. Let me just answer this here for the record:

1. The CSPRNG can technically fail, it is effectively infallible on 
modern operating systems, unless the system is severely misconfigured.

2. The CSPRNG *is* slower than a non-CS PRNG, but this is not likely to 
matter for the majority of applications. If it matters then Mt19937 
(which is what is behind mt_rand()) is not the right choice anyway. 
xoshiro256** is - that's the fastest engine PHP provides.

> On the other hand, I'm sure you're right that there are people misusing
> rand()/mt_rand() in contexts where they really should use something
> secure. Maybe with improved documentation at the same time, a
> deprecation could be OK; but it would be worryingly easy to say
> "deprecate first, we'll get round to the documentation later", and have
> lots of confused users who think we're suddenly deprecating something
> that's been working fine for 20 years.
> 

Please see my PR above. I'm putting my money where my mouth is to 
improve the documentation and I'm looking forward to your ("you" 
referring to the broader audience of this email) feedback with regard to 
improving the documentation here. If you think you have something 
worthwhile to add, please add a review or send your own PR and ping me 
(@TimWolla) on it. My list of PRs might be a good starting point with 
regard to the changes and improvements I've made (or attempted to make) 
in the past:

https://github.com/php/doc-en/pulls?q=is%3Apr+author%3ATimWolla+is%3Aclosed

Best regards
Tim Düsterhus

PS: Don't get me started on uniqid() which I consider to be the most 
misused function in PHP with horrible constructs like 
sha1(md5(uniqid(mt_rand() . microtime(), true) . mt_rand())). I hope to 
deprecate that one with 8.4, because getBytesFromString() is available 
then as a reasonable replacement.