Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions app/Config/Encryption.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ class Encryption extends BaseConfig
*/
public string $key = '';

/**
* --------------------------------------------------------------------------
* Previous Encryption Keys
* --------------------------------------------------------------------------
* If you want to enable decryption using previous keys, set them here.
* See the user guide for more info.
*/
public string $previousKeys = '';
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just make it string|array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, you mean like anyone can pass both comma-seperated string or array of keys and both should work, like that?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. For working with .env files, the string syntax will be preferred. Arrays will still be allowed, but they must follow the usual array rules - specifically, keys must be explicitly declared for values seeded from the .env file. This will enable us to transform strings into arrays safely.


/**
* --------------------------------------------------------------------------
* Encryption Driver to Use
Expand Down
3 changes: 3 additions & 0 deletions env
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@

# encryption.key =

# Previous keys fallback; comma-separated list
# encryption.previousKeys =

#--------------------------------------------------------------------
# SESSION
#--------------------------------------------------------------------
Expand Down
30 changes: 23 additions & 7 deletions system/Config/BaseConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -130,18 +130,34 @@ public function __construct()
foreach ($properties as $property) {
$this->initEnvValue($this->{$property}, $property, $prefix, $shortPrefix);

if ($this instanceof Encryption && $property === 'key') {
if (str_starts_with($this->{$property}, 'hex2bin:')) {
// Handle hex2bin prefix
$this->{$property} = hex2bin(substr($this->{$property}, 8));
} elseif (str_starts_with($this->{$property}, 'base64:')) {
// Handle base64 prefix
$this->{$property} = base64_decode(substr($this->{$property}, 7), true);
if ($this instanceof Encryption) {
if ($property === 'key') {
$this->{$property} = $this->parseEncryptionKey($this->{$property});
} elseif ($property === 'previousKeys') {
$keysArray = array_map(trim(...), explode(',', $this->{$property}));
$parsedKeys = [];

foreach ($keysArray as $key) {
$parsedKeys[] = $this->parseEncryptionKey($key);
}
$this->{$property} = implode(',', $parsedKeys);
Copy link
Member

Choose a reason for hiding this comment

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

This is a very bad idea. We can't safely implode encryption keys because once they are decoded to raw bytes, we lose a reliable way to explode them again without corrupting the keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. So, I think the other way is, keep this as it is, then, when needed, parse those keys on the fly. Would that work? But for that, we need this parseEncryptionKeys function to be accessible from both handlers.

Copy link
Member

Choose a reason for hiding this comment

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

We can parse encryption keys as we do now. Just assign an array, not a string (don't use implode). It will be allowed if we declare $previousKeys in the config as string|array.

$keysArray  = is_string($this->{$property}) ? array_map(trim(...), explode(',', $this->{$property})) : $this->{$property};
$parsedKeys = [];

foreach ($keysArray as $key) {
    $parsedKeys[] = $this->parseEncryptionKey($key);
}

$this->{$property} = $parsedKeys;

}
}
}
}

protected function parseEncryptionKey(string $key): string
{
if (str_starts_with($key, 'hex2bin:')) {
return hex2bin(substr($key, 8));
}
if (str_starts_with($key, 'base64:')) {
return base64_decode(substr($key, 7), true);
}

return $key;
}

/**
* Initialization an environment-specific configuration setting
*
Expand Down
19 changes: 13 additions & 6 deletions system/Encryption/Encryption.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ class Encryption
*/
protected $key;

/**
* Comma-separated list of previous keys for fallback decryption.
*/
protected string $previousKeys = '';

/**
* The derived HMAC key
*
Expand Down Expand Up @@ -91,9 +96,10 @@ public function __construct(?EncryptionConfig $config = null)
{
$config ??= new EncryptionConfig();

$this->key = $config->key;
$this->driver = $config->driver;
$this->digest = $config->digest ?? 'SHA512';
$this->key = $config->key;
$this->previousKeys = $config->previousKeys;
$this->driver = $config->driver;
$this->digest = $config->digest ?? 'SHA512';

$this->handlers = [
'OpenSSL' => extension_loaded('openssl'),
Expand All @@ -116,9 +122,10 @@ public function __construct(?EncryptionConfig $config = null)
public function initialize(?EncryptionConfig $config = null)
{
if ($config instanceof EncryptionConfig) {
$this->key = $config->key;
$this->driver = $config->driver;
$this->digest = $config->digest ?? 'SHA512';
$this->key = $config->key;
$this->previousKeys = $config->previousKeys ?? '';
$this->driver = $config->driver;
$this->digest = $config->digest ?? 'SHA512';
}

if (empty($this->driver)) {
Expand Down
49 changes: 47 additions & 2 deletions system/Encryption/Handlers/OpenSSLHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ class OpenSSLHandler extends BaseHandler
*/
protected $key = '';

/**
* List of previous keys for fallback decryption.
*/
protected string $previousKeys = '';

/**
* Whether the cipher-text should be raw. If set to false, then it will be base64 encoded.
*/
Expand Down Expand Up @@ -127,8 +132,48 @@ public function decrypt($data, #[SensitiveParameter] $params = null)
throw EncryptionException::forNeedsStarterKey();
}

$result = false;

try {
$result = $this->decryptWithKey($data, $this->key);
} catch (EncryptionException $e) {
$exception = $e;
}

if ($result === false && $this->previousKeys !== '') {
foreach (explode(',', $this->previousKeys) as $previousKey) {
try {
$result = $this->decryptWithKey($data, $previousKey);
if ($result !== false) {
return $result;
}
} catch (EncryptionException) {
// Try next key
}
}
}

if (isset($exception)) {
throw $exception;
}

return $result;
}

/**
* Decrypt the data with the provided key
*
* @param string $data
* @param string $key
*
* @return false|string
*
* @throws EncryptionException
*/
protected function decryptWithKey($data, #[SensitiveParameter] $key)
{
// derive a secret key
$authKey = \hash_hkdf($this->digest, $this->key, 0, $this->authKeyInfo);
$authKey = \hash_hkdf($this->digest, $key, 0, $this->authKeyInfo);

$hmacLength = $this->rawData
? $this->digestSize[$this->digest]
Expand All @@ -152,7 +197,7 @@ public function decrypt($data, #[SensitiveParameter] $params = null)
}

// derive a secret key
$encryptKey = \hash_hkdf($this->digest, $this->key, 0, $this->encryptKeyInfo);
$encryptKey = \hash_hkdf($this->digest, $key, 0, $this->encryptKeyInfo);

return \openssl_decrypt($data, $this->cipher, $encryptKey, OPENSSL_RAW_DATA, $iv);
}
Expand Down
52 changes: 49 additions & 3 deletions system/Encryption/Handlers/SodiumHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@
*/
protected $key = '';

/**
* List of previous keys for fallback decryption.
*/
protected string $previousKeys = '';

/**
* Block size for padding message.
*
Expand Down Expand Up @@ -80,6 +85,48 @@
throw EncryptionException::forNeedsStarterKey();
}

$result = false;

try {
$result = $this->decryptWithKey($data, $this->key);
sodium_memzero($this->key);
} catch (EncryptionException $e) {
$exception = $e;
sodium_memzero($this->key);
}

if ($result === false && $this->previousKeys !== '') {
foreach (explode(',', $this->previousKeys) as $previousKey) {
try {
$result = $this->decryptWithKey($data, $previousKey);
if (isset($result)) {
return $result;
}
} catch (EncryptionException) {
// Try next key
}
}
}

if (isset($exception)) {
throw $exception;
}

return $result;

Check failure on line 115 in system/Encryption/Handlers/SodiumHandler.php

View workflow job for this annotation

GitHub Actions / PHP Static Analysis

Variable $result might not be defined.
Comment on lines +88 to +115
Copy link
Member

Choose a reason for hiding this comment

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

I find this hard to read in its current form. Wrapping the logic in a callback method might make it clearer and easier to maintain, especially since the existing method logic won't change.

}

/**
* Decrypt the data with the provided key
*
* @param string $data
* @param string $key
*
* @return string
*
* @throws EncryptionException
*/
protected function decryptWithKey($data, #[SensitiveParameter] $key)
{
if (mb_strlen($data, '8bit') < (SODIUM_CRYPTO_SECRETBOX_NONCEBYTES + SODIUM_CRYPTO_SECRETBOX_MACBYTES)) {
// message was truncated
throw EncryptionException::forAuthenticationFailed();
Expand All @@ -90,7 +137,7 @@
$ciphertext = self::substr($data, SODIUM_CRYPTO_SECRETBOX_NONCEBYTES);

// decrypt data
$data = sodium_crypto_secretbox_open($ciphertext, $nonce, $this->key);
$data = sodium_crypto_secretbox_open($ciphertext, $nonce, $key);

if ($data === false) {
// message was tampered in transit
Expand All @@ -106,7 +153,6 @@

// cleanup buffers
sodium_memzero($ciphertext);
sodium_memzero($this->key);

return $data;
}
Expand All @@ -120,7 +166,7 @@
*
* @throws EncryptionException If key is empty
*/
protected function parseParams($params)
protected function parseParams(#[SensitiveParameter] $params)
{
if ($params === null) {
return;
Expand Down
Loading