Because it is a Drupal-provided function and components cannot call them. Since drupal_random_bytes() is a simple utility function, I propose we move *it* to a Drupal\Component\Utility\RandomBytes.php and remove the function in bootstrap.inc.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Assigned: Unassigned » Dave Reid
Status: Active » Needs review
FileSize
20.72 KB

This moves drupal_random_bytes() to a new component and replaces all function calls.

longwave’s picture

Patch appears to contain unrelated changes in core/modules/field/field.attach.inc?

longwave’s picture

+      // PHP versions prior 5.3.4 experienced openssl_random_pseudo_bytes()
+      // locking on Windows and rendered it unusable.
+      if (!isset($php_compatible)) {
+        $php_compatible = version_compare(PHP_VERSION, '5.3.4', '>=');
+      }

This is presumably redundant now our minimum version is 5.3.5, although this should probably be dealt with separately.

Dave Reid’s picture

Unrelated changes gone. Yeah, we should probably address it as a follow-up. Just wanted to move around code only.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

This is simply moving code out of bootstrap.inc and into a component, looks good to me.

And if this is now a component that can be used outside of Drupal, I guess the version_compare() test should stay in place.

catch’s picture

Status: Reviewed & tested by the community » Needs work

It'd be good to have a follow-up for the version_compare() - while other projects might use the component it's OK for our components to have sensible PHP version requirements regardless. It'd need replacing with a comment though at least.

+  public static function generate($count) {
+    // $random_state does not use drupal_static as it stores random bytes.
+    static $random_state, $bytes, $php_compatible;

The reference to drupal_static() should go, it wouldn't be allowed to call that anyway.

I think the static variable/method is probably fine here at least for now. It could move to a property if we turned this into a service. Was there a specific reason not to do that or is that just a product of the straight conversion?

ParisLiakos’s picture

Title: Drupal\Component\Uuid\Php cannot call drupal_random_bytes() » Move drupal_random_bytes() to a RandomBytes component
Status: Needs work » Needs review
Issue tags: +PHPUnit Blocker
FileSize
14.62 KB

removed the comment and rerolled
i think maybe we should keep it like this for now, since this is used in mostly procedural code..and we should also add some unit tests..but lets do the conversion first?

dawehner’s picture

I'm wondering whether there is any kind of way to test this class.

All of them are nitpicky.

+++ b/core/includes/install.core.incundefined
@@ -1123,7 +1124,7 @@ function install_settings_form_submit($form, &$form_state) {
+    'value'    => drupal_hash_base64(RandomBytes::generate(55)),

I guess we could remove the spaces in that patch.

+++ b/core/lib/Drupal/Component/Utility/RandomBytes.phpundefined
@@ -0,0 +1,75 @@
+ * Definition of Drupal\Component\Utility\RandomBytes.

Should be \Contains

+++ b/core/lib/Drupal/Component/Utility/RandomBytes.phpundefined
@@ -0,0 +1,75 @@
+   * @param $count

... that's probably an int.

+++ b/core/lib/Drupal/Component/Utility/RandomBytes.phpundefined
@@ -0,0 +1,75 @@
+  }

empty line missing.

ParisLiakos’s picture

newline is there as far as i can see:)
and did not remove the space, cause there is consistent spacing like that in this function, and its out of scope cleaning it up all

dawehner’s picture

+  }
+}

should be

+  }
+
+}
ParisLiakos’s picture

ah, thats what you meant:)

RobLoach’s picture

+++ b/core/lib/Drupal/Component/Utility/RandomBytes.phpundefined
@@ -0,0 +1,76 @@
+   *   The number of characters (bytes) to return in the string.
+   */
+  public static function generate($count) {
+    static $random_state, $bytes, $php_compatible;
+    // Initialize on the first call. The contents of $_SERVER includes a mix of

Since we're using a default of 55 almost everywhere we use ::generate(), would it make sense to introduce a default of 55 in the function?

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.phpundefined
@@ -45,7 +46,7 @@
     // Generate and set a D7-compatible session cookie.
     $this->curlInitialize();
-    $sid = drupal_hash_base64(uniqid(mt_rand(), TRUE) . drupal_random_bytes(55));
+    $sid = drupal_hash_base64(uniqid(mt_rand(), TRUE) . RandomBytes::generate(55));

I see lots of use of generation of the session ID via:

  drupal_hash_base64(uniqid(mt_rand(), TRUE) . RandomBytes::generate(55));

If RandomBytes was named Random, then we could add more things related to ::generate in there, including the base 64 hash generator. Might be out of scope here though.

+++ b/core/modules/user/user.pages.incundefined
@@ -10,6 +10,7 @@
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 use Symfony\Component\HttpKernel\HttpKernelInterface;
+use Drupal\Component\Utility\RandomBytes;

Noticed that there arn't any tests for ::generate(). I understand that it just returns a random set of bytes, but that is still test-worthy, and this seems like a great chance to introduce some new PHPUnit tests.

If anyone has any objections, I'd love to put together a PHPUnit hit on this.

ParisLiakos’s picture

agreed on all points above, i ll try to roll a patch with them later

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned

sorry, seems i wont have time till next week

Crell’s picture

+++ b/core/lib/Drupal/Component/Utility/RandomBytes.php
@@ -0,0 +1,76 @@
+      // PHP versions prior 5.3.4 experienced openssl_random_pseudo_bytes()
+      // locking on Windows and rendered it unusable.
+      if (!isset($php_compatible)) {
+        $php_compatible = version_compare(PHP_VERSION, '5.3.4', '>=');

This is no longer relevant as we don't support that version of PHP.

ParisLiakos’s picture

Completely different patch, that moves the class to a service, turns static variables into properties and injects Request since we need $_SERVER.
Also added a phpunit test.
Lets leave the hash thing out for now, i guess

Status: Needs review » Needs work

The last submitted patch, drupal-randomBytes-1929288-16.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
16.99 KB

yeah have to register it on the install bundle as well

Status: Needs review » Needs work

The last submitted patch, drupal-randomBytes-1929288-18.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
17.44 KB

sigh..this will install

Status: Needs review » Needs work

The last submitted patch, drupal-randomBytes-1929288-20.patch, failed testing.

ParisLiakos’s picture

typo:/

ParisLiakos’s picture

Status: Needs work » Needs review

status

Status: Needs review » Needs work

The last submitted patch, drupal-randomBytes-1929288-22.patch, failed testing.

ParisLiakos’s picture

yeah well for this to work Uuid needs to be a service..so we either go with #11 or postpone for #1969572: Make Uuid a service

Dave Reid’s picture

Feels like we're adding complications by making this a service rather than a Drupal-agnostic component. Is the strategy that all these types of fuctions that don't really depend on Drupal are supposed to be services rather than components?

ParisLiakos’s picture

i have to agree about the complexicity
here is the patch in #11 rerolled, with the phpunit added

ParisLiakos’s picture

Status: Needs work » Needs review

sigh, i totally miss status updating today

ParisLiakos’s picture

and with correct docblocks:P

dawehner’s picture

+++ b/core/includes/install.core.incundefined
@@ -1138,7 +1139,7 @@ function install_settings_form_submit($form, &$form_state) {
+    'value'    => drupal_hash_base64(RandomBytes::generate(55)),

odd spaces. Let's fix that.

+++ b/core/lib/Drupal/Component/Utility/RandomBytes.phpundefined
@@ -0,0 +1,76 @@
+   * @param int $count
+   *   The number of characters (bytes) to return in the string.
...
+  public static function generate($count) {

Missing @return

ParisLiakos’s picture

there:)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

pwolanin’s picture

Crell asked in IRC if all these uses are security critical.

Looking at the patch, of the calls to get random bytes I see, maybe only the UUID one could get replaced with something like uniqid(mt_rand(), TRUE)

That's probably the main one that's a performance hit anyhow.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work
Crell’s picture

Title: Move drupal_random_bytes() to a RandomBytes component » Move cryptographic functions to Crypt component
Category: bug » task

rootawc, pwolanin, msonnabaum, and I just had a lengthy conversation in IRC about this issue and we're going to change direction a little.

1) We don't need a RandomBytes class. We need a Cyrpt class, which would include randomBytes() as well as hashBase64() and hmacBase64(). They come as a matched set.

2) The $_SERVER here is not being used in a contextual way, just in a "we need some random crap" way; $_SERVER is available in both HTTP and CLI SAPIs in PHP, so it's safe to use for that without having to inject anything. (It has a completely different set of random crap in each case, but it does have data.)

3) That said... UUID's use of random_bytes() isn't necessary anyway since that doesn't need to be a cryptographically strong string. It can just be uniquid().

rootawc is working on a new patch that does the above 3 items. Coming soon to an issue near you!

Damien Tournoud’s picture

3) That said... UUID's use of random_bytes() isn't necessary anyway since that doesn't need to be a cryptographically strong string. It can just be uniquid().

Please, don't. Those should stay unpredictable, there is no way to know how they are going to be used down the line. Also, this is not in the critical path.

From the patch, we might also consider simplifying this:

-  session_id(drupal_hash_base64(uniqid(mt_rand(), TRUE) . drupal_random_bytes(55)));
+  session_id(drupal_hash_base64(uniqid(mt_rand(), TRUE) . RandomBytes::generate(55)));

... I have no idea why we are doing this really weird and broken thing. Let's just use the random bytes directly.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
32.78 KB

I did not touch Uuid, since it does not bring problems now, left it as is per Damien's comment.

Besides improving

drupal_hash_base64(uniqid(mt_rand(), TRUE) . RandomBytes::generate(55));

After search and replace i notice this

Crypt::hashBase64(Crypt::randomString(55));

occurs quite often. what about randomHashedString()?

Anyway, i think besides those above, patch is in good shape, i cant come up with a way to unit test hmacBase64() and hashBase64()

credits to Crell for the docblock:)

xjm’s picture

Are we really calling it... Crypt? :)

Crell’s picture

I wouldn't object to a randomHashedString($size) method. Seems a reasonable simplification.

Status: Needs review » Needs work

The last submitted patch, drupal-randomBytes-1929288-37.patch, failed testing.

ParisLiakos’s picture

here is a total hack

ParisLiakos’s picture

Status: Needs work » Needs review

..

ParisLiakos’s picture

meh, i keep missing stuff

ParisLiakos’s picture

Ok added randStringHashed()

Any reason:

Crypt::hashBase64(uniqid(mt_rand(), TRUE) . mt_rand())
Crypt::hashBase64(uniqid(mt_rand(), TRUE))
Crypt::hashBase64(uniqid(mt_rand(), TRUE) . Crypt::randomString())

Couldnt turn to

Crypt::randomStringHashed()

amd just work? or objections?

Damien Tournoud’s picture

Let's not try to fix what's not broken:

-function drupal_random_bytes($count)  {
+  public static function randomString($count = 55) {

The function should be called Crypt::randomBytes(), and $count should be mandatory.

Speaking of... I don't understand why we do this to begin with:

+  public static function randomStringHashed() {
+    return self::hashBase64(self::randomString());
+  }

Hashing a random string can really only result in losing entropy. We should really just convert the random bytes to base64.

Any reason:

Crypt::hashBase64(uniqid(mt_rand(), TRUE) . mt_rand())
Crypt::hashBase64(uniqid(mt_rand(), TRUE))
Crypt::hashBase64(uniqid(mt_rand(), TRUE) . Crypt::randomString())

Couldnt turn to

Crypt::randomStringHashed()

amd just work? or objections?

The initial reasoning was to have "safe" and "unsafe" random ids: "safe" are generated from the strong random source, "unsafe" are generated from mt_rand() directly, with some slight (but completely undefined) entropy added by uniqid().

This reasoning doesn't really hold: in PHP, mt_rand() Is a very weak RNG, mostly because its state is not *that* hard to guess (the generator is seeded from values - the request time, the PID of the PHP process, etc. - that are partially observable and not very random). We should (but we don't) seed the generator with strong values before using it. And at this point, the performance difference between generating random bytes from mt_rand() and reading them (in a 8k batch) directly from /dev/urandom is not going to make a difference for us.

So, I would recommend getting rid of all the mt_rand() and the uniqid() in core, and be done with it.

ParisLiakos’s picture

Speaking of... I don't understand why we do this to begin with:

See #37 on why i added randomStringHashed(). Just for DX

In #12

Since we're using a default of 55 almost everywhere we use ::generate(), would it make sense to introduce a default of 55 in the function?

Since i was already messing with DX, i thought why not?

Also, i thought RandomString is better, since it says in the docblock
Returns a string of highly randomized bytes (over the full 8-bit range).

But i am ok with just reuploading patch in #43 and renaming it to RandomBytes.

Damien Tournoud’s picture

@rootatwc: I'm looking at the why we did that *even before the patch*. I think it is useful to have a function that gets you a random URL-safe identifier, so I'm not criticizing the existence of randomStringHashed() at all. What I'm discussing is why we are hashing + base64-ing the random bytes instead of just base64-ing them directly. This is not changed by this patch.

We use 55 everywhere, but just because we actually want randomStringHashed() everywhere. The default doesn't make sense outside of this particular use-case.

Damien Tournoud’s picture

About "String" vs "Bytes": a String in Drupal always implies a UTF-8 string. It would be unwise to break this expectation here.

ParisLiakos’s picture

Ah, ok, sorry for misunderstanding:)

About "String" vs "Bytes": a String in Drupal always implies a UTF-8 string. It would be unwise to break this expectation here.

You are right, i am gonna change the method name

ParisLiakos’s picture

restored $count as required argument and renamed randomString to randomBytes

Status: Needs review » Needs work
Issue tags: -PHPUnit Blocker

The last submitted patch, drupal-crypt-1929288-50.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit Blocker

entity translation ui random failure?
#50: drupal-crypt-1929288-50.patch queued for re-testing.

ParisLiakos’s picture

no longer applies, reroll

ParisLiakos’s picture

reroll

dawehner’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -2579,9 +2484,12 @@ function drupal_valid_test_ua($new_prefix = NULL) {
+    // We cant use Crypt::hmacBase64() yet.

There should be also an explanation, why we an't yet.

+++ b/core/lib/Drupal/Component/Utility/Crypt.phpundefined
@@ -0,0 +1,125 @@
+   * @param $data
...
+   * @param $key
...
+   * @return
...
+   * @param $data
...
+   * @return
...
+   * @return

Should all be proper type hinted, sorry.

+++ b/core/lib/Drupal/Component/Utility/Crypt.phpundefined
@@ -0,0 +1,125 @@
+    return self::hashBase64(self::randomBytes($count));

I proper to use static if possible, as this allows you to override the class later. (you never know what this might be good for).

ParisLiakos’s picture

thanks for the review!
you are absolutely right on all 3 points, attached patch fixes them:)

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, this looks fairly straight-forward.

Committed and pushed to 8.x. Thanks!

webchick’s picture

Title: Move cryptographic functions to Crypt component » Change notice: Move cryptographic functions to Crypt component
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

Er. Change notice.

ParisLiakos’s picture

Title: Change notice: Move cryptographic functions to Crypt component » Move cryptographic functions to Crypt component
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Yay!
Change notice here http://drupal.org/node/1984806

ParisLiakos’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.