Encryption of pre-existing data

I think we should definetly have a look at this @Rob Holmes, great point.

Let's first try to verify this, could be another critical.

Thanks @Rob Holmes for the analysis here, I think that your conclusion is spot on. We need to be able to find out for each submission if it has been encrypted and with what profile, and we should provide an update hook to migrate existing submissions as well, or else they might get corrupted. Would you mind updating the issue summary with this information, and include the proposed solution?

Bumping priority to critical as this could result in lost data / data inconsistencies / etc.

I've been thinking about this, and I believe that the safest way to deal with this is to not allow enabling encryption on elements that already have data in them.

This approach would be by design so as to protect the integrity of the existing data. I honestly think this is the best solution here, as any other option would mean having the module bulk encrypt existing data, which would not only take a lot of time and testing to implement, but could easily run into unexpected errors, resulting in data loss.

If we can get consensus on this, I'll be happy to prepare a patch when I get some time.

Here is a better approach at this than #6. Instead of disabling the form elements, show just the values and the message.

There would be no harm possible when adding a new webform element to an existing webform that has submissions in it, so allowing to configure webform_encrypt in this case would make sense.

Updating #8 accordingly.

I left a comment yesterday, but for some reason I don't see it here :(

Anyway I completely agree with Manuel on #5.

I will have a look at the patch ASAP (still too early in the morning to switch on the brain :p), but I like the approach described.

I like the approach. I'm going to manually test soon, in the meanwhile.

1. +++ b/src/Element/WebformElementEncrypt.php
   @@ -43,20 +43,45 @@ class WebformElementEncrypt extends FormElement {
   +    $encrypt_value = isset($config[$element_name]['encrypt']) ? $config[$element_name]['encrypt'] : FALSE;
   

   We could use $encrypt_value = isset($config[$element_name]['encrypt']) && $config[$element_name]['encrypt']; here.

2. +++ b/src/Element/WebformElementEncrypt.php
   @@ -43,20 +43,45 @@ class WebformElementEncrypt extends FormElement {
   +    if ($webform_has_submissions && !empty($element_name)) {
   

   Not sure why we use $element_name to check if this is a new element? Is this the best way?

3. +++ b/src/Element/WebformElementEncrypt.php
   @@ -43,20 +43,45 @@ class WebformElementEncrypt extends FormElement {
   +      $message = '<p>'. t('Encryption: %value.', ['%value' => $encrypt_value ? 'Enabled' : 'Disabled']) . '</p>';
   +      $message .= '<p>'. t('Encryption profile: %value.', ['%value' => $encrypt_profile_value ? $encryption_options[$encrypt_profile_value] : 'None']) . '</p>';
   +      $message .= t('In order to prevent data loss, it is only allowed to configure the encryption for this element if there are no submissions. This webform already has submissions.');
   

   I don't really like using markup in code, but that's my personal feeling. Would using the "list" theme be too difficult?

@gambry thanks for the patch review!

@Rob Holmes yeah you're correct, that would be a problem, here my thoughts on the subject:

Disabling encryption on an element then would HAVE to decrypt all entries for that element on the database, as that would be the expected behaviour. I think we could write some tools for dealing with encrypted webform submissions perhaps, but not within the build webform workflow. Perhaps add a link saying "If you want to disable/reconfigure this element's encryption, use this tool."

That's my opinion from a workflow point of view, that said, there are things that would need to happen if we we're ever going to get to that point: Right now is that we do not keep track of whether a submission element is encrypted or not. All we know is that a webform element is configured to be encrypted. Once that's the case, we encrypt new submissions to that element, and assume they are encrypted when viewing them.

In my opinion we should change how we save the data to the encrypted webform elements so that we can check whether or not a value is encrypted, and then we can start building these batch operations around reconfiguring and disabling webform elements' encryption. We should probably create a new issue and get that done before we release the beta. We discussed this on some other issue, cant find it though.

So in any case, I think for now the approach in the latest patch at least prevents data loss, and its the best I can think of doing with what we have...

Opened a new task to track the current issue of knowing whether or not an element is currently encrypted.

#2931711: Tracking an elements encryption status

Thanks @thecraighammond for that. Looks like we're making good progress on #2931711: Tracking an elements encryption status so postponing this until we get that done.

Attached quite a big patch which covers managing (changing, disabling) of elements with encryption profiles as well as ui change @rob-holmes mentioned above. This is patched off the 8.x-1.x branch so includes any uncommitted patches up to this point that were required for this to work (and so might need to be broken up a little or patched from another point if it seems like a good approach).

Trying out a new patch with the same changes - this one passes tests locally. Previous also failed locally (but with a different issue, strangely).

Patch includes both serialisation changes, ability to change or disable profiles and the ui changes mentioned above.

I've added a work in progress patch to #2931711: Tracking an elements encryption status that should just be the serialisation.

Hi guys, had a quick review of the patch in #2925621-19: Encryption of pre-existing data.
Unable to test the main scope of the issue due to two main issues in the patch at the moment.

1. the webform_encrypt.routing.yml file is created in my project root directory, in this case, one above the drupal doc root.
2. The form controllers referenced in the new webform_encrypt.routing.yml file do not exist in the patch, so am unable to test the new bulk operations of changing and disabling encryption.

Also, having a read I can see that a decision has been made on allowing a user to change encryption i.e re-encrypting the data, but what of providing an option to encrypt pre-existing non-encrypted data? Has a consensus been reached on this?

There are checks in place when displaying the data to determine if it data needs to be decrypted or not already, so if we did want to go down the route of encrypting existing non-encrypted data, what about providing an additional "encrypt existing data" tick box in the element_encrypt fieldset to be present if the field has existing data?

Edit: An additional issue I also came across was that webform_ui is a dependency as an error was thrown when attempting to go to the build page with webform_encrypt enabled.

Updated the patch to contain the form controllers and added the extra dependency.

This issue was postponed in favour of #2931711: Tracking an elements encryption status, which is closer to a solution but not yet. In any case is not finished so this should be postponed until the other one is closed.

Also, having a read I can see that a decision has been made on allowing a user to change encryption i.e re-encrypting the data, but what of providing an option to encrypt pre-existing non-encrypted data? Has a consensus been reached on this?

I don't think there is consensus on any of them. They both have downsides (re-encrypting data will corrupt data with Asymmetric encryption, with pre-existing data there is no way to know if data is already encrypted and with which profile and #2931711: Tracking an elements encryption status will be a starting point to be able to).

I guess the issue summary needs to be update to reflect what this issue is about and the split in favour of #2931711: Tracking an elements encryption status. It should also mention the thoughts on #13, and the suggestions on #16.

We can use EncryptionProfile::validate() to find out if a value can be decrypted or not, in practice a check to see whether a value is already encrypted. This is what EncryptService::decrypt() and EncryptService::encrypt() checks prior to doing their thing, so we could leverage that.

As mentioned in #2931711: Tracking an elements encryption status #41:

EncryptionProfile::validate() checks if the encryption profile is valid, but it doesn't tell you if a value is already encrypted unless this is implemented on EncryptionMethodInterface::checkDependencies() by each EncryptionMethod. And we can't bet on that.

This is my understanding by reading the code.

However a potential solution can be to try to decrypt the value, and:

• if successful we set the value as encrypted with that specific encryption profile.
• if not we collect unsuccessful data and we ask the user to confirm if the value is encrypted and with which profile. I know it may be a long list, but we can help the user giving bulk updates features and guideline to bulk update from custom code.

I'm assuming 99% of the cases will be covered by first scenario.
Thoughts? Too crazy?

You are spot on as usual @gambry, thanks for doing this. At the moment I don't see a reliable way forward :(

#2931711: Tracking an elements encryption status is in, so we can proceed with this now.

I submitted few weeks ago #2943231: Support for Public-key cryptography, trying to gather feedbacks and thoughts about supporting public-key encryption. To be honest with few small changes we can improve massively the security. And if manage to get in #2943231: Support for Public-key cryptography then this issue is easily solve-able by trying decrypt() and catching the 'can't decrypt' exception.

Let's postpone on #2943231: Support for Public-key cryptography which is RTBC.

Wrong post - please ignore.

Correct post: https://www.drupal.org/project/webform_encrypt/issues/2990881#comment-12...

#2943231: Support for Public-key cryptography just landed \o/

OK just getting back to this after some time, we really need to do this :)

I'm finding it hard to figure out even what patch to continue with :s

@gambry @Rob Holmes can we summarize a bit the next steps?

So that anyone that wants to move this forward can do so :)

If we're going to depend on the work done as part of #2943231: Support for Public-key cryptography we will need to require encrypt 8.x-3.0-rc2 or later in our composer.json file.

Great news @Rob Holmes - I'll try and help with this as well next week

I haven't looked at it thoroughly - I will when tests bot is green - but just as first glance:

+++ b/src/WebformEncryptBulkEncryption.php
@@ -0,0 +1,328 @@
+  public function encryptElement($submission_data, $profile, array &$context) {
...
+    // If already encrypted then try decrypt.
+    if ($submission_profile) {
+      try {
+        $value = $this->encryptService->decrypt($unserialized_submission["data"], EncryptionProfile::load($unserialized_submission["encrypt_profile"]));
+      }
+      catch (EncryptException $e) {
...
+      }
+    }

We need to early return in here, skipping this entry, as a possible scenario could be:
- data is encrypted
- profile can't decrypt (or any other error occur)
- $value at this point is still encrypted.

We don't want to double encrypt any entry, for many reasons.

We may need to early return on decryptElement() catch block as well, even though that block is the last running for that method. This is to make sure in the future improvements of the code don't miss this piece of information and create troubles.

Updated issue summary with different scenarios. I have tested them (except scenario 4) locally without the patch in this issue. Running the same scenarios with the patch would give some clarity on what this issue would cover. Feel free to add, if I missed any scenarios.

Update: Patch in #40 is still work-in-progress and not hooked with the system yet. so I will continue the conversation of what should we do to cover the scenarios in the issue summary.

Scenario 1: No encrypt - Already working!

Scenario 2: With a new encrypt profile - functionally working (Thanks to #2931711: Tracking an elements encryption status) but the old submission with original data in DB wouldn't be encrypted when assigning the new profile.

Proposed solution(s): Since its functional, I am not sure if it is really mandatory to update original data as part of assigning a profile. However, a drush command would be nice. Say, "update my data to current settings" then the command checks for all elements with profile assigned and make sure to convert the original data.

Scenario 3: With an updated key - Not working
IMO, this is the hardest of all scenarios as there are few cases the key can be changed/removed:

1. Update key from an encryption profile (via. admin/config/system/encryption/profiles/manage/[profile_name]
2. Update the key value in the existing key (via. admin/config/system/keys/manage/[key_name]
3. Change at source (e.g. update the content of key file directly)

Even though use cases #1 and #2 warn the user editing them would cause data loss, doesn't prevent the user to do so. As @Rob Holmes mentioned in #12 there could be a valid reasons (e.g. key breached) to update the key.

Proposed solution(s):
1. For use case #1, Disable option to change encryption method/key at the profile - Suggest an alternative way to change them by creating a new profile with new key and method and switch profile. - This would align with the approach in the latest patch.
2. For use case #2, Provide an alternative approach for a key change - Create a new profile with a new key and switch profile - This would align with the approach in the latest patch. I also think some of these approaches should be pushed to upstream (encrypt and key modules) rather than try to solve here.
3. use case #3 is the tricky one and a) we could make it an exception and not provide any solution. b) Wonder if we could do a hash approach (like the core's config _core.hash) where we can keep the important data that could break the encryption/decryption as hash and try to check if any part of the profile has changed. The solution could be similar to #2931711: Tracking an elements encryption status where save the MD5 of profile data in a table. This could cause performance issues though.

Scenario 4: With an updated encrypt profile
Proposed solution(s): Solution in scenario 3 use case #1 applicable here.

Scenario 5: without an encrypt profile
Proposed solution(s): This would require bulk decryption similar to scenario 2, but it's mandatory to run it as part of field config update.

Thanks all for the work so far on this, and the detailed scenarios and testing @vijaycs85 that really helps.

Regarding the proposed scenarios, I believe we should include editing submissions that were existing prior to enabling encryption on the webform. If my memory doesn't betray me I believe some of this may be partially covered by existing tests, but I've not checked if it covers this scenario. They should:

1. Display the value correctly (I assume this is the case already).
2. When saved, get encrypted since encryption is now enabled for it.
3. When edited again display unencrypted correctly and save again correctly.

Regarding scenario 4 where we delete the encryption profile used by webform_encrypt, I don't think we can reliably provide support for this? two potential avenues here:

1. Prevent deleting of the encryption profile if a webform is configured to use it.
2. Automatically decrypt the values prior to deleting the encryption profile after warning the user about it.

A similar situation for scenarios 3 and 5. We should at least display a message warning the user when they try to edit / delete them.

The scenarios 3 4 and 5 I believe could be out of scope here, although valid and actually very worth of follow ups since they fail. Opened about this though so up to you guys.

In any case, given the complexity/time consuming nature of manually testing these scenarios, and the criticality of this due to potential data loss, I believe we should consider going TDD here, nail down the tests first so we can more confidently progress forward. It's great we have scenarios almost ready since they describe the tests themselves already.

Scenario 2: With a new encrypt profile - functionally working (Thanks to #2931711: Tracking an elements encryption status) but the old submission with original data in DB wouldn't be encrypted when assigning the new profile.

In my opinion the expected behaviour would be to encrypt existing data when you configure encryption for a webform. Similarly, users probably expect existing data to be decrypted when doing the opposite.

Thanks, @Manuel Garcia. Really appricate your time over the weekend to go through all points and providing valuable feedback.

#49:

we should include editing submissions

I believe it is covered (or indirectly implies) in some of the scenarios depending on when the edit is happening. But definitely, something to have tests.

Regarding scenario 4 where we delete the encryption profile used by webform_encrypt, I don't think we can reliably provide support for this? two potential avenues here:

pretty much. so the scenario #3 use case #1 is:

Disable option to change encryption method/key at the profile - Suggest an alternative way to change them by creating a new profile with new key and method and switch profile. - This would align with the approach in the latest patch.

The scenarios 3 4 and 5 I believe could be out of scope here, although valid and actually very worth of follow-ups since they fail. Opened about this though so up to you guys.

Ok, I am +1 either way(here or follow up). I just want to make sure we know what is there, what is going to be the scope of this issue and what is not.

I believe we should consider going TDD here, nail down the tests first so we can more confidently progress forward. It's great we have scenarios almost ready since they describe the tests themselves already.

Totally. This is my next step; providing a test-only patch to all scenarios. If we end up splitting them as subtask(s), we can move the corresponding tests.

#50: got it, updated the solution accordingly.

Uploading tests for scenario 1 and 2. Setting to needs review' to test the patch. Locally I see one failure (trying to load SID=1 after SID=2) though:

Drupal test run
---------------

Tests to be run:
- \Drupal\Tests\webform_encrypt\Kernel\WebformEncryptionSubmissionDataTest

Test run started:
Tuesday, July 2, 2019 - 23:34

Test summary
------------

\Drupal\Tests\webform_encrypt\Kernel\WebformEncryptionSubmis   0 passes   1 fails

Test run duration: 27 sec

Detailed test results
---------------------


---- \Drupal\Tests\webform_encrypt\Kernel\WebformEncryptionSubmissionDataTest ----


Status    Group      Filename          Line Function
--------------------------------------------------------------------------------
Fail      Other      phpunit-7.xml        0 \Drupal\Tests\webform_encrypt\Kerne
PHPunit Test failed to complete; Error: PHPUnit 6.5.14 by Sebastian
Bergmann and contributors.

Testing
Drupal\Tests\webform_encrypt\Kernel\WebformEncryptionSubmissionDataTest
F...                                                                4 / 4
(100%)

Time: 27 seconds, Memory: 8.00MB

There was 1 failure:

1)
Drupal\Tests\webform_encrypt\Kernel\WebformEncryptionSubmissionDataTest::testRawAndEncryptedSubmissions
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'[Value Encrypted]'
+'Test text field value'

/www/drupal8/core/tests/Drupal/KernelTests/KernelTestBase.php:1105
/www/drupal8/modules/contrib/webform_encrypt/tests/src/Kernel/WebformEncryptionSubmissionDataTest.php:100
/www/drupal8/modules/contrib/webform_encrypt/tests/src/Kernel/WebformEncryptionSubmissionDataTest.php:66

FAILURES!
Tests: 4, Assertions: 59, Failures: 1.

weird. didn't throw that error locally until I ran a different test.

Is #2990881: Create a Bulk Encryption/Decryption Service Class to facilitate encryption of pre-existing data duplicating efforts? can we join forces of both tickets?