-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(encryption): Add previous keys fallback feature #9853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.7
Are you sure you want to change the base?
Changes from all commits
7837aa0
be93000
972ad6e
486bf57
5f45c6f
55ed3a7
5e53f06
68e05ba
772bc29
2ad2e97
c3f5bae
97e32f4
6a8694f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a very bad idea. We can't safely
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 $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 | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,11 @@ | |
| */ | ||
| protected $key = ''; | ||
|
|
||
| /** | ||
| * List of previous keys for fallback decryption. | ||
| */ | ||
| protected string $previousKeys = ''; | ||
|
|
||
| /** | ||
| * Block size for padding message. | ||
| * | ||
|
|
@@ -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; | ||
|
Comment on lines
+88
to
+115
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
|
@@ -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 | ||
|
|
@@ -106,7 +153,6 @@ | |
|
|
||
| // cleanup buffers | ||
| sodium_memzero($ciphertext); | ||
| sodium_memzero($this->key); | ||
|
|
||
| return $data; | ||
| } | ||
|
|
@@ -120,7 +166,7 @@ | |
| * | ||
| * @throws EncryptionException If key is empty | ||
| */ | ||
| protected function parseParams($params) | ||
| protected function parseParams(#[SensitiveParameter] $params) | ||
| { | ||
| if ($params === null) { | ||
| return; | ||
|
|
||
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. For working with
.envfiles, 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.envfile. This will enable us to transform strings into arrays safely.