Skip to content

Deprecate the SecureRandom class #15879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 6, 2015
Merged

Deprecate the SecureRandom class #15879

merged 1 commit into from
Oct 6, 2015

Conversation

pierredup
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Deprecate the Symfony\Component\Security\Core\Util\SecureRandom and encourage the usage of the random_bytes function instead

@pierredup pierredup mentioned this pull request Sep 23, 2015
Merged
@pierredup pierredup force-pushed the deprecate_secure_random branch 2 times, most recently from 8018c29 to 00e5013 Compare September 24, 2015 07:20
@stof
Copy link
Member

stof commented Sep 24, 2015

This implementation is broken: the Symfony codebase still use the deprecated function, meaning it triggers deprecation warnings for everyone (and this is why the testsuite is failing)

@pierredup pierredup mentioned this pull request Sep 24, 2015
Closed
@pierredup
Copy link
Contributor Author

@stof What would be the best way to deprecate this class then? Should the dependency on paragonie/random_compat be added here already, with all the instances that use the SecureRandom class be updated to use the function instead? That way the class and interface can be deprecated with all the internal code using the new function. Or is there a better way to do the deprecation?

@stof
Copy link
Member

stof commented Sep 24, 2015

@pierredup yes, this is the way it should be done. A deprecated API should not be used anymore. If you cannot avoid using it, it cannot be a deprecated API.

derrabus
derrabus reviewed Sep 24, 2015
View reviewed changes
src/Symfony/Component/Security/Core/composer.json
@@ -33,7 +33,8 @@
"symfony/http-foundation": "",
"symfony/validator": "For using the user password constraint",
"symfony/expression-language": "For using the expression voter",
"ircmaxell/password-compat": "For using the BCrypt password encoder in PHP <5.5"
"ircmaxell/password-compat": "For using the BCrypt password encoder in PHP <5.5",
"paragonie/random_compat": "For secure number generation"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, we should document that this library is only a compatibility layer for PHP 5: "For secure random number generation in PHP 5.x"

@derrabus
Copy link
Member

Status: Needs Work

@pierredup pierredup force-pushed the deprecate_secure_random branch 5 times, most recently from b527c03 to 5077d83 Compare September 25, 2015 09:05
@pierredup
Copy link
Contributor Author

I think this PR is ready /cc @stof @fabpot

The travis failures seems unrelated

Status: Needs Review

@stof
Copy link
Member

stof commented Sep 25, 2015

I would like to see our testsuite being fixed and then rerunning the builds for this PR, to ensure things work properly though.

@pierredup
Copy link
Contributor Author

Can the build for this be re-run? Or should I do a push to trigger new builds?

@stof
Copy link
Member

stof commented Sep 29, 2015

@pierredup there are conflicts preventing the merge, so this needs a rebase anyway. After pushing the rebased version, a new build with run.

@pierredup pierredup force-pushed the deprecate_secure_random branch 2 times, most recently from 44a1d43 to 0d781d4 Compare September 29, 2015 12:52
xabbuh
xabbuh reviewed Sep 29, 2015
View reviewed changes
src/Symfony/Component/Security/CHANGELOG.md
@@ -12,6 +12,7 @@ CHANGELOG
`Symfony\Component\Security\Http\Authentication\SimpleFormAuthenticatorInterface` instead
* deprecated `Symfony\Component\Security\Core\Util\ClassUtils`, use
`Symfony\Component\Security\Acl\Util\ClassUtils` instead
* deprecated `Symfony\Component\Security\Core\Util\SecureRandom` class in favour of the `random_bytes` function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deprecated the [...]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in favor of

@pierredup pierredup force-pushed the deprecate_secure_random branch from 0c50c1f to 5bb8732 Compare October 3, 2015 17:21
@pierredup
Copy link
Contributor Author

Thanks for the info @stof

@pierredup
Copy link
Contributor Author

All comments have been addressed

@pierredup pierredup force-pushed the deprecate_secure_random branch from 5bb8732 to ee3f67a Compare October 3, 2015 17:23
@derrabus
Copy link
Member

derrabus commented Oct 4, 2015

Status: Reviewed

@xabbuh
Copy link
Member

xabbuh commented Oct 4, 2015

@pierredup You also have to update the SecureRandomTest class. The getSecureRandoms() data provider tries to modify the useOpenSsl property which no longer exists.

@xabbuh
Copy link
Member

xabbuh commented Oct 4, 2015

Status: Needs work

@pierredup
Copy link
Contributor Author

Thanks @xabbuh. Is it still necessary for the test, since the SecureRandom class is now basically only a wrapper for the random_bytes function? Can I remove the test, or do you prefer I just update it?

@xabbuh
Copy link
Member

xabbuh commented Oct 4, 2015

Imo you can simply drop the OpenSSL related part of that test.

@pierredup pierredup force-pushed the deprecate_secure_random branch from ee3f67a to 1afe984 Compare October 4, 2015 17:23
@derrabus
Copy link
Member

derrabus commented Oct 5, 2015

Status: Reviewed

@fabpot
Copy link
Member

fabpot commented Oct 5, 2015

👍 ping @symfony/deciders

Tobion
Tobion reviewed Oct 5, 2015
View reviewed changes
src/Symfony/Component/Security/Csrf/TokenGenerator/UriSafeTokenGenerator.php
* each token (in bits)
* Note: The $random parameter is deprecated since version 2.8 and will be removed in 3.0.
*
* @param SecureRandomInterface|int|null $random The random value generator used for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the deprecated parameter value should not be documented anymore

@Tobion
Copy link
Contributor

Tobion commented Oct 5, 2015

👍 apart from the above comments

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

should also be rebased to resolve conflict.

@pierredup pierredup force-pushed the deprecate_secure_random branch from 1afe984 to f02973a Compare October 6, 2015 18:09
@pierredup
Copy link
Contributor Author

Done

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

Thank you @pierredup.

@fabpot fabpot merged commit f02973a into symfony:2.8 Oct 6, 2015
fabpot added a commit that referenced this pull request Oct 6, 2015
@fabpot
feature #15879 Deprecate the SecureRandom class (pierredup)
141f9eb 
This PR was merged into the 2.8 branch.

Discussion
----------

Deprecate the SecureRandom class

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Deprecate the `Symfony\Component\Security\Core\Util\SecureRandom` and encourage the usage of the `random_bytes` function instead

Commits
-------

f02973a Deprecate the SecureRandom class
@fabpot fabpot mentioned this pull request Nov 16, 2015
Merged
@jrjohnson jrjohnson mentioned this pull request Jul 27, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pierredup @stof @derrabus @fabpot @xabbuh @Tobion @carsonbot