Gracefully handle EncryptionServiceInterface Exceptions

Created on 20 January 2023, almost 2 years ago
Updated 18 August 2023, over 1 year ago

Problem/Motivation

When using real_aes as the encryption service, encrypting and decrypting our seeds may throw a EncryptionException since https://www.drupal.org/project/real_aes/issues/3157453 πŸ› Decrypt and encrypt returns false Fixed

This can cause a 500 error on the Setup and Validation plugins when an exception is thrown.

πŸ› Bug report
Status

Active

Version

2.0

Component

Code

Created by

πŸ‡§πŸ‡ͺBelgium michel.g

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Status changed to Postponed: needs info almost 2 years ago
  • Issue was unassigned.
  • Status changed to Active over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    This is indeed a legitimate fault that we have.

    An example workflow that could cause this in 1.x is TfaTotpValidation::ready()->TfaTotpValidation::getSeed()->tfaBasePlugin::Decrypt($data) which can throw \Drupal\encrypt\Exception\EncryptionMethodCanNotDecryptException or \Drupal\encrypt\Exception\EncryptException. Similar path exists in the HOTP plugin.

    Real AES is known to throw this when its key is misconfiguration in some manner, however it could be any sort of issue from any of the various encryption engines, including network faults to remote systems.

    We call ready() when a user logs in to check if they have a token configured, we have no provision in our API for raising an exception, and at the same time we also can not just return FALSE from the ready() method when an exception occurs as that could allow an access bypass. Returning true() would also pose issues.

    Yes fixing the root fault in Real AES is the correct solution to solve however that doesn't really absolve us of our obligation to handle exceptions.

    Realistically anything that calls tfaBasePlugin::Decrypt() needs to expect a failure to occur, the big question is how we present this. WSOD's and Raised Exceptions at least prevent a login from occurring, but its not the most user friendly presentation.

    Moving this to 2.x, this could involve API changes.

  • πŸ‡¨πŸ‡¦Canada ambient.impact Toronto

    As a related aside (I skimmed some of this so apologies if you already addressed this) but I noticed the code referenced in the diff in #3 has this:

      public function submitSetupForm(array $form, FormStateInterface $form_state) {
        // Write seed for user.
        try {
          $this->storeSeed($this->seed);
          return TRUE;
        }
        catch (EncryptException $e) {
        }
        return FALSE;
      }
    

    which is found in src/Plugin/Tfa/TfaHotp.php. I was trying to set up TFA on a site where I hadn't provided the key to the Encrypt module and couldn't figure out why TFA wasn't working until I dug into the code and found that catch (EncryptException $e) { is doing nothing with the exception that would have told me what the problem was (I did a dpm() call to the Devel module there to eventually figure it out). I feel like that needs to be addressed as part of this or a related issue as well.

Production build 0.71.5 2024