feat(encryption): Add previous keys fallback feature

[patel-vansh]

Description
This PR adds a new feature to the encryption service. Currently encryption service only supports single key decryption. This feature adds a feature to enable fallback keys which can be used if data can't be decrypted by the main key.

For more detailed use cases and motivation, please take a look at this forum post.

For equivalent Laravel implementation, please look at this page.

Note:
This PR is not complete yet. I need help in testing as well as user guide update according to this new change. I've done basic logic implementation and hence I am creating this PR to let others have a look at the change and suggest some changes. Testing and User guide updation is still remaining.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

The encryption config can take in values from the .env file and having array properties can be hard to be populated. I suggest this takes a string of comma separated app keys instead.

[patel-vansh]

We can do that. So, this would be changed to comma separated string, right? So, we will have to do like, convert hex2bin or base64_decode on the fly when needed, or maybe, convert all previous keys and implode them as comma separated keys?

Which approach would be more better?

[patel-vansh]

I've currently set the second approach as for now (while initializing in the BaseConfig, do the parsing stuff and then implode as comma-separated string for future use), but we can use the other approach as well.

[patel-vansh]

I don't know why this static analysis action is failing. The $result variable is initialized to false, maybe that would be the case.
But I don't think the problem is that its initializd to false, there must be something that I'm missing.

Thanks for help.

[michalsn]

Nice start. Some thoughts: I think we should only fall back to the previous keys if the $params do not contain a key. If a key is provided in the $params, it suggests we are dealing with a custom encryption key, and the previous keys are meant to work only with the key from the config file, which would be overridden.

I would explore the option to wrap decrypt() methods content into a callback.

[michalsn]

I find this hard to read in its current form. Wrapping the logic in a callback method might make it clearer and easier to maintain, especially since the existing method logic won't change.

[patel-vansh]

I tried to make it a more readable, please check now.

[michalsn]

I was thinking of adding this method to the BaseHandler:

protected function tryDecryptWithFallback($data, #[SensitiveParameter] $params, callable $decryptCallback)
{
   try {
       return $decryptCallback($data, $params);
   } catch (EncryptionException $e) {
       if ($this->previousKeys === []) {
           throw $e;
       }

       if (is_string($params) || (is_array($params) && isset($params['key']))) {
           throw $e;
       }

       foreach ($this->previousKeys as $previousKey) {
           try {
               $previousParams = is_array($params)
                   ? array_merge($params, ['key' => $previousKey])
                   : $previousKey;

               return $decryptCallback($data, $previousParams);
           } catch (EncryptionException) {
               continue;
           }
       }

       throw $e;
   }
}

and then using it like:

public function decrypt($data, #[SensitiveParameter] $params = null)
{
    return $this->tryDecryptWithFallback($data, $params, function ($data, $params): string {
        // original method content
    });
}

From my perspective, this is clearer, as the original methods remain largely unchanged and a single shared method handles both handlers.

[patel-vansh]

Yeah, adding it in BaseHandler.php was also my first thought, but I thought that anyone that has created their own encryption handler would have to implement this method after update.

So, will it be okay to introduce a new method in base handler?

[michalsn]

Yes, as long as it's universal and can be used with any handler.

[patel-vansh]

One thing I would like to note here is, in the OpenSSLHandler.php, openssl_decrypt method returns string|false. So, only relying on catching EncryptionException to go for fallback would not be best especially for OpenSSL Handler.

[patel-vansh]

I was thinking something like this for BaseHandler.php

protected function tryDecryptWithFallback($data, #[SensitiveParameter] $params, callable $decryptCallback)
{
    $exception = null;
    
    try {
        $result = $decryptCallback($data, $params);
        
        // Handle case where decryption returns false (like OpenSSL)
        if ($result !== false) {
            return $result;
        }
    } catch (EncryptionException $e) {
        $exception = $e;
    }
    
    // Only try fallback keys if: 
    // 1. We have previous keys configured
    // 2. No custom key was provided in params
    if ($this->previousKeys === []) {
        if ($exception !== null) {
            throw $exception;
        }
        return false;
    }
    
    if (is_string($params) || (is_array($params) && isset($params['key']))) {
        if ($exception !== null) {
            throw $exception;
        }
        return false;
    }
    
    // Try each previous key
    foreach ($this->previousKeys as $previousKey) {
        try {
            $previousParams = is_array($params)
                ? array_merge($params, ['key' => $previousKey])
                : $previousKey;
            
            $result = $decryptCallback($data, $previousParams);
            
            // Return on success (not false)
            if ($result !== false) {
                return $result;
            }
        } catch (EncryptionException) {
            // Continue to next key
            continue;
        }
    }
    
    // All keys failed - throw original exception or return false
    if ($exception !== null) {
        throw $exception;
    }
    
    return false;
}

What are your thoughts on this?

[michalsn]

Good catch on openssl_decrypt() returning false. Given the existing pre-checks, this case should be extremely unlikely. I suggest updating OpenSSLHandler to throw EncryptionException::forAuthenticationFailed() when the result is false, which would allow us to simplify the BaseHandler logic.

[patel-vansh]

I've done that. Please check.

[patel-vansh]

One note:
I know that this might cause Copy/Paste Detection action to fail as both handlers essentially have same inner logic for the decrypt method.

Just committed it to get reviews on the working and logic of that function.

I think we can solve this by introducing method inside BaseHandler.php, but again, it would be kind of a breaking change so, we need to think about it.

[michalsn]

Why do we need this here?

[michalsn]

Please relocate this code to its original location.

[michalsn]

There is a problem - when you do:

$decryptParams = $params ?? $this->key;

You're always passing a key value to tryDecryptWithFallback. However, if you look at BaseHandler, there are lines:

if (is_string($params) || (is_array($params) && isset($params['key']))) {
    throw $e;
}

This check means: if an explicit key was provided in $params, don't try previousKeys.

I would try doing it this way:

return $this->tryDecryptWithFallback($data, $params, function ($data, $params): string {
    $key = $params !== null
        ? (is_array($params) && isset($params['key']) ? $params['key'] : $params)
        : $this->key;

    if (empty($key)) {
        throw EncryptionException::forNeedsStarterKey();
    }

    // derive a secret key
    $authKey = \hash_hkdf($this->digest, $key, 0, $this->authKeyInfo);

    $hmacLength = $this->rawData
        ? $this->digestSize[$this->digest]
        : $this->digestSize[$this->digest] * 2;

    $hmacKey  = self::substr($data, 0, $hmacLength);
    $data     = self::substr($data, $hmacLength);
    $hmacCalc = \hash_hmac($this->digest, $data, $authKey, $this->rawData);

    if (! hash_equals($hmacKey, $hmacCalc)) {
        throw EncryptionException::forAuthenticationFailed();
    }

    $data = $this->rawData ? $data : base64_decode($data, true);

    if ($ivSize = \openssl_cipher_iv_length($this->cipher)) {
        $iv   = self::substr($data, 0, $ivSize);
        $data = self::substr($data, $ivSize);
    } else {
        $iv = null;
    }

    // derive a secret key
    $encryptKey = \hash_hkdf($this->digest, $key, 0, $this->encryptKeyInfo);

    $result = \openssl_decrypt($data, $this->cipher, $encryptKey, OPENSSL_RAW_DATA, $iv);

    if ($result === false) {
        throw EncryptionException::forAuthenticationFailed();
    }

    return $result;
});

And a similar approach with the SodiumHandler. At this point, it would be nice to have some tests for this.

[michalsn]

After reviewing the current implementation, I think there are handler-level issues that block a reliable implementation of this feature.

I will put together a separate PR. Thanks for your work on this - and of course, you will be credited as a co-author.

[patel-vansh]

Ok. Thank you @michalsn for your help.

Looking forward to have this feature in 4.7.

[michalsn]

Resolved via #9870

[patel-vansh]

Thank you @michalsn for this feature.