-
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
Changes from 12 commits
7837aa0
be93000
972ad6e
486bf57
5f45c6f
55ed3a7
5e53f06
68e05ba
772bc29
2ad2e97
c3f5bae
97e32f4
6a8694f
2f58622
628c41d
a1902e1
e1331e2
5f63c3f
2e871d2
bd6e045
e1315e3
c76a1dc
f1d8312
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 |
|---|---|---|
|
|
@@ -14,7 +14,6 @@ | |
| namespace CodeIgniter\Encryption\Handlers; | ||
|
|
||
| use CodeIgniter\Encryption\Exceptions\EncryptionException; | ||
| use SensitiveParameter; | ||
|
|
||
| /** | ||
| * SodiumHandler uses libsodium in encryption. | ||
|
|
@@ -31,6 +30,11 @@ | |
| */ | ||
| protected $key = ''; | ||
|
|
||
| /** | ||
| * List of previous keys for fallback decryption. | ||
| */ | ||
| protected string $previousKeys = ''; | ||
|
|
||
| /** | ||
| * Block size for padding message. | ||
| * | ||
|
|
@@ -41,7 +45,7 @@ | |
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| public function encrypt(#[SensitiveParameter] $data, #[SensitiveParameter] $params = null) | ||
|
patel-vansh marked this conversation as resolved.
|
||
| public function encrypt($data, $params = null) | ||
| { | ||
| $this->parseParams($params); | ||
|
|
||
|
|
@@ -72,14 +76,56 @@ | |
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| public function decrypt($data, #[SensitiveParameter] $params = null) | ||
| public function decrypt($data, $params = null) | ||
| { | ||
| $this->parseParams($params); | ||
|
|
||
| if (empty($this->key)) { | ||
| 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; | ||
|
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.
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. I tried to make it a more readable, please check now.
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 was thinking of adding this method to the protected function tryDecryptWithFallback($data, #[SensitiveParameter] $params, callable $decryptCallback)
{
try {
return $decryptCallback($data, $params);
} catch (EncryptionException $e) {
if ($this->previousKeys === []) {
throw $e;
}
if (is_string($params) || (is_array($params) && isset($params['key']))) {
throw $e;
}
foreach ($this->previousKeys as $previousKey) {
try {
$previousParams = is_array($params)
? array_merge($params, ['key' => $previousKey])
: $previousKey;
return $decryptCallback($data, $previousParams);
} catch (EncryptionException) {
continue;
}
}
throw $e;
}
}and then using it like: public function decrypt($data, #[SensitiveParameter] $params = null)
{
return $this->tryDecryptWithFallback($data, $params, function ($data, $params): string {
// original method content
});
}From my perspective, this is clearer, as the original methods remain largely unchanged and a single shared method handles both handlers.
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. Yeah, adding it in So, will it be okay to introduce a new method in base handler?
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. Yes, as long as it's universal and can be used with any handler.
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. One thing I would like to note here is, in the
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. I was thinking something like this for protected function tryDecryptWithFallback($data, #[SensitiveParameter] $params, callable $decryptCallback)
{
$exception = null;
try {
$result = $decryptCallback($data, $params);
// Handle case where decryption returns false (like OpenSSL)
if ($result !== false) {
return $result;
}
} catch (EncryptionException $e) {
$exception = $e;
}
// Only try fallback keys if:
// 1. We have previous keys configured
// 2. No custom key was provided in params
if ($this->previousKeys === []) {
if ($exception !== null) {
throw $exception;
}
return false;
}
if (is_string($params) || (is_array($params) && isset($params['key']))) {
if ($exception !== null) {
throw $exception;
}
return false;
}
// Try each previous key
foreach ($this->previousKeys as $previousKey) {
try {
$previousParams = is_array($params)
? array_merge($params, ['key' => $previousKey])
: $previousKey;
$result = $decryptCallback($data, $previousParams);
// Return on success (not false)
if ($result !== false) {
return $result;
}
} catch (EncryptionException) {
// Continue to next key
continue;
}
}
// All keys failed - throw original exception or return false
if ($exception !== null) {
throw $exception;
}
return false;
}What are your thoughts on 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. Good catch on
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. I've done that. Please check. |
||
| } | ||
|
|
||
| /** | ||
| * Decrypt the data with the provided key | ||
| * | ||
| * @param string $data | ||
| * @param string $key | ||
| * | ||
| * @return string | ||
| * | ||
| * @throws EncryptionException | ||
| */ | ||
| protected function decryptWithKey($data, $key) | ||
| { | ||
| if (mb_strlen($data, '8bit') < (SODIUM_CRYPTO_SECRETBOX_NONCEBYTES + SODIUM_CRYPTO_SECRETBOX_MACBYTES)) { | ||
| // message was truncated | ||
| throw EncryptionException::forAuthenticationFailed(); | ||
|
|
@@ -90,7 +136,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 +152,6 @@ | |
|
|
||
| // cleanup buffers | ||
| sodium_memzero($ciphertext); | ||
| sodium_memzero($this->key); | ||
|
|
||
| return $data; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.