MAGETWO-47050: Magento 2: encryption keys generated in the admin interface are very weak

- changed key generation algorithm

Author: eddielau

app/code/Magento/EncryptionKey/Model/ResourceModel/Key/Change.php

@@ -44,12 +44,20 @@ class Change extends \Magento\Framework\Model\ResourceModel\Db\AbstractDb
      */
     protected $writer;
 
+    /**
+     * Random
+     *
+     * @var \Magento\Framework\Math\Random
+     */
+    protected $random;
+
     /**
      * @param \Magento\Framework\Model\ResourceModel\Db\Context $context
      * @param \Magento\Framework\Filesystem $filesystem
      * @param \Magento\Config\Model\Config\Structure $structure
      * @param \Magento\Framework\Encryption\EncryptorInterface $encryptor
      * @param \Magento\Framework\App\DeploymentConfig\Writer $writer
+     * @param \Magento\Framework\Math\Random $random
      * @param string $connectionName
      */
     public function __construct(
@@ -58,13 +66,15 @@ public function __construct(
         \Magento\Config\Model\Config\Structure $structure,
         \Magento\Framework\Encryption\EncryptorInterface $encryptor,
         \Magento\Framework\App\DeploymentConfig\Writer $writer,
+        \Magento\Framework\Math\Random $random,
         $connectionName = null
     ) {
         $this->encryptor = clone $encryptor;
         parent::__construct($context, $connectionName);
         $this->directory = $filesystem->getDirectoryWrite(DirectoryList::CONFIG);
         $this->structure = $structure;
         $this->writer = $writer;
+        $this->random = $random;
     }
 
     /**
@@ -92,7 +102,7 @@ public function changeEncryptionKey($key = null)
         }
 
         if (null === $key) {
-            $key = md5(time());
+            $key = md5($this->random->getRandomString(ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE));
         }
         $this->encryptor->setNewKey($key);
 

app/code/Magento/EncryptionKey/Test/Unit/Model/ResourceModel/Key/ChangeTest.php

@@ -29,6 +29,8 @@ class ChangeTest extends \PHPUnit_Framework_TestCase
     protected $tansactionMock;
     /** @var |\PHPUnit_Framework_MockObject_MockObject */
     protected $objRelationMock;
+    /** @var \Magento\Framework\Math\Random|\PHPUnit_Framework_MockObject_MockObject */
+    protected $randomMock;
     /** @var \Magento\EncryptionKey\Model\ResourceModel\Key\Change */
     protected $model;
 
@@ -72,6 +74,7 @@ public function setUp()
             ->disableOriginalConstructor()
             ->setMethods([])
             ->getMock();
+        $this->randomMock = $this->getMock('Magento\Framework\Math\Random', [], [], '', false);
 
         $helper = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this);
 
@@ -85,20 +88,20 @@ public function setUp()
                 'adapterInterface' => $this->adapterMock,
                 'resource' => $this->resourceMock,
                 'transactionManager' => $this->tansactionMock,
-                'relationProcessor' => $this->objRelationMock
+                'relationProcessor' => $this->objRelationMock,
+                'random' => $this->randomMock
             ]
         );
     }
 
-    public function testChangeEncryptionKey()
+    private function setUpChangeEncryptionKey()
     {
         $paths = ['path1', 'path2'];
         $table = ['item1', 'item2'];
         $values = [
             'key1' => 'value1',
             'key2' => 'value2'
         ];
-        $key = 'key';
 
         $this->writerMock->expects($this->once())->method('checkIfWritable')->willReturn(true);
         $this->resourceMock->expects($this->atLeastOnce())->method('getConnection')->willReturn($this->adapterMock);
@@ -112,10 +115,23 @@ public function testChangeEncryptionKey()
         $this->selectMock->expects($this->any())->method('update')->willReturnSelf();
         $this->writerMock->expects($this->once())->method('saveConfig');
         $this->adapterMock->expects($this->once())->method('getTransactionLevel')->willReturn(1);
+    }
 
+    public function testChangeEncryptionKey()
+    {
+        $this->setUpChangeEncryptionKey();
+        $this->randomMock->expects($this->never())->method('getRandomString');
+        $key = 'key';
         $this->assertEquals($key, $this->model->changeEncryptionKey($key));
     }
 
+    public function testChangeEncryptionKeyAutogenerate()
+    {
+        $this->setUpChangeEncryptionKey();
+        $this->randomMock->expects($this->once())->method('getRandomString')->willReturn('abc');
+        $this->assertEquals(md5('abc'), $this->model->changeEncryptionKey());
+    }
+
     public function testChangeEncryptionKeyThrowsException()
     {
         $key = 'key';
@@ -127,6 +143,6 @@ public function testChangeEncryptionKeyThrowsException()
             return;
         }
 
-        $this->fail('An excpected exception was not signaled.');
+        $this->fail('An expected exception was not signaled.');
     }
 }