Skip to content
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.

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

Support random_bytes() as an additional randomness source #6

Open
wants to merge 6 commits into
base: 1.0
Choose a base branch
from
Open

Conversation

kerwus
Copy link

@kerwus kerwus commented Feb 28, 2019

No description provided.

Copy link
Owner

@mariuswilms mariuswilms left a comment

Choose a reason for hiding this comment

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

Hey, thanks for trying to add a more contemporary source for randomness and for submitting this PR. The actual feature addition probably only needs a couple of lines, but the PR as it is, is a little noisey. Can you fix this?

@@ -0,0 +1,28 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Owner

Choose a reason for hiding this comment

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

Your editors configuration directory should not be added to project. Please modifiy this PR so that it only contains the actual feature changes/additions. You've probably added this by mistake, using a global .gitignore file will help. https://help.github.com/en/articles/ignoring-files#create-a-global-gitignore

composer.json Outdated
@@ -1,5 +1,5 @@
{
"name": "atelierdisko/coupon_code",
"name": "HYP3/coupon_code",
Copy link
Owner

Choose a reason for hiding this comment

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

There should be no changes to the composer.json file necessary. I guess this is a leftover from local development.

* @param integer $bytes Number of bytes to return.
* @return string
* @throws Exception
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Don't change the identation style

protected function _random($bytes) {
if (is_readable('/dev/urandom')) {
Copy link
Owner

Choose a reason for hiding this comment

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

If you run into openbasedir issues with this line, than your openbasedir configuration needs to be changed as it is too strict. It is safe to generally include /dev/urandom.

$stream = fopen('/dev/urandom', 'rb');
$result = fread($stream, $bytes);
//if (is_readable('/dev/urandom')) {
if ($fh = @fopen('/dev/urandom', 'rb')) {
Copy link
Owner

Choose a reason for hiding this comment

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

Errors should not be surpressed.

if (function_exists('mcrypt_create_iv')) {
return mcrypt_create_iv($bytes, MCRYPT_DEV_RANDOM);
return random_bytes($bytes);
Copy link
Owner

Choose a reason for hiding this comment

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

This line will not be reachable unless the mcrypt extension is installed, which is not what you want. How about adding a similar block as a first possible source of randomness in our _random() method here? That'd come before the check for using /dev/urandom. Before being able to use random_bytes() check if the function is present, older PHP versions may not have it and we still want to support those.

@mariuswilms mariuswilms changed the title Hyp3 Support random_bytes() as an additional randomness source Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants