Skip to content

Commit ec56738

Browse files
sreenia806sreenia806
committed
AC-8017 system settings improvement
1 parent 7c7461a commit ec56738

File tree

9 files changed

+97
-58
lines changed

9 files changed

+97
-58
lines changed

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

Lines changed: 40 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,22 @@
55
*/
66
namespace Magento\EncryptionKey\Model\ResourceModel\Key;
77

8+
use \Exception;
9+
use Magento\Config\Model\Config\Backend\Encrypted;
10+
use Magento\Config\Model\Config\Structure;
11+
use Magento\Framework\App\DeploymentConfig\Writer;
812
use Magento\Framework\App\Filesystem\DirectoryList;
913
use Magento\Framework\Config\ConfigOptionsListConstants;
1014
use Magento\Framework\Config\Data\ConfigData;
1115
use Magento\Framework\Config\File\ConfigFilePool;
16+
use Magento\Framework\Encryption\EncryptorInterface;
17+
use Magento\Framework\Exception\FileSystemException;
18+
use Magento\Framework\Exception\LocalizedException;
19+
use Magento\Framework\Filesystem;
20+
use Magento\Framework\Filesystem\Directory\WriteInterface;
21+
use Magento\Framework\Math\Random;
22+
use Magento\Framework\Model\ResourceModel\Db\AbstractDb;
23+
use Magento\Framework\Model\ResourceModel\Db\Context;
1224

1325
/**
1426
* Encryption key changer resource model
@@ -19,60 +31,60 @@
1931
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
2032
* @since 100.0.2
2133
*/
22-
class Change extends \Magento\Framework\Model\ResourceModel\Db\AbstractDb
34+
class Change extends AbstractDb
2335
{
2436
/**
2537
* Encryptor interface
2638
*
27-
* @var \Magento\Framework\Encryption\EncryptorInterface
39+
* @var EncryptorInterface
2840
*/
2941
protected $encryptor;
3042

3143
/**
3244
* Filesystem directory write interface
3345
*
34-
* @var \Magento\Framework\Filesystem\Directory\WriteInterface
46+
* @var WriteInterface
3547
*/
3648
protected $directory;
3749

3850
/**
3951
* System configuration structure
4052
*
41-
* @var \Magento\Config\Model\Config\Structure
53+
* @var Structure
4254
*/
4355
protected $structure;
4456

4557
/**
4658
* Configuration writer
4759
*
48-
* @var \Magento\Framework\App\DeploymentConfig\Writer
60+
* @var Writer
4961
*/
5062
protected $writer;
5163

5264
/**
53-
* Random
65+
* Random string generator
5466
*
55-
* @var \Magento\Framework\Math\Random
67+
* @var Random
5668
* @since 100.0.4
5769
*/
5870
protected $random;
5971

6072
/**
61-
* @param \Magento\Framework\Model\ResourceModel\Db\Context $context
62-
* @param \Magento\Framework\Filesystem $filesystem
63-
* @param \Magento\Config\Model\Config\Structure $structure
64-
* @param \Magento\Framework\Encryption\EncryptorInterface $encryptor
65-
* @param \Magento\Framework\App\DeploymentConfig\Writer $writer
66-
* @param \Magento\Framework\Math\Random $random
73+
* @param Context $context
74+
* @param Filesystem $filesystem
75+
* @param Structure $structure
76+
* @param EncryptorInterface $encryptor
77+
* @param Writer $writer
78+
* @param Random $random
6779
* @param string $connectionName
6880
*/
6981
public function __construct(
70-
\Magento\Framework\Model\ResourceModel\Db\Context $context,
71-
\Magento\Framework\Filesystem $filesystem,
72-
\Magento\Config\Model\Config\Structure $structure,
73-
\Magento\Framework\Encryption\EncryptorInterface $encryptor,
74-
\Magento\Framework\App\DeploymentConfig\Writer $writer,
75-
\Magento\Framework\Math\Random $random,
82+
Context $context,
83+
Filesystem $filesystem,
84+
Structure $structure,
85+
EncryptorInterface $encryptor,
86+
Writer $writer,
87+
Random $random,
7688
$connectionName = null
7789
) {
7890
$this->encryptor = clone $encryptor;
@@ -98,20 +110,20 @@ protected function _construct()
98110
*
99111
* @param string|null $key
100112
* @return null|string
101-
* @throws \Exception
113+
* @throws FileSystemException|LocalizedException|Exception
102114
*/
103115
public function changeEncryptionKey($key = null)
104116
{
105117
// prepare new key, encryptor and new configuration segment
106118
if (!$this->writer->checkIfWritable()) {
107-
throw new \Exception(__('Deployment configuration file is not writable.'));
119+
throw new FileSystemException(__('Deployment configuration file is not writable.'));
108120
}
109121

110122
if (null === $key) {
111-
// md5() here is not for cryptographic use. It used for generate encryption key itself
112-
// and do not encrypt any passwords
113-
// phpcs:ignore Magento2.Security.InsecureFunction
114-
$key = md5($this->random->getRandomString(ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE));
123+
$key = $this->random->getRandomBytes(
124+
ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE,
125+
ConfigOptionsListConstants::STORE_KEY_ENCODED_RANDOM_STRING_PREFIX
126+
);
115127
}
116128
$this->encryptor->setNewKey($key);
117129

@@ -128,7 +140,7 @@ public function changeEncryptionKey($key = null)
128140
$this->writer->saveConfig($configData);
129141
$this->commit();
130142
return $key;
131-
} catch (\Exception $e) {
143+
} catch (LocalizedException $e) {
132144
$this->rollBack();
133145
throw $e;
134146
}
@@ -142,11 +154,11 @@ public function changeEncryptionKey($key = null)
142154
protected function _reEncryptSystemConfigurationValues()
143155
{
144156
// look for encrypted node entries in all system.xml files
145-
/** @var \Magento\Config\Model\Config\Structure $configStructure */
157+
/** @var Structure $configStructure */
146158
$configStructure = $this->structure;
147159
$paths = $configStructure->getFieldPathsByAttribute(
148160
'backend_model',
149-
\Magento\Config\Model\Config\Backend\Encrypted::class
161+
Encrypted::class
150162
);
151163

152164
// walk through found data and re-encrypt it

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -148,16 +148,16 @@ private function setUpChangeEncryptionKey()
148148
public function testChangeEncryptionKey()
149149
{
150150
$this->setUpChangeEncryptionKey();
151-
$this->randomMock->expects($this->never())->method('getRandomString');
151+
$this->randomMock->expects($this->never())->method('getRandomBytes');
152152
$key = 'key';
153153
$this->assertEquals($key, $this->model->changeEncryptionKey($key));
154154
}
155155

156156
public function testChangeEncryptionKeyAutogenerate()
157157
{
158158
$this->setUpChangeEncryptionKey();
159-
$this->randomMock->expects($this->once())->method('getRandomString')->willReturn('abc');
160-
$this->assertEquals(hash('md5', 'abc'), $this->model->changeEncryptionKey());
159+
$this->randomMock->expects($this->once())->method('getRandomBytes')->willReturn('abc');
160+
$this->assertEquals('abc', $this->model->changeEncryptionKey());
161161
}
162162

163163
public function testChangeEncryptionKeyThrowsException()

‎lib/internal/Magento/Framework/Config/ConfigOptionsListConstants.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,4 +167,9 @@ class ConfigOptionsListConstants
167167
*/
168168
public const STORE_KEY_RANDOM_STRING_SIZE = SODIUM_CRYPTO_AEAD_CHACHA20POLY1305_KEYBYTES;
169169
//phpcs:enable
170+
171+
/**
172+
* Prefix of encoded random string
173+
*/
174+
public const STORE_KEY_ENCODED_RANDOM_STRING_PREFIX = 'base64';
170175
}

‎lib/internal/Magento/Framework/Encryption/Encryptor.php

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
use Magento\Framework\App\DeploymentConfig;
1212
use Magento\Framework\App\ObjectManager;
13+
use Magento\Framework\Config\ConfigOptionsListConstants;
1314
use Magento\Framework\Encryption\Adapter\EncryptionAdapterInterface;
1415
use Magento\Framework\Encryption\Adapter\Mcrypt;
1516
use Magento\Framework\Encryption\Adapter\SodiumChachaIetf;
@@ -265,7 +266,12 @@ public function hash($data, $version = self::HASH_VERSION_SHA256)
265266
throw new \InvalidArgumentException('Unknown hashing algorithm');
266267
}
267268

268-
return hash_hmac($this->hashVersionMap[$version], (string)$data, $this->keys[$this->keyVersion], false);
269+
return hash_hmac(
270+
$this->hashVersionMap[$version],
271+
(string)$data,
272+
$this->decodeKey($this->keys[$this->keyVersion]),
273+
false
274+
);
269275
}
270276

271277
/**
@@ -375,7 +381,7 @@ private function explodePasswordHash($hash)
375381
*/
376382
public function encrypt($data)
377383
{
378-
$crypt = new SodiumChachaIetf($this->keys[$this->keyVersion]);
384+
$crypt = new SodiumChachaIetf($this->decodeKey($this->keys[$this->keyVersion]));
379385

380386
return $this->keyVersion .
381387
':' . self::CIPHER_AEAD_CHACHA20POLY1305 .
@@ -445,7 +451,7 @@ public function decrypt($data)
445451
if (!isset($this->keys[$keyVersion])) {
446452
return '';
447453
}
448-
$crypt = $this->getCrypt($this->keys[$keyVersion], $cryptVersion, $initVector);
454+
$crypt = $this->getCrypt($this->decodeKey($this->keys[$keyVersion]), $cryptVersion, $initVector);
449455
if (null === $crypt) {
450456
return '';
451457
}
@@ -520,7 +526,7 @@ private function getCrypt(
520526
}
521527

522528
if (null === $key) {
523-
$key = $this->keys[$this->keyVersion];
529+
$key = $this->decodeKey($this->keys[$this->keyVersion]);
524530
}
525531

526532
if (!$key) {
@@ -596,4 +602,17 @@ private function getArgonHash(
596602
)
597603
);
598604
}
605+
606+
/**
607+
* Find out actual decode key
608+
*
609+
* @param string $key
610+
* @return false|string
611+
*/
612+
private function decodeKey(string $key)
613+
{
614+
return (strpos($key, ConfigOptionsListConstants::STORE_KEY_ENCODED_RANDOM_STRING_PREFIX) === 0) ?
615+
base64_decode(substr($key, strlen(ConfigOptionsListConstants::STORE_KEY_ENCODED_RANDOM_STRING_PREFIX))) :
616+
$key;
617+
}
599618
}

‎lib/internal/Magento/Framework/Encryption/KeyValidator.php

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,12 @@ class KeyValidator
2525
*/
2626
public function isValid($value) : bool
2727
{
28-
return $value && strlen($value) === ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE
29-
&& preg_match('/^\S+$/', $value);
28+
if (strpos($value, ConfigOptionsListConstants::STORE_KEY_ENCODED_RANDOM_STRING_PREFIX) === 0) {
29+
return (bool)$value;
30+
} else {
31+
return $value
32+
&& strlen($value) === ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE
33+
&& preg_match('/^\S+$/', $value);
34+
}
3035
}
3136
}

‎lib/internal/Magento/Framework/Math/Random.php

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66
namespace Magento\Framework\Math;
77

8+
use \Exception;
89
use Magento\Framework\Exception\LocalizedException;
910
use Magento\Framework\Phrase;
1011

@@ -19,11 +20,11 @@ class Random
1920
/**#@+
2021
* Frequently used character classes
2122
*/
22-
const CHARS_LOWERS = 'abcdefghijklmnopqrstuvwxyz';
23+
public const CHARS_LOWERS = 'abcdefghijklmnopqrstuvwxyz';
2324

24-
const CHARS_UPPERS = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
25+
public const CHARS_UPPERS = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
2526

26-
const CHARS_DIGITS = '0123456789';
27+
public const CHARS_DIGITS = '0123456789';
2728

2829
/**#@-*/
2930

@@ -83,4 +84,17 @@ public function getUniqueHash($prefix = '')
8384
{
8485
return $prefix . $this->getRandomString(32);
8586
}
87+
88+
/**
89+
* Generate a base64 encoded binary string.
90+
*
91+
* @param int $length
92+
* @param string $prefix
93+
* @return string
94+
* @throws Exception
95+
*/
96+
public function getRandomBytes($length, $prefix = '')
97+
{
98+
return $prefix . base64_encode(random_bytes($length));
99+
}
86100
}

‎setup/src/Magento/Setup/Model/CryptKeyGenerator.php

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,7 @@ public function __construct(Random $random)
3737
*/
3838
public function generate()
3939
{
40-
// md5() here is not for cryptographic use. It used for generate encryption key itself
41-
// and do not encrypt any passwords
42-
// phpcs:ignore Magento2.Security.InsecureFunction
43-
return md5($this->getRandomString());
40+
return $this->getRandomString();
4441
}
4542

4643
/**

‎setup/src/Magento/Setup/Test/Unit/Model/CryptKeyGeneratorTest.php

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -45,17 +45,4 @@ public function testStringForHashingIsReadFromRandom()
4545

4646
$this->cryptKeyGenerator->generate();
4747
}
48-
49-
public function testReturnsMd5OfRandomString()
50-
{
51-
$expected = 'fdb7594e77f1ad5fbb8e6c917b6012ce'; // == 'magento2'
52-
53-
$this->randomMock
54-
->method('getRandomString')
55-
->willReturn('magento2');
56-
57-
$actual = $this->cryptKeyGenerator->generate();
58-
59-
$this->assertEquals($expected, $actual);
60-
}
6148
}

‎setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public function testCreateCryptConfigWithoutInput()
7878
$returnValue = $this->configGeneratorObject->createCryptConfig([]);
7979
$this->assertEquals(ConfigFilePool::APP_ENV, $returnValue->getFileKey());
8080
// phpcs:ignore Magento2.Security.InsecureFunction
81-
$this->assertEquals(['crypt' => ['key' => md5('key')]], $returnValue->getData());
81+
$this->assertEquals(['crypt' => ['key' => 'key']], $returnValue->getData());
8282
}
8383

8484
public function testCreateSessionConfigWithInput()

0 commit comments

Comments
 (0)