Remove hard dependency on mcrypt #5496
Conversation
Using a pluggable system will let us reduce our reliance on mcrypt and remove the hard requirement on mcrypt. It will also enable us to have an SSL adapter for more maintained library implementations.
OpenSSL does not support 256bit blocksized AES, this means we cannot provide a compatible implementation or rijndael. Make the engine() method accept an instance so that developers can force mcrypt/openssl as necessary.
Sort out the nasty problems that were preventing Mcrypt and OpenSSL backends from decrypting each others ciphertext. Add a test to show how each engine can decode its own and the other's cipher texts.
Also include a test to show that 2.x data can still be decrypted now.
I cocked up the tests the first time around.
This will be the least incompatible change for users who have both openssl and mcrypt installed. Encrypted data from old mcrypt cannot be decrypted with openssl as the padding scheme is incorrect (null bytes instead of PKCS#7).
| if ($instance === null && static::$_instance === null) { | ||
| if (extension_loaded('mcrypt')) { | ||
| $instance = new Mcrypt(); | ||
| } elseif (extension_loaded('openssl')) { |
There was a problem hiding this comment.
Shouldn't availability of openssl be checked first as the point of the change is to avoid mcrypt usage? Those upgrading their app and requiring mcrypt can set is explicitly in bootstrap by doing Security::engine(new Mcrypt()); isn't it?
There was a problem hiding this comment.
Given the point that it is easily possible to switch the default engine, I would also rather use openssl as default and state the change in the migration guide.
There was a problem hiding this comment.
Those are both good points, but my concern is this.
- Install has both mcrypt and openssl installed.
- App has data encrypted with existing code (mcrypt).
- Upgrade CakePHP, now no data can be decrypted. Furthermore, there is no error.
Mcrypt is not necessarily bad. It just won't be supported on some distros in the future. The goal of these changes was twofold.
- Remove the hard dependency on mcrypt. This will allow CakePHP to install on systems with no mcrypt, and openssl only.
- Allow openssl to be used, as it is actually faster at encrypting data.
If a developer has both extensions installed, they can force the implementation they want with engine().
That's what migration guides are for :P |
| * | ||
| * @copyright Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org) | ||
| * @link http://cakephp.org CakePHP(tm) Project | ||
| * @since 0.10.0 |
There was a problem hiding this comment.
3.0.0 and in the other one also
@ADmad Migrating encrypted data is generally not a simple/easy process so I'd like to avoid forcing people's hand if possible. mcrypt still works fine, and if it is installed/installable on your distro then there is really no reason to switch. |
|
@markstory I think the point is that people can easily configure mcrypt if they have existing data and not need to migrate, but I can easily see that being something people don't read and then get mad about. |
I didn't mean migrating encrypted data. I meant stating in the migration guide that the default crypto has changed and if they are upgrading they should configure Security class to use Mycrpt instead. Those migrating will have to carefully read about the changes anyway. Eg. for Cookies also the default encryption has changed. |
|
Ideally, there is also already a commented out bootstrap code ready to be used: |
|
I'm +1 on making OpenSSL the default. As mentioned above, people already have to read carefully to migrate and changing it back to mcrypt is just a single line change. For new installations, it won't make a difference and making OpenSSL the default more clearly states that it is the best way going forward. |
Because openssl will be better in the long term, make it the default implementation used if the openssl extension is installed. While this will cause some short term pain, app developers can switch the app around by including one line in their bootstrap.
|
Ok you guys have convinced me, I've updated the default engine, and will add a commented out config option to the app skeleton. |
| * | ||
| * @copyright Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org) | ||
| * @link http://cakephp.org CakePHP(tm) Project | ||
| * @since 0.10.0 |
|
@markstory thanks! I appreciate your concern for users that "miss" that part during migration, just think this is the better way going forward. Haven't checked, but has a "big red note" being added to the migration guide too? |
|
@thaJeztah The whole 3.0 migration guide can have a red background |
|
Open question: should the mcrypt check on the app home page be updated as well, maybe even checking openssl? If so, I can do that. |
|
@bravo-kernel Yes it should be updated. First check for openssl and show green, if unavailable check for mcrypt and show green or red if both are unavailable. |
|
ok, will do |
|
Any other feedback on this? I can work on the docs changes tonight. |
|
👍 |
1 similar comment
|
👍 |
Remove hard dependency on mcrypt
From #5440 a few key distros (redhat) will be dropping mcrypt support. We can and should switch to openssl earlier. openssl is more widely maintained and easier to use.
I've left the default to be mcrypt mostly out of backwards compatibility issues. Because our earlier implementation of
encrypt()did not use PKCS#7 the data encrypted by it cannot be decrypted with openssl. For people with both extensions, it would be best if their code didn't break.