Hi all,
This is RFC for improving uniqid() uniqueness.
https://wiki.php.net/rfc/uniqid
PR
https://github.com/php/php-src/pull/2123
If there is anything left to discuss, please comment.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
This is RFC for improving
uniqid()uniqueness.
https://wiki.php.net/rfc/uniqidPR
https://github.com/php/php-src/pull/2123If there is anything left to discuss, please comment.
I think uniqid() should not be changed in BC break way, it should be
left as is.
You said,
Almost all
uniqid()usages do not care about return value chars nor
length. Therefore, BC will be minimum.
but you may be underestimating.
I found that some code saved output of uniqid() without more_entropy to
DB, in the search results. Output length change may cause problem in
such case. And you are not supposed to forget that most of php codes
are not open source and not opened to the public.
In addition, you shoud hear "I expect the numbers to grow" about output
of uniqid(), as reply to you.
--
Kazuo Oishi
Hi Kazuo,
This is RFC for improving
uniqid()uniqueness.
https://wiki.php.net/rfc/uniqidPR
https://github.com/php/php-src/pull/2123If there is anything left to discuss, please comment.
I think
uniqid()should not be changed in BC break way, it should be
left as is.You said,
Almost all
uniqid()usages do not care about return value chars nor
length. Therefore, BC will be minimum.but you may be underestimating.
I found that some code saved output of
uniqid()without more_entropy to
DB, in the search results. Output length change may cause problem in
such case. And you are not supposed to forget that most of php codes
are not open source and not opened to the public.In addition, you shoud hear "I expect the numbers to grow" about output
ofuniqid(), as reply to you.
I know some code breaks, but it's not many. It's not fatal BC also.
IMHO, uniqid() should try to generate uniqid() possible. uniqid() does produce
non unique ID because it is system time based. This change mitigates impact of
misuse also which is common in both open and close codes.
Which is important?
- Fix known issues and generate unique ID (as much as possible)
- Let it generate non unique ID and ignore for some code may complain.
Fixing is my priority.
Smart developers uses mt_rand() to improve uniqueness, but such tweak
shouldn't be needed in the first place as uniqid() should generate unique ID.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
I know some code breaks, but it's not many. It's not fatal BC also.
IMHO,
uniqid()should try to generateuniqid()possible.uniqid()does produce
non unique ID because it is system time based. This change mitigates impact of
misuse also which is common in both open and close codes.Which is important?
- Fix known issues and generate unique ID (as much as possible)
- Let it generate non unique ID and ignore for some code may complain.
IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)
If good unique ID generator is needed in core, please create new
function with another name like "unique_id".
--
Kazuo Oishi
Hi Kazuo,
I know some code breaks, but it's not many. It's not fatal BC also.
IMHO,
uniqid()should try to generateuniqid()possible.uniqid()does produce
non unique ID because it is system time based. This change mitigates impact of
misuse also which is common in both open and close codes.Which is important?
- Fix known issues and generate unique ID (as much as possible)
- Let it generate non unique ID and ignore for some code may complain.
IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)
It cannot not produce unique ID as name "uniqid()" implies by default.
Reason is described in the RFC. Please read RFC because it's the
official proposal.
Since we have to change "more entropy" to TRUE by default, why not use
much better entropy? php_combined_lcg() is legacy entropy generator
must not be used now. New code's entropy is more than a million
times better for the same length. 50 bits entropy is far less enough for
crypt safety, though.
If good unique ID generator is needed in core, please create new
function with another name like "unique_id".
Already is, session_create_id() could be used.
Or UUID if 3rd party module can be used.
Although I would not like to leave legacy functions as much as
possible, this could be a vote option. Any more request for new
function, anyone?
I don't like the name because "unique_id()" implies "absolute unique ID",
but we can only provide "very close to unique ID". I'm not sure if it's good to
have "uniqid()" and "unique_id()" that have the same note:
"This function does not guarantee 100% uniqueness".
If we are going to have unique_id(), I would like to keep timestamp
value, since this improves uniqueness and handy for non crypt usage.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
It cannot not produce unique ID as name "uniqid()" implies by default.
It cannot not produce unique ID by default as name "uniqid()" implies.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Yasuo,
uniqid() has never been, and is not claimed to be, guaranteed unique to
any particular standard.
Since we have to change "more entropy" to
TRUEby default
Is your intention that the version without "more entropy" be deprecated,
and at some point the option removed? Or do you just want to increase
the visibility of this option by enabling it by default?
In other words, do you consider the function to be broken / useless if
this option is not set to true? Or do you think users don't understand
when to use it and when not?
why not use much better entropy? php_combined_lcg() is legacy entropy generator
must not be used now. New code's entropy is more than a million
times better for the same length. 50 bits entropy is far less enough for
crypt safety, though.
What costs and benefits will users see of changing the entropy
generator? Does it make uniqid() collisions less likely, and if so what
kind of probability are we talking about? Does it have a speed or memory
cost (over the existing more_entropy version, i.e. ignoring sleep)?
Even if we accept a) that the default parameters should be changed, and
b) that the source for "more entropy" should be changed, I'm not clear
why the output format also needs to change. Is there some reason the
output of php_random_bytes() can't be encoded into decimal digits,
rather than [0-v]?
Regards,
Rowan Collins
[IMSoP]
Hi Rowan,
uniqid()has never been, and is not claimed to be, guaranteed unique to any
particular standard.
Right. We need to improve documentation. It only has crypt related
usage warning now.
Since we have to change "more entropy" to
TRUEby defaultIs your intention that the version without "more entropy" be deprecated, and
at some point the option removed? Or do you just want to increase the
visibility of this option by enabling it by default?
I don't think we should remove "more entropy" option now, but it may
be preferred.
Currently, I'm proposing "more entropy" by default and use of better
entropy source.
In other words, do you consider the function to be broken / useless if this
option is not set to true? Or do you think users don't understand when to
use it and when not?
It's useful. It's works as serial ID in most cases.
Apparently, some users don't understand what is does. Some of them are
fatal misusages.
why not use much better entropy? php_combined_lcg() is legacy entropy
generator
must not be used now. New code's entropy is more than a million
times better for the same length. 50 bits entropy is far less enough for
crypt safety, though.What costs and benefits will users see of changing the entropy generator?
Does it makeuniqid()collisions less likely, and if so what kind of
probability are we talking about? Does it have a speed or memory cost (over
the existing more_entropy version, i.e. ignoring sleep)?
I pasted simple benchmark to the PR.
New code uses about 2x cpu time on my Fedora 24. CSPRNG uses more complex
code than php_combined_lcg(), so this is expected.
Even if we accept a) that the default parameters should be changed, and b)
that the source for "more entropy" should be changed, I'm not clear why the
output format also needs to change. Is there some reason the output of
php_random_bytes() can't be encoded into decimal digits, rather than [0-v]?
If I encode php_random_bytes() to the same length of digits, it does
not increase entropy space. It remains about a million (a little less
than 10 bits). It's too small for current baseline.
Proposed code has 50 bits entropy. Besides php_combined_lcg() is based
on system time. Therefore, it is extremely poor entropy source for
uniqid() which generates timestamp string. It's more than a million
times better entropy than now. Since uniqid() has timestamp string
prefix, collision is unlikely and very close to 0, it's much more
reliable than now.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
I pasted simple benchmark to the PR.
New code uses about 2x cpu time on my Fedora 24. CSPRNG uses more complex
code than php_combined_lcg(), so this is expected.
To me, this is at least as important as changing the length and
character range of the output.
If I encode php_random_bytes() to the same length of digits, it does
not increase entropy space. It remains about a million (a little less
than 10 bits). It's too small for current baseline.
Not enough entropy for what? Can you give some concrete scenarios where
you see this being a problem?
To me, uniqid() is useful because it is a quick way of getting a short
string that's likely to be fairly unique. If that is its purpose, then
making it slower, and its output longer, are not helping anybody.
If it's purpose is to be truly random, and have controllable entropy,
etc, then we might as well deprecate it in favour of random_bytes().
Regards,
Rowan Collins
[IMSoP]
Hi Rowan,
To me,
uniqid()is useful because it is a quick way of getting a short
string that's likely to be fairly unique. If that is its purpose, then
making it slower, and its output longer, are not helping anybody.If it's purpose is to be truly random, and have controllable entropy, etc,
then we might as well deprecate it in favour ofrandom_bytes().
Reasonable.
Then enable more entropy and use php_random_byte() for it.
Question is what format:
- the same as now (digits and .)
- convert alphanumeric [0-v]
- convert to hex [0-f]
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.
I had read it, of course. But I could not understand why you chose BC
break way.
Now, I understand your intention to change default value of
"more_entropy" despite of BC break. You do want to change the default
behavior of uniqid.
But I cannot agree.
--
Kazuo Oishi
Hi,
IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.I had read it, of course. But I could not understand why you chose BC
break way.Now, I understand your intention to change default value of
"more_entropy" despite of BC break. You do want to change the default
behavior of uniqid.But I cannot agree.
--
Kazuo Oishi--
Full ACK on what Kazuo said.
I use uniqid() daily in my test suites, even new ones (not much else),
and introducing the . everywhere will just cause issues.
Changing string length may also break everything.
Marco Pivetta
Hi Marco,
Full ACK on what Kazuo said.
I use
uniqid()daily in my test suites, even new ones (not much else), and
introducing the.everywhere will just cause issues.
Changing string length may also break everything.
It seems you have code that will be broken.
Could you share the code and what it does?
Thank you.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
I use
uniqid()daily in my test suites, even new ones (not much else), and
introducing the.everywhere will just cause issues.
Changing string length may also break everything.It seems you have code that will be broken.
Could you share the code and what it does?
BTW, my proposed code does not use "." at all.
More entropy is [0-v]{10} from php_random_bytes().
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Kazuo,
IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.I had read it, of course. But I could not understand why you chose BC
break way.
IMHO, 10 bits (about a million) entropy is not considered enough
entropy, do you?
How serious BC is? It's much less impact than using mt_rand() all over
the code. i.e. rand() and mt_rand() is predictable random generator.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.I had read it, of course. But I could not understand why you chose BC
break way.IMHO, 10 bits (about a million) entropy is not considered enough
entropy, do you?
Do you say about extra part which is added by "more_entropy" option?
Current "more_entropy" part (10 bytes) pattern is "n.nnnnnnnn" and its
variation is 10^9 (1 billion) as written in your RFC. (about 30bits?)
I think it is enough to avoid collision in the same usec, for
non-security purpose.
How serious BC is?
You should already know that this BC-breack breaks existing
valid PHP codes in some situation. (DB error, test failure, etc.)
BC-breack may be acceptable if the change is clearly greate improvement
or obviously necessary. But this change is not, I think.
It's much less impact than using
mt_rand()all over
the code. i.e.rand()andmt_rand()is predictable random generator.
Sorry, I cannot understand what you say... (Why mt_rand?)
--
Kazuo Oishi
IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.I had read it, of course. But I could not understand why you chose BC
break way.IMHO, 10 bits (about a million) entropy is not considered enough
entropy, do you?Do you say about extra part which is added by "more_entropy" option?
Current "more_entropy" part (10 bytes) pattern is "n.nnnnnnnn" and its
variation is 10^9 (1 billion) as written in your RFC. (about 30bits?)I think it is enough to avoid collision in the same usec, for
non-security purpose.
Oops. Thank you for the correction :) I'll fix the RFC.
Actually not with current implementation. If time is rewined, the same ID
could be generated by chance. Because, both ID part and entropy part
is generated based on the current system time. Collision would be rare,
but it's not rare as it should be at all.
I think minimum would be 64 bits, 128 at least, 256 bits recommended.
How about use 2nd parameter for entropy length? 0 for none, 1 for
default, 10 to 255 chars.
Then user can have choice for prefered number of entropy chars.
How serious BC is?
You should already know that this BC-breack breaks existing
valid PHP codes in some situation. (DB error, test failure, etc.)BC-breack may be acceptable if the change is clearly greate improvement
or obviously necessary. But this change is not, I think.
I do think this is needed.
Let's not please security audit companies. Use of current uniqid() in
security sensitive context is fatal because it is too easy to predict
generated ID even with "more_entropy". Letting make such mistake
moderate is worth the change.
It's much less impact than using
mt_rand()all over
the code. i.e.rand()andmt_rand()is predictable random generator.Sorry, I cannot understand what you say... (Why mt_rand?)
The change have been done.
https://wiki.php.net/rfc/rng_fixes
IMHO, this RFC's BC is nothing compare to this.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Kazuo,
Current "more_entropy" part (10 bytes) pattern is "n.nnnnnnnn" and its
variation is 10^9 (1 billion) as written in your RFC. (about 30bits?)I think it is enough to avoid collision in the same usec, for
non-security purpose.Oops. Thank you for the correction :) I'll fix the RFC.
Oops again. I wrote correctly in RFC. Typo was in mail.
How serious BC is?
You should already know that this BC-breack breaks existing
valid PHP codes in some situation. (DB error, test failure, etc.)BC-breack may be acceptable if the change is clearly greate improvement
or obviously necessary. But this change is not, I think.I do think this is needed.
Let's not please security audit companies. Use of current
uniqid()in
security sensitive context is fatal because it is too easy to predict
generated ID even with "more_entropy". Letting make such mistake
moderate is worth the change.
In short, making PHP be more secure platform (tolerant even for
mistakes) matter to me.
This BC is nothing compared to mt_rand() everywhere.
Anyway, let's talk BC with real code.
I didn't look into all, but only briefly.
https://searchcode.com/?q=uniqid&loc=0&loc2=10000&lan=24
I could only find one code that tests uniqid() return value to test uniqid() (?)
Other than that, almost all code does not care about uniqid() return
value at all.
Who cares about uniqid() return value? for what purpose? other than testing
uniqid() itself?
Even if some test code breaks, does it worth than making PHP be more
secure platform?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Actually not with current implementation. If time is rewined, the same ID
could be generated by chance. Because, both ID part and entropy part
is generated based on the current system time. Collision would be rare,
but it's not rare as it should be at all.I think minimum would be 64 bits, 128 at least, 256 bits recommended.
How about use 2nd parameter for entropy length? 0 for none, 1 for
default, 10 to 255 chars.Then user can have choice for prefered number of entropy chars.
I think, uniqid should be left as is.
How serious BC is?
You should already know that this BC-break breaks existing
valid PHP codes in some situation. (DB error, test failure, etc.)BC-break may be acceptable if the change is clearly great improvement
or obviously necessary. But this change is not, I think.I do think this is needed.
Let's not please security audit companies. Use of current
uniqid()in
security sensitive context is fatal because it is too easy to predict
generated ID even with "more_entropy". Letting make such mistake
moderate is worth the change.
Misuse is just a misuse. The function uniqid() will not be a secure
function to be able to use in security sensitive context even if this
RFC is passed, isn't it?
In short, making PHP be more secure platform (tolerant even for
mistakes) matter to me.
IMO, THIS change is not important enough and not effective enough to do
in BC break way.
This BC is nothing compared to
mt_rand()everywhere.
It would be worth enough.
(cryptographically secure random > predictive by seed)
Anyway, let's talk BC with real code.
I didn't look into all, but only briefly.
https://searchcode.com/?q=uniqid&loc=0&loc2=10000&lan=24
Why didn't you...
You said "BC (BC break) will be minimum"...
I could only find one code that tests
uniqid()return value to testuniqid()(?)
Example:
https://github.com/BrianPrz/worklist/blob/master/classes/Login.class.php
In this class, output of uniqid() is saved to DB (maybe).
If 'token' field has only 13 length, output length change cause problem.
Other than that, almost all code does not care about
uniqid()return
value at all.
Who cares aboutuniqid()return value? for what purpose? other than testing
uniqid()itself?
The uniqid() manual explicitly say,
-
default value of more_entropy is false
-
the returned string will be 13 characters long. If more_entropy is
TRUE, it will be 23 characters. -
if more_entropy is set to TRUE,
uniqid()will add additional entropy
(using the combined linear congruential generator) at the end of the
return value
http://php.net/manual/en/function.uniqid.php
So, it is fairly valid to design
- field length limit to 13 chars in validation code or DB column.
- acceptable character type limit to "[0-9A-Za-z]+".
- expect increasing value
in their PHP applications.
--
Kazuo Oishi
Hi Kazuo,
The
uniqid()manual explicitly say,
default value of more_entropy is false
the returned string will be 13 characters long. If more_entropy is
TRUE, it will be 23 characters.if more_entropy is set to TRUE,
uniqid()will add additional entropy
(using the combined linear congruential generator) at the end of the
return valuehttp://php.net/manual/en/function.uniqid.php
So, it is fairly valid to design
- field length limit to 13 chars in validation code or DB column.
- acceptable character type limit to "[0-9A-Za-z]+".
- expect increasing value
in their PHP applications.
It's legacy design.
php_combined_lcg() must not be used, especially functions like
uniqid(). i.e. It's supposed to generate unique ID based on time, but
php_combined_lcg() generates pseudo random from current time.
It's more than obvious it's legacy and obsolete today.
Anyway, let's talk BC with real code.
I didn't look into all, but only briefly.
https://searchcode.com/?q=uniqid&loc=0&loc2=10000&lan=24Why didn't you...
You said "BC (BC break) will be minimum"...
Why should I look into all?
Show me the real code that breaks if you insist this minor BC matters.
I can think of number of way to be broken, but I cannot imagine real
production codes that are broken by change.
BTW, the current manual states uniqid() return unique identifier.
This is false.
I updated the manual, but we should do better job to generate almost unique ID.
<refsect1 role="returnvalues"> &reftitle.returnvalues; <para> - Returns the unique identifier, as a string. + Returns timestamp based unique identifier as a string. </para> + <warning> + <para> + This function tries to create unique identifier, but it does not + guarantee 100% uniqueness of return value. + </para> + </warning> </refsect1>Current implementation is good enough for most cases, but it can be better.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
The
uniqid()manual explicitly say,
default value of more_entropy is false
the returned string will be 13 characters long. If more_entropy is
TRUE, it will be 23 characters.if more_entropy is set to TRUE,
uniqid()will add additional entropy
(using the combined linear congruential generator) at the end of the
return valuehttp://php.net/manual/en/function.uniqid.php
....
It's legacy design.php_combined_lcg() must not be used, especially functions like
uniqid(). i.e. It's supposed to generate unique ID based on time, but
php_combined_lcg() generates pseudo random from current time.It's more than obvious it's legacy and obsolete today.
I agree that uniqid() is legacy design API.
And,
Current implementation is good enough for most cases, but it can be better.
I agree this legacy design API works good enough for most cases.
So, I think it should not be changed in BC break way.
--
Kazuo Oishi
Hi Kauzo,
Current implementation is good enough for most cases, but it can be better.
I agree this legacy design API works good enough for most cases.
So, I think it should not be changed in BC break way.
I updated the RFC.
2nd parameter (more_entropy) is int now.
- 0 for disable more entropy.
(Compatible with current $more_entropy=FALSE) - 1 for 10 digits entropy. e.g. 1.23456789
(Compatible with current $more_entropy=TRUE) DEFAULT - 13 to 255 to number of entropy [0-v]{13,255} chars.
e.g. 1234abcdefghi (13 = 65 bits)
65 bits entropy + timestamp will provide good enough uniqueness for
most usage.
More secure default may be future scope, but attack against misused
code will be much harder by default as a bonus.
Default could be more secure by using [0-v]+.
Marco does not like "." in default output.
I would like to choose default from discussion (or make some vote choices)
Current behavior is not an option because whole point of this RFC is
to make uniqid() to return unique ID almost always even when system
clock is adjusted.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
I updated the RFC.
2nd parameter (more_entropy) is int now.
- 0 for disable more entropy.
(Compatible with current $more_entropy=FALSE)- 1 for 10 digits entropy. e.g. 1.23456789
(Compatible with current $more_entropy=TRUE) DEFAULT- 13 to 255 to number of entropy [0-v]{13,255} chars.
e.g. 1234abcdefghi (13 = 65 bits)
65 bits entropy + timestamp will provide good enough uniqueness for
most usage.More secure default may be future scope, but attack against misused
code will be much harder by default as a bonus.Default could be more secure by using [0-v]+.
Marco does not like "." in default output.I would like to choose default from discussion (or make some vote choices)
Basically, I will not oppose if backward compatibility is kept (default
$number_of_entropy_chars = 0). I have no opinion about specifying
length of entropy chars.
However, I don't think this new 2nd parameter design is good.
-
It is not natural (or straightforward) to specify 1 as
parameter named $number_of_entropy_chars, to use 10 digits
entropy ($more_entropy=TRUE compatible output). -
Why number of new style entropy ([0-9a-v]+) starts with 13?
(Why not 2 or 11?) -
Why max number of entropy is 255? (32^255 = 1275 bits)
(Ease of implementation?)
And, what will happen when 2-12, greater than 255, or negative
value is specified?
--
Kazuo Oishi
Hi Kazuo,
I updated the RFC.
2nd parameter (more_entropy) is int now.
- 0 for disable more entropy.
(Compatible with current $more_entropy=FALSE)- 1 for 10 digits entropy. e.g. 1.23456789
(Compatible with current $more_entropy=TRUE) DEFAULT- 13 to 255 to number of entropy [0-v]{13,255} chars.
e.g. 1234abcdefghi (13 = 65 bits)
65 bits entropy + timestamp will provide good enough uniqueness for
most usage.More secure default may be future scope, but attack against misused
code will be much harder by default as a bonus.Default could be more secure by using [0-v]+.
Marco does not like "." in default output.I would like to choose default from discussion (or make some vote choices)
Basically, I will not oppose if backward compatibility is kept (default
$number_of_entropy_chars = 0). I have no opinion about specifying
length of entropy chars.However, I don't think this new 2nd parameter design is good.
It is not natural (or straightforward) to specify 1 as
parameter named $number_of_entropy_chars, to use 10 digits
entropy ($more_entropy=TRUE compatible output).Why number of new style entropy ([0-9a-v]+) starts with 13?
(Why not 2 or 11?)Why max number of entropy is 255? (32^255 = 1275 bits)
(Ease of implementation?)And, what will happen when 2-12, greater than 255, or negative
value is specified?
I'm going to propose more compatible fix that does not change
length of entropy chars nor parameter type.
Anyway, I was thinking to raise error for any invalid numbers.
I'm going to propose
A. Current format (digits and . e.g. 1.23456788)
B. HEX format ([0-9a-f]{10})
I'll make these vote options.
A is compatible with when $more_entropy=TRUE.
B uses compatible chars without $more_entropy (=FALSE).
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
This is RFC for improving
uniqid()uniqueness.
https://wiki.php.net/rfc/uniqidPR
https://github.com/php/php-src/pull/2123If there is anything left to discuss, please comment.
Regards,
Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.
https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede
I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.
Any comments?
If there is no objections, I'll apply this few days later.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede
I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.Any comments?
If there is no objections, I'll apply this few days later.
If you need comments on a patch, send a PR?
Marco Pivetta
Hi Yasuo
2016-10-02 20:56 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:
I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.Any comments?
If there is no objections, I'll apply this few days later.
If anything this should be considered from 7.1+, I don't think we
should change uniqid() mid life time of a branch, ccing Anatol and
Davey
--
regards,
Kalle Sommer Nielsen
kalle@php.net
Hi all,
Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede
I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.Any comments?
If there is no objections, I'll apply this few days later.
Updated patch a little
https://gist.github.com/yohgaki/cbe5431f9d072b57af2883a2b5745195
Exception should not be ignored, but added few lines for this.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede
I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.Any comments?
If there is no objections, I'll apply this few days later.Updated patch a little
https://gist.github.com/yohgaki/cbe5431f9d072b57af2883a2b5745195
Exception should not be ignored, but added few lines for this.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net--
I'm curious, did you consider using random_int? It already handles
biasing, and you can reduce the repeated calls to random_bytes.
Hi Leigh,
I'm curious, did you consider using random_int? It already handles
biasing, and you can reduce the repeated calls to random_bytes.
Yes. It seemed it might be slower due to number of retries at first,
but I realized that it isn't later.
It could be something like
$entropy = random_int(10000000000, 9999999999);
$entropy[1] = '.';
$uniqid = timestamp . $entropy;
I don't have particular preference.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Leigh,
I'm curious, did you consider using random_int? It already handles
biasing, and you can reduce the repeated calls to random_bytes.Yes. It seemed it might be slower due to number of retries at first,
but I realized that it isn't later.It could be something like
$entropy = random_int(10000000000, 9999999999);
$entropy[1] = '.';
$uniqid = timestamp . $entropy;I don't have particular preference.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Since we want to preserve BC
entropy = random_int(0, 99999999);
uniqid = strpprintf(0, "%s%08x%05x.%08d", prefix, sec, usec, entropy);
Hi all,
Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede
I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.Any comments?
If there is no objections, I'll apply this few days later.
Yasuo,
This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.
Please check with the RMs before merging functionality changes into release
branches. All functionality changes need consent and consensus. Bug fixes
(that don't change functionality or break BC) do not.
I understand your desire to fix these things, especially the security
related type stuff, but as a group we have a responsibility to create
predictable, sane, and safe (as in, don't break stuff) migration paths when
we can. A history of doing this is WHY php is still going strong after so
long.
Thanks,
- Davey
Hi Davey,
Hi all,
Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede
I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.Any comments?
If there is no objections, I'll apply this few days later.Yasuo,
This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.Please check with the RMs before merging functionality changes into release
branches. All functionality changes need consent and consensus. Bug fixes
(that don't change functionality or break BC) do not.I understand your desire to fix these things, especially the security
related type stuff, but as a group we have a responsibility to create
predictable, sane, and safe (as in, don't break stuff) migration paths when
we can. A history of doing this is WHY php is still going strong after so
long.Thanks,
I agree fully.
The only case this patch could break code is caused by broken PRNG in
the system which is fatal anyway. i.e. If PRNG is broken, session
module/randon_*() cannot produce secure session ID/values. We don't
have to worry about changed behavior/BC.
The main motivation is to simply this RFC discussion. I'll commit this
patch master only.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Davey,
Hi all,
On Mon, Oct 3, 2016 at 3:56 AM, Yasuo Ohgaki yohgaki@ohgaki.net
wrote:Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede
I would like to apply this patch from PHP 7.0 branch, then discuss
what
the default should be.Any comments?
If there is no objections, I'll apply this few days later.Yasuo,
This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.Please check with the RMs before merging functionality changes into
release
branches. All functionality changes need consent and consensus. Bug
fixes
(that don't change functionality or break BC) do not.I understand your desire to fix these things, especially the security
related type stuff, but as a group we have a responsibility to create
predictable, sane, and safe (as in, don't break stuff) migration paths
when
we can. A history of doing this is WHY php is still going strong after
so
long.Thanks,
I agree fully.
The only case this patch could break code is caused by broken PRNG in
the system which is fatal anyway. i.e. If PRNG is broken, session
module/randon_*() cannot produce secure session ID/values. We don't
have to worry about changed behavior/BC.The main motivation is to simply this RFC discussion. I'll commit this
patch master only.
Did you solve any of the issues we discussed here? Some of them are BC
breaks.
Thanks
Pierre
Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 4. Okt. 2016, 03:54:
Hi Davey,
Hi all,
On Mon, Oct 3, 2016 at 3:56 AM, Yasuo Ohgaki yohgaki@ohgaki.net
wrote:Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede
I would like to apply this patch from PHP 7.0 branch, then discuss
what
the default should be.Any comments?
If there is no objections, I'll apply this few days later.Yasuo,
This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.Please check with the RMs before merging functionality changes into
release
branches. All functionality changes need consent and consensus. Bug fixes
(that don't change functionality or break BC) do not.I understand your desire to fix these things, especially the security
related type stuff, but as a group we have a responsibility to create
predictable, sane, and safe (as in, don't break stuff) migration paths
when
we can. A history of doing this is WHY php is still going strong after so
long.Thanks,
I agree fully.
The only case this patch could break code is caused by broken PRNG in
the system which is fatal anyway. i.e. If PRNG is broken, session
module/randon_*() cannot produce secure session ID/values. We don't
have to worry about changed behavior/BC.The main motivation is to simply this RFC discussion. I'll commit this
patch master only.
It still needs a RFC.
Regards, Niklas
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net