convenient
commented
1 month ago Hello @andrew-ebsco
On the surface of it that makes sense. If you have successfully migrated every encrypted pieces of data in your system to use the new keys at the higher index, then essentially nothing should be looking at the old key index and leaving it in place shouldn't hurt.
However that is an assumption that is hard to verify, and this is actually about more than just the data you have encrypted in your system.
The hotfix release today (which achieves very much the same thing as this module did) looks like so
diff --git a/vendor/magento/module-jwt-user-token/Model/SecretBasedJwksFactory.php b/vendor/magento/module-jwt-user-token/Model/SecretBasedJwksFactory.php
--- a/vendor/magento/module-jwt-user-token/Model/SecretBasedJwksFactory.php (revision 022e64b08a88658667bc2d6b922eada2b7910965)
+++ b/vendor/magento/module-jwt-user-token/Model/SecretBasedJwksFactory.php (revision 8d2b0c1c6b421cdcd7f62a48a5edc9b0211d92a2)
@@ -35,6 +35,7 @@
public function __construct(DeploymentConfig $deploymentConfig, JwkFactory $jwkFactory)
{
$this->keys = preg_split('/\s+/s', trim((string)$deploymentConfig->get('crypt/key')));
+ $this->keys = [end($this->keys)];
//Making sure keys are large enough.
foreach ($this->keys as &$key) {
$key = str_pad($key, 2048, '&', STR_PAD_BOTH);In this scenario it wasn't just encrypted values that were using the old key, but other parts of the system were using the encryption keys from env.php to initialise other logic.
This SecretBasedJwksFactory.php has been patched now, and should help secure against cosmicsting however that does not account for
- the rest of the magento application
- any third party software you have plugged in
The only 100% verifiable way to properly verify the old key is no longer in use is to remove it from the crypt/key section. This module moves it to crypt/invalidated_key as both a reference point, and so that the media gallery can still be generated based on the original values.
The process of invalidating your key does sound scary however consider the following
- If you have properly migrated all your encrypted data from
0:3:abc123to1:3:xyz456then then it's not really needed incrypt/keyanyway- You should enable the logging on your production system after you think you are complete, so that you can verify you haven't missed any data
- then invalidate
At this stage we must basically assume our keys are public and that bad actors will be trying to find things to do with them. If the key is still active and plugged into your system as part of crypt/key, then there are chances that some code may pick it up and execute it in manners that are not desirable. I would not assume that we're safe because we blocked the current known attack vector, and would rather have a repeatable process to actually rotate encryption keys which includes invalidating the old one.
andrew-ebsco
Hello Gene Commerce Team,
Thank you very much for the module. It has been very helpful for rotating encryption keys and re-encrypting values in our database. However, I have a question about the invalidate key process. What is the functional advantage of placing the old crypt key in the 'invalidate_key' section? Basically, I want to know what would be the downside of just leaving the old crypt key in the crypt/key config. If all the encrypted values have been upgraded to the new key, then the old one will not be used anyway. It is unclear from the documentation why invalidating the key is an improvement. I appreciate any feedback or clarification.
Thanks!
Andrew