Problem/Motivation
The MCrypt function is deprecated from PHP 7.1. and is removed from PHP 7.2.
The the recommended version of this module at the time of this writing (7.x-2.x), has MCrypt as the default.
It is proposed to move MCrypt to a separate project in 7.x-3.x at #2554093: Roadmap to Encrypt 7.x-3.0. In the meantime, it might be better to throw an exception instead of die outright if MCrypt is not available.
Proposed resolution
Makeopenssl(AES) an built-in encryption method.Throw an exception if MCrypt is required and not available.Create fallback to decrypt legacy data encrypted withmcrypt_aes_cbcusingopenssl.- Do not set MCrypt as the default. (Do not have any default encryption method.)
- Set status of
mcrypt_aes_cbcto deprecated ihook_FILENAME_encrypt_encryption_methods(). - Fix tests.
- Provide upgrade path to convert legacy data.
Remaining tasks
See list above. Strikeout indicate that a task is completed.
User interface changes
Not having any defaults require users to set up a default profile. Prior versions did not require this step.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | Screenshot from 2019-03-18 11-03-04.png | 59.32 KB | manuel garcia |
| #6 | 2942898-6.patch | 1.51 KB | manuel garcia |
| #6 | interdiff-2942898-2-6.txt | 722 bytes | manuel garcia |
| #2 | 2942898-2-encrypt-7.x-2.x-mcrypt-not-found.patch | 1.54 KB | alberto56 |
Comments
Comment #2
alberto56 commentedHere is a patch which does this.
Comment #4
manuel garcia commentedTriggered the tests again to see where we stand with this.
Comment #5
manuel garcia commentedComment #6
manuel garcia commentedThis should get rid of the test failures I hope
Comment #7
manuel garcia commentedmcrypt is gone on PHP 7.2, and support for PHP 7.1 ends on December 1st of this year, so this is not going to be enough pretty soon...
Comment #8
manuel garcia commentedComment #9
manuel garcia commentedHere's a log from our travis build on PHP 7.2:
Comment #10
manuel garcia commentedSo the tests are failing on PHP 7.2 because we're using
mcrypt_aes_cbcas the default encryption profile.PHP 7.2 recommends using
libsodiummoving forward: https://secure.php.net/manual/en/book.sodium.phpSo we should probably:
Comment #11
manuel garcia commentedCurrently on PHP7.2 the module installs without errors, however the default configuration is still using mcrypt:
Comment #12
rlhawkAn encryption method plugin for Sodium already exists: https://www.drupal.org/project/sodium/
Comment #13
manuel garcia commentedThanks @rlhawk that's great news. How do you think we should handle the default enryption profile going forward?
I also wanted to point out that mcrypt isn't actually gone, just moved to pecl. People could still install it, for PHP 7.2:
Comment #14
gisleIsn't throwing an exception if it does not exist a bit heavy-handed?
We've already have support for alternatives (e.g. OpenSSL and Sodium).
And as pointed out in #13 - people can still install it from PECL (for example to convert legacy data to a newer encryption method).
There should be no requirement for mcrypt to exist or to be available, as future use of mcrypt is depreciated. But removing it altogether as an encryption method would be disruptive to legacy data.
I think the best plan for moving forwards with this is just to keep the plugin, but no longer have it pre-configured as the default encryption method.
We also need to adjust the tests to account for this.
Comments please.
Comment #15
manuel garcia commentedThanks @gisle for taking a look here, some of my thoughts:
encrypt_mcryptat some point, perhaps on7.x-3.xif we ever get there.Comment #16
gisleManuel Garcia wrote:
I shall keep it as a plugin for now (with a depreciation warning). There need to be a stable 7.x-2.4-release with an upgrade path before we can consider removing it. I think that if we're going to move it to a third party module, it should happen in the 7.x-3.x branch. There is a separate issue for this: #2554093: Roadmap to Encrypt 7.x-3.0, so the discussion of what to do with mcrypt in that release should take place there.
I'm testing now on a site with PHP 7.2 and no mcrypt. I am not getting a WSOD, just the same display as the one you posted in comment #11. The radiobutton for any method (including mcrypt) is already unselectable unless preticked. So I believe removing it as the default selection should suffice. If you think there is a situation where we will get a WSOD with the library missing, please provide the steps required to reproduce.
I am committed to make the Drupal 7 branch as similar to the Drupal 8 branch as possible, so I think it makes sense to follow the lead from the Drupal 8 version.
Comment #17
manuel garcia commentedThanks @gisle for the detailed responses,
+1
I've not directly tested this, but I assume that an existing site, which is using an mcrypt encrytion method, say using
webform_encryptor something similar, and they upgrade to PHP 7.2, then when they try to decrypt a value or encrypt a value (making a new webform submission for example), then it probably will throw a fatal error I think.Sounds good to me :)
Comment #18
cellar door commentedI think the best way to handle this is making sure there's a try/catch on encrypt so that if there's an exception thrown from not having mcrypt then we can gracefully fail with an error message.
There will still need to be a way to notify people of the deprecation of mcrypt and likely some support steps on how to decrypt/re-encrypt with a new encryption profile setup using openssl.
Comment #19
gisleI'll try to get this one resolved.
Comment #20
gisleAdded #2421921: Update all encryption plugins to use base64 so data can be saved to database as a related issue. It looks like all non-depreciated methos default to base64 now, but should make sure as part of resolving this one.
Comment #22
gisleCellar Door wrote:
I agree. I've verified that there is a WSOD if the module is handed legacy data encrypted with
mcrypt_aes_cbcwhen using plain vanilla PHP 7.2, and that the patch in #6 fixes that. I've just pushed this patch to 7.x-2.x-dev.True. I am leaving status set to "Needs work" for this reason.
However:
'deprecated' => TRUEinhook_FILENAME_encrypt_encryption_methods().opensslto decrypt legacy data encrypted withmcrypt_aes_cbc. Such a fallback will be pushed in the next snapshot.(I think legacy data should be converted by means of an upgrade path, but the fallback may be used as a stopgap.)
Comment #23
gisleUpdated summary with current status.
Comment #24
gisleComment #26
gisleFallback to decrypt legacy data encrypted with
mcrypt_aes_cbcusingopenssl(AES) just pushed to latest snapshot of 7-x.2.x-dev.Comment #27
q2_faith commentedI don't think that hard-coded fallback from
mcrypt_aes_cbctoopensslit's a flexible solution. Because site owner can use another AES compatible plugin.Comment #28
gisleIt is not supposed to be a flexible solution. It supposed to offer a solution that will work in the majority of cases (because the openssl method is included in any vanilla PHP configuration - and even if openssl is missing, it will fail with a meaningful error message.)
This issue isn't done (see issue summary). We will eventually offer an upgrade path where the site owner can chose another AES compatible plugin (or whatever). We're not just there yet - this will not be done until step 7.
Comment #29
tom.smith commentedI may be able to help out a little with part 7. I am currently using this in conjunction with the Field encrypt module and am writing a patch to help migrate our data. We have just shy of 100k encrypted entries so I am currently evaluating 3 migration paths.
Upgrade while on php 7.1
1. drush command to decrypt via mcrypt and encrypt via sodium. (Would require sodium.so added to server. this is my current favorite)
Upgrade while on php 7.2
2. drush command to decrypt mcrypt data via openssl and encrypt via sodium.
3. Write fallback to do lazy upgrade. (This is my least favorite option)
I understand my migration is very specific to the the Field encrypt module and moving to sodium but hopefully my patch will be helpful in some way. Even if it is just a good laugh :)
Comment #30
gisle@tom.smith,
great!
I am thankful for any help.
If I understand things correctly, your first two options (drush commands) can be implemented without altering Encrypt. If this is correct, just go ahead and we'll package the scripted drush commands with the project when they are ready.
If this is not correct, just say what you need from me, and I'll do my best.
Comment #31
tom.smith commented@gisle,
That is correct. It should not require any changes to Encrypt.
Comment #32
steven jones commentedJust changing the issue title to be a little more accurate.
As for providing an upgrade path, am I right in thinking that really since encrypt module is pretty much an API module that provides two main functions (encrypt/decrypt) that actually the upgrade path for legacy data needs to go in the modules using encrypt, and that it would probably look a lot like https://git.drupalcode.org/project/webform_encrypt/blob/7.x-1.x/webform_... for example, but doing an immediate encrypt on the data too?
Comment #33
gisleSteven Jones wrote:
Well, there is nothing stopping the maintainers of modules that build on Encrypt from doing so, following the example you posted (Webform encrypt).
However, I still think that Encrypt should come with some sort of upgrade path. This may be drush-scripts to decrypt and then re-encrypt as suggested in comment #29, or it may just be instructions describing how to go about decrypting all data, and then re-encrypting with the method of your choice.
Comment #34
divyesh19@gisle and @tom.smith, any update on the scripted drush commands as mentioned in comments #29 and #30? (or a rough idea on when would we be reaching to step 7 of providing an upgrade path?)
Comment #35
steven jones commentedWhat I went with in the end was this:
hook_cronimplementation that is currently re-encrypting the millions of rows of data I have from the older config to the new one. An update hook simply would have taken too long. It could have been a custom Drush command, since the implementation of the hook is about as simple as it gets. Project constraints forced me to use ahook_cronimplementation. Note that this is webform_encrypt's data, encrypt module doesn't manage any actual encrypted data in the database as such.hook_requirementsthat checks to see if there's any more data to convert and pops a warning onto admin/reports/status if there is.So, I reckon the upgrade path needs to be a bunch of documentation and some useful drush/cron commands for webform_encrypt and any other modules that use the data that re-encrypt everything at the right point in the process.
Here's my hook_cron implementation for those that are interested.
To be general purpose it would need to support identifying the legacy data in a more configurable way, and probably setting an amount to do per invocation, but otherwise it's pretty straightforward:
Comment #36
moshe weitzman commentedThanks @steven. If one only uses Encrypt for TFA, is a data migration needed when switching from Mcrypt to OpenSSL? Maybe backup codes become invalid without a migration.
Comment #37
steven jones commented@moshe weitzman I'm not sure how Two-factor Authentication (TFA) module uses mcrypt, but yeah, if it's simply encrypting/decrypting short lived data, then you could probably get away with moving the encryption and invalidating any logins in progress (if that's what it's doing).
Comment #38
jenna.tollersonJust FYI for any Acquia-hosted folks looking at this issue, Acquia Security has rejected the use of PECL mcrypt on their platform. Their recommendation is to move to OpenSSL. That is the entirety of the guidance we received.
Comment #39
powrsurg commentedThere is an issue with proposed resolution item #3. While this module does attempt to decrypt data that was encrypted with mycrypt, the openssl decrypt method fails to decrypt the data with errors like:
error:03000082:digital envelope routines::invalid key length
I have attempted this in multiple environments and this seems pretty consistent.
Comment #40
ptmkenny commentedDevelopment of Encrypt for Drupal 7 has ended.