Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.It would be nice if rather than hard-coding the one mcrypt plugin, we could use any available mcrypt algorithms available on the system.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | encrypt_provide_a_generic-2418753-12.patch | 10.87 KB | rlhawk |
| #10 | encrypt_provide_a_generic-2418753-10.patch | 8.43 KB | rlhawk |
| #8 | encrypt_provide_a_generic-2418753-8.patch | 6.95 KB | rlhawk |
| #7 | provide_a_generic-2418753-7.patch | 6.88 KB | rlhawk |
| #6 | 2418753-generic-mcrypt-plugin-deprecated.patch | 5 KB | Dave Reid |
Comments
Comment #1
Dave ReidComment #2
Dave ReidScreenshot in action.
Comment #3
Dave ReidSorry, empty patch.
Comment #4
rlhawkI like this idea a lot and I definitely think we should do it, but I have a few thoughts.
1) It would be preferable to consolidate the mcrypt encryption methods, so that only one appears in the UI. In order to do that, we'd need to provide backward compatibility to allow decryption of data that was encrypted with the original mcrypt method. Perhaps it would make sense to support methods that are hidden from the UI, but that can still be used to decrypt existing data. Ideally, this would actually just be a sort of alias: mcrypt_rij_256, for instance would just alias to mcrypt with rijndael-256 chosen as the cipher.
2) For cipher settings, currently the value returned in the form is a number (12 for rijndael-256, for example), but I assume that we should actually be storing the name ("rijndael-256").
3) By offering other, less-secure ciphers as options, we will be encouraging poor security practices. Ultimately, if a user is trying to meet compliance requirements, they will need to use whatever cipher is specified, but that is generally going to be some version of Rijndael, or one of the other finalists for AES (Serpent, Twofish, RC6, and MARS). We shouldn't be encouraging the use of DES or Blowfish, for instance. Would it make sense to allow filtering of what ciphers appear in the list, using a hook that can be implemented in a custom module?
4) Maybe we should also consider allowing mode (ecb, cbc, cfb, ofb, nofb, or stream) to be selected. The current mcrypt implementation is hard-coded to ecb. Again, we will probably want to allow filtering of what modes appear.
Comment #5
gregglesAgreed that we should ideally expose the mode and switch away from ecb as the default.
I'm not sure how to give the best advice about which are "good" methods to people, but the defaults should be something better.
Comment #6
Dave ReidThe results of mcrypt_list_algorithms() vary, so I wasn't sure we could rely on any specific algo, but I figured from the perspective of "replace the existing plugin with this one" that it would make sense to default to rijndael-256.
Switching the modes could be a follow-up.
If we add a 'hidden' property support to plugins, then when I edit an existing encryption configuration, I will get an "Invalid choice" error message until I select a new encryption method. This may actually be a good thing since we want to get people to use the new plugin.
If we just add a 'deprecated' message to the title of the existing plugin, I can still select it, and it may be confusing to the end-user.
I've provided the two different solutions, and fixed the bug of the value of the 'algo' option not being saved properly.
Comment #7
rlhawkI think the second solution is more user-friendly. It's certainly not urgent that anybody switch to the new method, so we can continue to support the old one, but it should be hidden unless it already is associated with a configuration. Here's a new patch that builds on solution #2. I've added a new setting for the encryption method plugin, called "deprecated," which is false by default. Mycrypt AES-256 is flagged as deprecated, so it will only appear if that method was previously selected for the configuration, but with a notice about deprecation. Otherwise, it won't appear in the list.
Comment #8
rlhawkHere's a new patch that adds the new generic Mcrypt method and deprecates the old one. It also implements a solution that Greg and I discussed at DrupalCon last month, whereby administrators can define a variable with the list of allowed ciphers; otherwise, only the three versions of Rijndael are available.
Comment #10
rlhawkBad patch. Trying again.
Comment #11
rlhawkIgnore the previous patch. I will be uploading a replacement shortly.
Comment #12
rlhawkHere's a replacement patch. The encryption wasn't being called correctly, so no matter which cipher you chose, it always used Rijndael 256. I also altered the Mcrypt test to use the new generic plugin.
Comment #13
rlhawkThis is proving to be more challenging than just providing more options for cipher, especially if we're also going to allow the mode to be selected, since some ciphers don't support some modes. For example, with the current patch, if you choose RC4 (arcfour) as the cipher, encryption will fail and you'll get a lot of PHP warnings, because RC4 only works with Stream mode and not with ECB. If we're going to implement this functionality, we need to make the plugin much more robust, so users can't choose ciphers and/or modes that won't work.
Comment #14
rlhawkGiven the varying requirements for different ciphers and modes (key length, IV, etc.), allowing users to choose a cipher via a method settings field is not going to be possible. Modules that provide new encryption methods that use Mcrypt are going to need to provide very specific implementations of them. This way developers are free to do whatever they want for their specific needs, but we are not promoting outdated or insecure encryption algorithms.
For Encrypt 3.x, my goal is that the Mcrypt method will be a recommended implementation that can be extended by other modules that want to provide variations, such as using a different cipher.