Problem/Motivation

The current interfaces on Encryption module - and all the implementations I've seen so far - don't take in consideration public key cryptography.

"Public key cryptography, or asymmetrical cryptography, is any cryptographic system that uses pairs of keys: public keys which may be disseminated widely, and private keys which are known only to the owner." source Wikipedia.

Currently the module description page clearly states "Encrypt is a Drupal module that provides an application programming interface (API) for performing two-way data encryption", which presume we exclude asymmetrical cryptography, when the user want to use only the public key and so only encrypt the data.

To be honest currently implementing two-way encryption is not 100% safe, as the key to encrypt as well as decrypt is stored in the same instance. This can be mitigated by not storing the key anywhere in the current machine and making sure it's not cached anyway, as well as storing the key on 3rd party services.

I'm the maintainer of the Encrypt RSA, which allow users to use a Private key to encrypt/decrypt as well as just the public key to encrypt-only their data.

Proposed resolution

My suggestion is to:

  • introduce a flag/method in the EncryptionMethodInterface i.e. ::canDecrypt() to check if method can actually decrypt
  • Raise an exception when "encryption" service try to decrypt through a method with ::canDecrypt() FALSE.
  • Extend EncryptionMethodInterface::decrypt() docblock by adding the @throw exception annotation when the method can't decrypt
  • Adding some test coverage
  • Update the module description and documentation, flagging it supports Public-key cryptography as well and so don't assume decryption is always possible
  • Advertisting a bit this change, mainly to increase the knowledge around asymmetrical cryptography

Remaining tasks

  • introduce a flag/method in the EncryptionMethodInterface i.e. ::canDecrypt() to check if method can actually decrypt
  • Raise an exception when "encryption" service try to decrypt through a method with ::canDecrypt() FALSE.
  • Extend EncryptionMethodInterface::decrypt() docblock by adding the @throw exception annotation when the method can't decrypt
  • Adding some test coverage
  • Update the module README.md, description and documentation, flagging it supports Public-key cryptography as well and so don't assume decryption is always possible
  • Getting in touch with the main contrib modules providing Encryption Methods and for the module to be listed in Encrypt project page it needs to include a small template/widget/bullet-list (TBD) where it says: Encryption Method being used, link to the method documentation, Can decrypt? Yes/No.

User interface changes

No.

API changes

A new Encryption Method property definition can_decrypt and a new EncryptionMethodInterface::canDecrypt() method have been created. They are TRUE by default.

Data model changes

No.

CommentFileSizeAuthor
#8 2943231-8.patch13.04 KBgambry
#8 interdiff-5-8.txt4.81 KBgambry
#5 2943231-5.patch11.97 KBgambry

Comments

gambry created an issue. See original summary.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue summary: View changes
nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented

@gambry, thank you for bringing this up. I agree that we can likely strengthen the module to better support asynchronous modes. We did use Encrypt within Google Summer of Code for this module, https://www.drupal.org/project/pubkey_encrypt, which seems to have at least some similar objectives as the module you proposed. Could you please explore this? Maybe the two modules could combine.

As noted, that module is making use of encrypt. But, your suggestions may actually improve both modules.

Some specifics ---

1. "introduce a flag/method to check if it can decrypt" -> this sounds like an annotation, as it's likely static

2. and 3. and 4. agreed, this should be tied to the annotation

5. yes this would be helpful and is already happening. so it's not accurate as it is now

6. suggestions?

In addition, I'd like to see us work on the usability of the module. Users should be very keenly aware that an encryption method uses one-way encryption and their info cannot be decrypted. I would love if someone could review the UI and make a suggestion.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented

@nerdstein thanks for your reply. My main concern was if my request was totally crazy. It doesn't look like, so thank you again for that.

We did use Encrypt within Google Summer of Code for this module, https://www.drupal.org/project/pubkey_encrypt, which seems to have at least some similar objectives as the module you proposed.

I had a look at pubkey_encrypt during my investigation, and this was my understanding:
- The module does much more than providing an encryption method, i.e. "the module generates an Encryption Profile for each role".
- The module depends on https://www.drupal.org/project/encrypt_seclib , which doesn't have any release.
- The module uses phpseclib, which although being a wonderful library, it's hard to use in the right way (I've been playing around with it a lot and I do prefer its wrapper https://github.com/paragonie/EasyRSA )

That's why I built Encryption RSA.

But let me know by any meaning how we can contribute together, I would rather have one module doing something perfectly than two or more doing roughly the same thing.

Advertisting a bit this change, mainly to increase the knowledge around asymmetrical cryptography

suggestions?

Yes, and I'd like to have your feedbacks:

  • We can start by updating the Documentation of the module, as well as the description in the module's page
  • Speaking at events about the state of the Encryption stack in Drupal, and all the tools. I'm planning to submit a session ad Drupal Dev Days about this, and I would love to co-present this with you guys! Give me a shout.
  • Encrypt module has done a wonderful job with listing some/all of other contrib make use of it. I suggest we get in touch with them and for a module to be listed it needs to include a small template/widget/bullet-list (TBD) where it says: Encryption Method being used, link to the method documentation, Can decrypt? Yes/No. No widget, no party. :D

Will start working on a patch for this issue soon.

Thanks again and many thanks to @Manuel Garcia for intermediating! :D

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2943231-5.patch11.97 KB

This is first draft of the work. I thought it would have been more disruptive, but it's not at all and it could nicely be merged in current version.

The work introduces a new Encryption Method can_encrypt (annotation) definition. If this is FALSE the encryption service throws a Drupal\encrypt\Exception\EncryptionMethodCanNotDecryptException exception if decrypt() is called for an encryption method where can_decrypt is set to FALSE (or anyway its canDecrypt() method return FALSE).

Existing encryption method plugins don't need alteration, they will inherit the default can_decrypt annotation value which is TRUE.

Because decryption can be invoked also by calling directly an Encryption Profile's method $encryption_profile->getEncryptionMethod->decrypt(), Encryption Methods who can NOT decrypt should also throw the same exception on they decrypt() method.

The patch also update the readme.md file and add some test coverage.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue summary: View changes

Fixing couple of typos and adding reference to external key management tools.

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work

Patch is looking really good, some minor nitpicks:

  1. +++ b/readme.md
@@ -33,6 +33,12 @@ Read the README.txt document provided by the Real AES module for detailed
+Encrypt RSA module (https://www.drupal.org/project/encrypt_rsa) provides ¶

@@ -74,6 +80,10 @@ $encryption_profile = \Drupal::service('entity.manager')
+asymmetrical encryption methods may be able to encrypt BUT NOT decrypt your ¶
+data! [Read more about symmetric and asymmetric cryptography.](https://en.wikipedia.org/wiki/Cryptographic#Modern_cryptography) ¶

@@ -86,3 +96,9 @@ will automatically be shown upon creation of an EncryptionProfile entity.
+your "decrypt()" method implementation should just throw a ¶
+"EncryptionMethodCanNotDecryptException" exception. See the ¶

    Trailing spaces. 

  2. +++ b/tests/modules/encrypt_test/src/Plugin/EncryptionMethod/AsymmetricalEncryptionMethod.php
@@ -0,0 +1,44 @@
+    return '###decrypted###';

    This should return '###encrypted###' instead. Test should be updated accordingly. 

  3. +++ b/tests/modules/encrypt_test/src/Plugin/EncryptionMethod/AsymmetricalEncryptionMethod.php
@@ -0,0 +1,44 @@
+    throw new EncryptionMethodCanNotDecryptException();

    Lets return something other than the exception here (say for example 'data encrpyted'), so that the test actually verifies that it is the EncriptService the one throwing the exception.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-5-8.txt4.81 KB
new 2943231-8.patch13.04 KB

Thanks for the review!

I fixed all the feedback.
For point 3 I opted for throwing a different exception rather then just returning dummy data. This because in a normal scenario that method should throw EncryptionMethodCanNotDecryptException exception and I wanted to make it clear if developers use it as a blueprint for their implementation. I left a - hopefully - explanatory comment.
But honestly it doesn't matter what that returns, so let me know if you want me to return dummy data.

Also although this issue add a new method to the interface I think we can live with the API break because:

  1. The module is in alpha, so developers and site-builder should expect API changes
  2. All contrib modules currently exposing encryption methods extend EncryptionMethodBase, which implement the new method with a default value not changing the expected behaviour
  3. because the current version is alpha, I'm worried creating a new major release only due this change will leave us with 2 alpha releases to maintain... and one of them will never see the stable goal which is really confusing for the users.
manuel garcia’s picture

manuel garcia commented
Status: Needs review » Reviewed & tested by the community

Thanks @gambry - changes look good, and using the test encryption method as an example is a good idea.

I agree about the API break by adding a new method to the interface (we discussed this during DrupalDevDays encrypt sprint).

Ive got nothing else to complain about so RTBCing

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue tags: +DevDaysLisbon
cellar door’s picture

cellar door commented

A few thoughts:

1. This is a great set of work that provides some very interesting possibilities for what's possible with encrypt.

2. I checked and the 3 listed encryption methods that we know of using Encrypt in D8 (Real AES, Sodium, and Encrypt KMS) all are safe from this break in the API so it's a soft break in my opinion as most everyone using the module should be using one of those (rolling your own crypto isn't really recommended).

3. Though in Alpha, and people know the risks, there really isn't any reason this module shouldn't get a full release. It's stable, it's been used in production on a number of sites without any concerns.

So I'm going to put in my vote to include this in Alpha, cut a beta and set a timeline to get a full release out asap barring anything breaking with this implementation. I'd say that if we can roll this out to beta, and if by Drupal Europe there's no major issues that have been brought up we move to a full release.

In the meantime we'll want to write some pretty serious release notes for anyone implementing this that if they aren't using any of the modules known to support Encrypt and are compatible with this they check for compatibility first.

manuel garcia’s picture

manuel garcia commented

Thanks @Cellar Door for doing due diligence on the API break, the approach towards release sounds reasonable to me.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue tags: +Contributed project blocker

Pinging this. I'm going to start the work for Encrypt RSA for v2, in order to drop support for PHP < 7.2 and make use of libsodium methods directly.

It would be good to also lock the dependency to this module to a version where Asymmetrical Encryption is officially supported.
Also I believe this is a blocker for some issues on Webform Encrypt too.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented

Confirming that patch in #8 works.
It would be nice to have this committed.

manuel garcia’s picture

manuel garcia commented
Related issues: +#2925621: Encryption of pre-existing data

Adding related issue on webform_encrypt which is postponed on this.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented

Support for public-key encryption is a tremendous addition to this module. Thanks for proposing it.

  • rlhawk committed 0b8b87a on 8.x-3.x authored by gambry 
    Issue #2943231 by gambry, Manuel Garcia: Support for Public-key...
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Reviewed & tested by the community » Fixed
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Issue summary: View changes
gambry’s picture

gambry
Primary language Italian
Location Milan
commented

Amazing! Thanks!!!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.