Fix #6103 #6104

zsturgess · 2016-01-04T16:42:10Z

Q	A
Doc fix?	yes
New docs?	no
Applies to	>= 2.8
Fixed tickets	#6103

Re-written this page from talking about the deprecated SecureRandom class to talking about the random_bytes replacement.

javiereguiluz · 2016-01-04T16:45:43Z

components/security/secure_tools.rst

Is polyfill word common enough to use it?

I would argue in the world of programming it is, but I'll happily replace with an alternative if desired. fail-safe? fallback?

Should I also mention that the polyfill is provided by the random_compat package, or leave it open?

xabbuh · 2016-01-04T16:59:40Z

What do you think if we didn't talk about the SecureRandom class in all versions of the docs given that the Symfony polyfills (as well as the paragonie/random-compat package) provide implementations for all supported PHP versions?

zsturgess · 2016-01-04T17:04:56Z

@xabbuh Whichever you think is better here.

I've placed against the 2.8 branch since that is when SecureRandom was deprecated and paragonie/random-compat was "introduced" as a dependency of the Security component.

Using it on older Symfony versions would require installing this as a dependency, which at that point isn't really documenting the Security component any more, but happy to amend this if that is what is desired.

xabbuh · 2016-01-04T17:09:09Z

Yeah, let's wait what the others think (maybe just adding a tip in older versions is enough).

zsturgess · 2016-01-04T23:32:02Z

@xabbuh Another question: How would you feel about renaming this page? The title is Generating a Secure random Number, but the article is really about Generating a Secure random String

Pardon my ignorance of the technology driving the docs here, but would I need to do anything more than change the title on this page?

OskarStark · 2016-01-07T09:15:39Z

components/security/secure_tools.rst

Would write:

PHP 7 and up .... natively, for older versions we provide a polyfill.

polyfill should be a link to more information IMO

Changed the wording here, not exactly as per your suggestion, but to add more details. I've just linked to the github pages for now, when #6052 is complete, these links can be updated.

zsturgess · 2016-01-09T23:15:59Z

@xabbuh Giving this a bump as, other than my title question above, I think this is just waiting on the docs team to decide about your idea for pre-2.8 versions.

javiereguiluz · 2016-01-11T07:38:10Z

I like the new doc a lot and I also like @xabbuh's proposal to "backport" these changes to older versions. Thanks @zsturgess.

zsturgess · 2016-01-11T09:55:15Z

@javiereguiluz Would you like me to change the base of this pull req to the 2.3 branch, or are you planning something a little different for those versions in a separate pull req?

xabbuh · 2016-01-11T10:04:29Z

@zsturgess Imo we can just make the changes here as general as possible, merge the changes into the 2.3 branch (we can switch the branch when merging) and maybe remove some unneeded stuff when merging branches up.

zsturgess · 2016-01-11T10:10:05Z

OK, I will add a "versionadded" hint to instruct users to add paragonie/random_compat as a dependancy on older versions.

Is there anything else you'd like? What do you think of my title proposal, above?

xabbuh · 2016-01-12T08:50:18Z

@zsturgess It makes sense to update the title with your suggestion.

conradkleinespel · 2016-01-22T10:57:27Z

Given that random_bytes is displayed PHP 7 only on php.net, I think having this page describe how to generate a random number is very good.

Coming from Symfony 2.8, upgrading to 3.0, I would have loved to see @zsturgess's changes. Would have saved me a few minutes of looking through the Security component changelog and figuring out that Symfony had a PHP 7 polyfill.

zsturgess · 2016-01-22T11:47:54Z

Thanks @conradkleinespel !

I think this PR is pretty much ready, @xabbuh mentioned changing the base when merging this PR, so I haven't altered it, but can change it to 2.3 if desired.

xabbuh · 2016-01-22T11:58:01Z

components/security/secure_tools.rst

"a secure random" sounds incomplete to me. I suggest something like a Secure Random String.

People might search for "random number symfony" on search engines though. So it does make sense to me to keep "number" in there. Or else "a secure random string or number".

But either way, it would be good to have this merged as people move from 2.8 to 3.0. And maybe make a new PR with this change once we know what we want. Having PR merged is better than leaving the doc in the outdated state, even if this text is not perfect for everyone, IMO.

I've added documentation for random_int that should therefore address both of your concerns.

xabbuh · 2016-01-30T19:05:00Z

👍

stof · 2016-02-01T19:45:36Z

This should go in the 2.3 branch too, as we know include the polyfill in 2.3 too since the latest 2.3 release.

stof · 2016-02-01T19:47:03Z

components/security/secure_tools.rst

md5 is actually a bad idea, as it would make the string less secure (due to md5 colisions). Using base64_encode (as done in Symfony for CSRF tokens) is a much better idea.

It's OK to use base64_encode, but I don't think "md5 collisions" is a good argument against MD5. According to this, the probability of a collision is around 2.7×10^-20.

I would personally use base64_encode if I were to do this, so I'm tempted to update this anyway. I didn't touch this originally to keep the diff small, but I think we're a little beyond that now :)

zsturgess · 2016-02-02T17:17:52Z

@stof How would you like to go about this. Shall I open a new PR, or would you like to switch the base when you merge? I'm wary that opening a new PR loses the chain of comments here. /cc @xabbuh

xabbuh · 2016-02-02T19:47:25Z

@zsturgess We can easily switch the branch when merging.

zsturgess · 2016-02-02T22:54:05Z

Thanks for all the help and comments, @stof @xabbuh - I believe all your concerns have now been addressed and this is ready for review again.

This PR was submitted for the 2.8 branch but it was merged into the 2.3 branch instead (closes #6104). Discussion ---------- Fix #6103 | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | >= 2.8 | Fixed tickets | #6103 Re-written this page from talking about the deprecated SecureRandom class to talking about the random_bytes replacement. Commits ------- 984c49e Fix #6103

wouterj · 2016-02-06T10:59:15Z

Thank you for this PR & help @zsturgess! It's a lot better now.

I've merged your PR into the 2.3 branch with 932530f and did some minor formatting fixes in d6958d6. Thanks again!

This PR was merged into the 2.3 branch. Discussion ---------- Use hash_equals instead of StringUtils::equals | Q | A | --- | --- | doc fix? | yes | new docs? | yes | applies to | 2.3+ | Fixed tickets | - We've merged #6104 in the 2.3 branch, so I think we should use the `hash_equals` function the 2.3 docs as well. Now, this article isn't related to the Security component at all and it might be a good start for the Polyfill component. Commits ------- 9f7f1dd Use hash_equals instead of StringUtils::equals

javiereguiluz reviewed Jan 4, 2016
View reviewed changes

zsturgess force-pushed the secure-random-usage branch from e71b992 to ce8b068 Compare January 4, 2016 17:08

zsturgess force-pushed the secure-random-usage branch from ce8b068 to fe7b609 Compare January 4, 2016 17:14

OskarStark reviewed Jan 7, 2016
View reviewed changes

zsturgess force-pushed the secure-random-usage branch 2 times, most recently from e4258e2 to e5336da Compare January 7, 2016 11:06

zsturgess force-pushed the secure-random-usage branch from e5336da to 99dae89 Compare January 11, 2016 10:14

zsturgess force-pushed the secure-random-usage branch 2 times, most recently from 034f740 to 4adda40 Compare January 12, 2016 09:37

xabbuh reviewed Jan 22, 2016
View reviewed changes

zsturgess force-pushed the secure-random-usage branch 3 times, most recently from e9ada96 to fbb8251 Compare January 22, 2016 19:55

xabbuh added the Status: Reviewed label Jan 30, 2016

xabbuh mentioned this pull request Feb 1, 2016

Remove "Securely Generating Random Numbers" #6214

Closed

stof reviewed Feb 1, 2016
View reviewed changes

zsturgess force-pushed the secure-random-usage branch from fbb8251 to 05fb331 Compare February 2, 2016 17:14

Fix symfony#6103

3808582

zsturgess force-pushed the secure-random-usage branch from 05fb331 to 3808582 Compare February 2, 2016 22:50

wouterj added a commit that referenced this pull request Feb 6, 2016

[#6104] Minor formatting fixes

d6958d6

wouterj closed this Feb 6, 2016

wouterj mentioned this pull request Feb 6, 2016

Use hash_equals instead of StringUtils::equals #6231

Merged

zsturgess deleted the secure-random-usage branch February 6, 2016 12:51

Uh oh!

Fix #6103 #6104

Fix #6103 #6104

Uh oh!

Conversation

zsturgess commented Jan 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Jan 4, 2016

Uh oh!

zsturgess commented Jan 4, 2016

Uh oh!

xabbuh commented Jan 4, 2016

Uh oh!

zsturgess commented Jan 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsturgess commented Jan 9, 2016

Uh oh!

javiereguiluz commented Jan 11, 2016

Uh oh!

zsturgess commented Jan 11, 2016

Uh oh!

xabbuh commented Jan 11, 2016

Uh oh!

zsturgess commented Jan 11, 2016

Uh oh!

xabbuh commented Jan 12, 2016

Uh oh!

conradkleinespel commented Jan 22, 2016

Uh oh!

zsturgess commented Jan 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Jan 30, 2016

Uh oh!

stof commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsturgess commented Feb 2, 2016

Uh oh!

xabbuh commented Feb 2, 2016

Uh oh!

zsturgess commented Feb 2, 2016

Uh oh!

wouterj commented Feb 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants