Upgrade random_compat to latest version

Its encouraging that the tests passed. However, we should quantify differences between the versions and try to isolate the potential impact to Drupal

Drupal 8 core uses random_compat library when Crypt::randomBytes is called for generating a string of highly randomized bytes. The latest version of random_compat, i.e. 2.x, does not use OpenSSL as one of its sources. The removal is due to the associated unreliability of openssl_random_pseudo_bytes() as can be seen here:

openssl_random_pseudo_bytes() is not cryptographically secure

But the openssl_random_pseudo_bytes() removal actually got done in the random_compat release 1.3.0. So updating D8 core from 1.x to 2.x shouldn't cause any problems.

Instead of marking the issue fixed, let's have someone else review it too. For reference: random_compat CHANGELOG.

See https://github.com/FriendsOfPHP/security-advisories/pull/146 for a discussion about the security implications.

The difference between version 1 and 2 is: When no other CSPRNGs are available, version 1 falls back to openssl, version 2 throws an Exception. Most non-Windows servers will never experience this error. Windows users can use version 1 safely by ensuring they installed ext/mcrypt (for CryptGenRandom).

Encouraging people to use version 2 instead of version 1 is safer but using version 1 isn't inherently unsafe.

So one issue that we might face is that Drupal 8 windows installs might break :(

Here's a patch that upgrades the library properly... the procedure is:

1. edit core/composer.json
2. run composer update package/name

For maximum library compatibility we should IMHO use "paragonie/random_compat": "~1.0|~2.0"
Their internal API didn't changed, its simply that openssl got dropped. Let's assume you have a library which depends on 1.0, it will no longer be installable. I think in an ideal world both this library and core specifies ~1.0|~2.0.

+1

Let's go with ^1.0|^2.0 instead ...

Added an updated patch for the testbot to munch on

Marking as needs review

Let's try this again...

The patch #13 looks good to me as per #7 & #9.

But the composer documentation specifies that "a double pipe ( || ) will be treated as a logical OR" and we're using a single pipe ( | ) in #13. I'm not sure whether it is done purposely or not, hence not marking this issue as RTBC.

What I think is weird is that the hash in the composer.lock is not changed:

    "hash": "7d101b08e5ae002d827cd42ae9a4e344",
    "content-hash": "60f7057617c6d995bf9946d0b12f0b5d",

Still learning how this is set up in core... I ran `composer update` from the Drupal root. The hash changes were not applied. Further, the other changes represented in composer.lock from both alexpott and I's last patch now seems to be missing.

After updating core/composer.json, what steps do I need to do?

@dawehner the hash does not change when the version doesn't change - it's part of the fun of the merge plugin. That is to say, I think this only changes when /composer.json changes - not core/composer.json changes.

Based on #17, is the patch provided in #13 suitable? I'm happy to make whatever changes are needed.

Oh yeah you are right @alexpott

So the one thing that gives me pause is #2768953: Prevent insecure Guzzle from being installed when using composer to manager your project dependencies - there we decided to prevent insecure Guzzle regardless of interoperability - is this not the same thing?

I don't have a ton of confidence regarding my knowledge of how core interacts with these external projects. But, if I read the related issue in #20, it would suggest we should just use 2.0 and move forward.

Is this correct? If so, I can work on another patch

One other take on this, is that the upgrade of random_compat doesn't appear to be a security-related upgrade. This is just supporting compatibility with other projects that can use the new version. So, isn't it appropriate for us to support 1.x or 2.x? If so, the patch I uploaded should be fine if testbot stops barfing.

Right, and see comment #5 regarding security implications.

Encouraging people to use version 2 instead of version 1 is safer but using version 1 isn't inherently unsafe.

Looks like we just need a reroll of the latest patch?

Marking as needs review now that #28 is passed

I'm not 100% sure whether supporting both version is the right thing to do, but to be honest, both are kinda the same though anyway.

Here's a re-rolled patch.

remove unrelated changes

Thanks. I'm not sure how those sneaked in there.

Back to RTBC

Yet another patch re-roll.

I'm not sure I agree with leaving 1.0 in composer.json here.

If the API is unchanged, any dependency that required 1.0 could be upgraded to 2.0 trivially?

Maybe it doesn't matter since if using PHP 7 you will always use the built-in?

I also think this is a bug fix that should be applied to 8.2.x also

Not allowing 1.x seems fine with me, but I'm not clear on all of the possible implications within other parts of core.

Well, one reason to maybe leave 1.0 in there is that contrib modules / their dependencies might use 1.0 in their composer.json files. In case they do, we would have a conflict here,
much like we have it in core now.

Looks good! We can discuss changing the version constraint in a follow-up, the current patch is most permissible and should not cause problems for contrib.

I think it's fair to leave 1.0 and 2.0 for now, since it's impossible until the patch lands to require 2.0 in a module, making it impossible not to is a hard break, even if composer.json is considered @internal.

We could have a follow-up (8.4.x?) to require the 2.0 branch maybe, by then there's plenty for modules to update their dependencies.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

As a library update this is rc-eligible (especially since it's compat rather than providing a specific API).

Thanks a ton for cherry-picking to 8.2!!

Probably this issue would also be the part of the release note.

diff --git a/composer.lock b/composer.lock
index 8d2b325..ddc7b5a 100644
--- a/composer.lock
+++ b/composer.lock
@@ -999,16 +999,16 @@
         },
         {
             "name": "paragonie/random_compat",
-            "version": "1.1.1",
+            "version": "v2.0.2",

Given that drupal/core requires drupal/core-utility we should quickly follow-up to update its paragonie/random_compat": "~1.0" requirement declaration. (core/lib/Drupal/Component/Utility/composer.json)
(Or revert.)
We really need an issue to create a test to prevent this type of composer breakage.

My site no longer works after upgrading to drupal-8.2.0-rc2.

Exception: There is no suitable CSPRNG installed on your system in vendor/paragonie/random_compat/lib/random.php on line 185 #0 core/lib/Drupal/Component/Utility/Crypt.php(31): random_bytes(55)

Maybe the problem is random_bytes_openssl.php file was removed in paragonie/random_compat.

Reverted from both branches for now.

@maximpodorov
Thanks a lot to catch this problem before the actual release!

@maximpodorov can you describe your system? Ie. operating system and php version.

CentOS 7
PHP 5.6.26
All extensions required for D8 (https://www.drupal.org/docs/7/system-requirements/php) are installed.

The possible explanation is here:
https://github.com/paragonie/random_compat/issues/111
This new version of random_compat requires that PHP's open_basedir restriction must permit to read /dev/urandom.

@maximpodorov - any chance you could read https://github.com/paragonie/random_compat/issues/109 and paste the output of

var_dump([
    'version' => PHP_VERSION,
    'open_basedir' => ini_get('open_basedir'),
    'safe_mode' => ini_get('safe_mode'),
    'directory separator' => DIRECTORY_SEPARATOR,
    'mcrypt_create_iv exists' => function_exists('mcrypt_create_iv'),
    'openssl_random_pseudo_bytes exists' => function_exists('openssl_random_pseudo_bytes'),
    'readable /dev/urandom' => is_readable('/dev/urandom')
]);

here... thanks!

array(7) {
["version"]=> string(6) "5.6.26"
["open_basedir"]=> string(34) "my secret path to site :)"
["safe_mode"]=> bool(false)
["directory separator"]=> string(1) "/"
["mcrypt_create_iv exists"]=> bool(false)
["openssl_random_pseudo_bytes exists"]=> bool(true)
["readable /dev/urandom"]=> bool(false)
}

Looks like it's time to switch from open_basedir to chroot. Or switch to PHP7 finally.

Doesn't #65 in combination with #4 suggest that #56 would happen on 1.3.0 also?

Looks like 1.3.0 will use openssl and openssl_random_pseudo_bytes exists so it won't hard fail?

So maybe we should do the same as Drupal 7 patch and fall back to the hash generator?

The direction here is still under discussion and the issue summary is not clearly identifying what novice task might be appropriate (yet). so removing the tag for now. https://drupal.org/core-mentoring/novice-tasks

For reference, this is what the maintainer of this library has to say about versioning here:

https://twitter.com/ParagonIE/status/776096550103625728

Or for 8.2 we should fallback to openssl on exception, and re-commit to 8.3.x as is (after fixing core/lib/Drupal/Component/Utility/composer.json)?

talking to @pwolanin at the drupalcon dublin sprint. these are notes from the conversation.

1.

there is an actual fix hinted at in #55
and that is that we should actually specify which version to match the change this issue is making to upgrade the version

---------------------
now something more complicated. what to do.

2.

for openssl there is evidence that people *are* getting repeated "random" strings. (this could make things less secure)

in the code in drupal 7 is/was (see the backport in #2550519: Crypt::randomBytes()/drupal_random_bytes() doesn't actually return cryptographically secure random bytes which is rtbc (right now)) not possible to get repeat random strings because it concats many "random" things *and* includes microtime, which unless the request is at exactly the same time will always give a different string.

in drupal_random_bytes()

if (!isset($random_state)) {
        $random_state = print_r($_SERVER, TRUE);
        if (function_exists('getmypid')) {
          // Further initialize with the somewhat random PHP process ID.
          $random_state .= getmypid();
        }
        $bytes = '';
      }

      do {
        $random_state = hash('sha256', microtime() . mt_rand() . $random_state);
        $bytes .= hash('sha256', mt_rand() . $random_state, TRUE);
      }
      while (strlen($bytes) < $count);

So for 8.2.x we should do the upgrade to v2.0.2, and add a fallback on exception to something like drupal_random_bytes(). That would have a requirement error on the status page, so gives people on 8.2 a chance to notice they will need to improve their system, but keep it working

For 8.3.x we could just do the upgrade to v2.0.2 ... but that breaks the BC promise of 8.x

+1 for fallback + site status warning.

Discussed in person with alexpott and think we concluded again that the path forward described by YesCT is the best option for BC

Here's a patch pulling in basically the same code from #2550519: Crypt::randomBytes()/drupal_random_bytes() doesn't actually return cryptographically secure random bytes

alexpott suggest putting a function inside the Crypt namespace for a test, since a function inside the namespace will be used in preference.

Here's with a unit test.

I will take a look at the patch now. (others can also. :) )

Notes from looking at the test interdiff.

1. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,51 @@
   +namespace Drupal\Component\Utility;
   +/**
   ...
   +use Drupal\Component\Utility\Crypt;
   +/**
   

   typically, a blank line between the use (or namespace) and the comment block.

2. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,51 @@
   + * Define a function in the namespace of Drupal\Component\Utility\Crypt.
   

   Forces throwing an exception in the test environment.

   Test environments have the fallbacks, so force an exception so we can test the exception. Also, for php 7, this needs to be in the same namespace that the crypt class is in, so it gets preferentially called over the php 7 built in function.

3. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,51 @@
   + * @param $count
   

   Add type and description...
   https://www.drupal.org/node/1354#param

   @param int $count
   Integer just to match the global function definition.

4. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,51 @@
   +function random_bytes($count) {
   

   type the function (public, protected, or private)

   https://www.drupal.org/node/608152#visibility

5. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,51 @@
   +  throw new \Exception($count);
   

   $count passed to exception, just so $count is "used" and IDEs dont complain... hm. ok. That would be a pretty silly comment to add to the code? Just noting it on the issue here.

6. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,51 @@
   + * Tests random byte generation.
   

   Tests random byte generation fallback exception situations.

7. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,51 @@
   +  static protected $functionCalled = 0;
   +
   +  public static function functionCalled() {
   +    static::$functionCalled++;
   +  }
   

   this is here to make sure that the overriding function is the one that was called, and not the global one.

   we could add comments to explain that, and meet the doc block requirements.

8. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,51 @@
   +   * Tests random byte generation using the fallback generator.
   

   Tests if the call to random_bytes() throws an exception, that the crypt::random_bytes() still a useful string of random bytes.

9. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,51 @@
   +  public function testRandomBytesFallback() {
   

   Just noting that this was copied from CryptTest::testRandomBytes()

10. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
    @@ -0,0 +1,51 @@
    +    $this->assertEquals(30, static::$functionCalled, 'The namespaced function was called the expected number of times');
    

    I think the message can be a sentence that ends with a period.

+++ b/core/lib/Drupal/Component/Utility/Crypt.php
@@ -19,7 +19,7 @@ class Crypt {
-   * the random_compat library.
+   * the random_compat library, or the fallback hash-based generator from 7.x.

Probably useful to say Drupal 7.x since in this same paragraph we are mentioning PHP 7.

------

read the rest of this, compared it to the drupal 7 code and looks good. the message text is still relevant too, so using the same is ok.

Here with code and comment cleanups

oh, I didn't realize I might not have had enough context lines in the review. sorry.

#79

2. was probably too wordy. ok.

4. ah, it can't have visibility cause it's not in a class. and function visibility is an object oriented thing.

---

tweaked some comments.

(todo to myself or someone else, might be nice to get a tests only patch to document proof of the behavior on the issue. I was gonna do that, but sprint room being closed. :) )

some sporadic test fail

+++ b/core/modules/system/system.install
@@ -257,6 +257,24 @@ function system_requirements($phase) {
+    } catch (\Exception $e) {

catch should be on the next line.

Otherwise looks good. I'm hesitant to RTBC this because I really think we should not have this fallback at all. I'd rather see the site fail and make people fix their random number sources.

1. +++ b/core/lib/Drupal/Component/Utility/Crypt.php
   @@ -28,7 +29,42 @@ class Crypt {
   +          $random_state = hash('sha256', microtime() . mt_rand() . $random_state);
   +          $bytes .= hash('sha256', mt_rand() . $random_state, TRUE);
   

   Sorry for this question.

   Why is there once a third parameter and once not? Maybe we could document that

2. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,67 @@
   +/**
   + * Tests random byte generation fallback exception situations.
   + *
   + * @group Utility
   + *
   + * @coversDefaultClass \Drupal\Component\Utility\Crypt
   + */
   +class CryptRandomFallbackTest extends UnitTestCase {
   

   Just to be sure I would put runTestsInSeparateProcesses on there

3. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,67 @@
   +      $this->assertEquals(strlen(Crypt::randomBytes($count)), $count);
   

   Note: you can use $this->assertCount()

Added annotation

The long comment at the start of the catch mentions that the two hash() invocations are different. That's a suggestion from Solr Designer to improve the robustness.

I don't think $this->assertCount() works to calculate a string length. Fixed the param ordering.

@pwolanin
Thank you for clarifying that

I am not 100% comfortable, but given we have done something similar in d7 ...

Ah, now that we reintroduce our own random number generator we forgot to bring back the preseeding of mt_rand(), thanks David Rothstein for discovering that in #2550519: Crypt::randomBytes()/drupal_random_bytes() doesn't actually return cryptographically secure random bytes.

We should not put it back in DrupalKernel, but rather do it once in the request cycle in Crypt::randomBytes(). See http://cgit.drupalcode.org/drupal/diff/core/lib/Drupal/Core/DrupalKernel...

And that is why you should not do your own random number generator, because you tend to forget stuff :(

YEs, we should reseed - but hopefully was can just call the function without giving it a seed value? I think YesCT was trying to find more info.

Ok, adding the seed call into the fallback generator. And improving the requirements check based on Heine's change in #2550519: Crypt::randomBytes()/drupal_random_bytes() doesn't actually return cryptographically secure random bytes to look for open_basedir

clarifying in summary.

I re-read the entire patch. I think this has BC, tests, and appropriate messages (requirement on install, message on site status page (for upgraded sites)).

Note to self: the phase of the site on install is install, on upgrade is upgrade, and runtime for a site that is... running (so that is what it will be for an upgraded site that will let them see the message on the site status page).

+++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
@@ -0,0 +1,69 @@
+namespace Drupal\Component\Utility;
+
+/**
+ * Defines a function in same namespace as Drupal\Component\Utility\Crypt.
+ *
+ * Forces throwing an exception in this test environment because the function
+ * in the namespace is used in preference to the global function.
+ *
+ * @param int $count
+ *   Matches the global function definition.
+ *
+ * @throws \Exception
+ */
+function random_bytes($count) {
+  \Drupal\Tests\Component\Utility\CryptRandomFallbackTest::functionCalled();
+  throw new \Exception($count);
+}
+
+namespace Drupal\Tests\Component\Utility;
+
+use Drupal\Tests\UnitTestCase;
+use Drupal\Component\Utility\Crypt;

Don't need two of the same namespaces... and general convention has been to put the function after the class so the namespace / use statement / class is as expected.

We also should annotate the test so that it only runs on php 5.5 and 5.6. I think we can just do https://phpunit.de/manual/current/en/incomplete-and-skipped-tests.html#i...

So I think we want @requires PHP 5

Afaics @requires does not work - we need to compare the PHP version in the test and mark as skipped if PHP version > 5

There are 2 different namespaces, but sounds like we need to re-order the code so the function is at the end of the file

As designed, I think the test will work on both PHP 5 and 7, but let's confirm

@pwolanin - yes the test will work but it is pointless on PHP 7. And sorry for the confusion about namespaces - that was because all the other examples of this pattern we put the class first.

@alexpott - it's not pointless on 7 since the built-in function can also throw an exception.

http://php.net/random_bytes

"If an appropriate source of randomness cannot be found, an Exception will be thrown."

Just moving the function to the bottom

Some minor stuff:

1. +++ b/core/lib/Drupal/Component/Utility/Crypt.php
   @@ -28,7 +29,44 @@ class Crypt {
   +          // Ensure mt_rand() is reseeded on every page load.
   +          mt_srand();
   

   This has nothing to do with page load now. Could be "Ensure mt_rand() is reseeded before calling it the first time."

2. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
   @@ -0,0 +1,69 @@
   +  throw new \Exception($count);
   +}
   \ No newline at end of file
   

   newline at the end of file missing.

I like the hook_requirements() implementation with the open basedir detection, that should give people a better hint.

looks good, we should copy that wording change to the D7 issue also.

Committed 2bccef8 and pushed to 8.3.x. Thanks!

diff --git a/core/lib/Drupal/Component/Utility/Crypt.php b/core/lib/Drupal/Component/Utility/Crypt.php
index 9409f30..6ebdc4a 100644
--- a/core/lib/Drupal/Component/Utility/Crypt.php
+++ b/core/lib/Drupal/Component/Utility/Crypt.php
@@ -60,8 +60,7 @@ public static function randomBytes($count) {
         do {
           $random_state = hash('sha256', microtime() . mt_rand() . $random_state);
           $bytes .= hash('sha256', mt_rand() . $random_state, TRUE);
-        }
-        while (strlen($bytes) < $count);
+        } while (strlen($bytes) < $count);
       }
       $output = substr($bytes, 0, $count);
       $bytes = substr($bytes, $count);
diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index 31c34b5..e8f45eb 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -268,7 +268,8 @@ function system_requirements($phase) {
         throw new \Exception(t('Tried to generate 10 random bytes, generated @count', array('@count' => strlen($bytes))));
       }
       $requirements['php_random_bytes']['value'] = t('Successful');
-    } catch (\Exception $e) {
+    }
+    catch (\Exception $e) {
       // If /dev/urandom is not available on a UNIX-like system, check whether
       // open_basedir restrictions are the cause.
       $open_basedir_blocks_urandom = FALSE;
diff --git a/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
index 176df1a..eb746e6 100644
--- a/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
+++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
@@ -52,6 +52,8 @@ public function testRandomBytesFallback() {
 
 namespace Drupal\Component\Utility;
 
+use  \Drupal\Tests\Component\Utility\CryptRandomFallbackTest;
+
 /**
  * Defines a function in same namespace as Drupal\Component\Utility\Crypt.
  *
@@ -64,7 +66,7 @@ public function testRandomBytesFallback() {
  * @throws \Exception
  */
 function random_bytes($count) {
-  \Drupal\Tests\Component\Utility\CryptRandomFallbackTest::functionCalled();
+  CryptRandomFallbackTest::functionCalled();
   throw new \Exception($count);
 }
 

Code style fixes on commit.

This was reverted on 8.2.x by @catch, but there is no indication of why?

Needs re-commit to 8.2.x?

http://cgit.drupalcode.org/drupal/commit/composer.lock?id=3fda618323bef5...

It was reverted because it breaks sites with non-empty open_basedir setting which does not contain /dev directory.

But now we take care of that with the bad fallback generator, so we just need to backport this to 8.2.x.

Comment #106 says

Committed 2bccef8 and pushed to 8.2.x. Thanks!

But it was only pushed to 8.3.x (see #107).
Retesting against 8.2.x now.

RTBC unless bot disagrees.

Sorry my comment was wrong about the branch pushed to. I can't see any compelling reason to commit this to 8.2.x - @larowlan is there one beyond the general security improvements?

Well, its not only that, but it also enables some contrib modules to deal with the dependency on random_compat

@dawehner hmmm but what is wrong with those contrib modules saying the same as what we do here... "paragonie/random_compat": "^1.0|^2.0",?

@dawehner hmmm but what is wrong with those contrib modules saying the same as what we do here... "paragonie/random_compat": "^1.0|^2.0",?

External libraries don't do that: https://github.com/defuse/php-encryption/blob/master/composer.json#L23

This includes a major version change for a dependency. In semver and our allowed changes policy, only patch-level updates should be made in patch releases:
https://www.drupal.org/core/d8-allowed-changes#patch

Technically it's even not allowed in a minor, but I can see the case for specifically breaking the policy for security hardening. But I definitely don't think this should be backported to a patch release.

We don't expose random_compat in our own API, so there's no reason we shouldn't change it in a minor release IMO. Not to change it would be to essentially commit us to forking random_compat 1.x and maintaining it until the end of Drupal 8's LTS release - i.e. it's not just a case of security hardening now, but being able to upgrade to patch releases later on if there are bugs. In terms of disruption it can be as disruptive not to update (as this issue shows) as it is to update.

Having said that, I also don't see a reason to do this in a patch release unless there's a pressing reason - and the contrib dependency isn't one for me given the first version of this patch broke someone's install; contrib can recommend this core patch and/or require 8.3.x. If there was a pressing reason later like a critical bug fix in 2.x that's not in 1.x we could re-open and do it then.

Personally I don't see the harm in updating in a patch release (especially given we're going with the ^1.0|^2.0 version constraint). As it stands we can't use defuse/php-encryption (https://github.com/defuse/php-encryption - the best-of-breed php encryption library) with Drupal 8.2, as it requires the 2.x version.

The random_compat docs explicitly mention this:

If you're using a project that has a line like this in its composer.json

"require" {
...
"paragonie/random_compat": "~1.1",
...
}

...and then you try to add random_compat 2 (or another library that explicitly requires random_compat 2, such as this secure PHP encryption library), you will get a version conflict.

The solution is to get the project to update its requirement string to allow version 2 and above to be used instead of hard-locking users to version 1.

"require" {
...
- "paragonie/random_compat": "~1.1",
+ "paragonie/random_compat": "^1|^2",
...
}

So I guess we have to wait until 8.3 to use php-encryption with Drupal 8, or apply this patch.

Ok, so let's do the minimal thing first for 8.2.x which we all can agree on: change the composer version constraint to at least allow higher versions of random_compat.

Thanks @klausi

That's a good idea updating composer.json and not composer.lock. Committed/pushed to 8.2.x, thanks!