dev/core#2258 - Add services to support encryption

[totten]

Overview

Storing specific fields as encrypted values provides a way to protect confidentiality of particularly sensitive data even if an attacker gains the ability to read the database (e.g. via SQL injection or leaked SQL backup).

In the past, CiviCRM has protected the SMTP password in this way (specifically, using the helper CRM_Utils_Crypt). However, CRM_Utils_Crypt has faced multiple issues which prevented broader/systematic usage.

This patchset provides a more robust take on CRM_Utils_Crypt which aims to allow broader/systematic usage. It is work toward https://lab.civicrm.org/dev/core/-/issues/2258

Before

define('CIVICRM_SITE_KEY', 'abcdefghijklmnopqrstuvwxyz');

$encrypted = CRM_Utils_Crypt::encrypt("my-mail-password");
$decrypted = CRM_Utils_Crypt::decrypt($encrypted);

There is a helper, CRM_Utils_Crypt, which provides encrypt and decrypt methods. However, it is problematic in a few ways:

• When the output of encrypt() is written to disk, it becomes ambiguous. A program which reads the "SMTP password" cannot tell if "YWJjZGVmZ2hp" is a plain-text password or a base64-encoded ciphertext.
• All values must be encrypted with the same key+cipher+mode. There is no way to rotate keys or to read old data.
• The encryption uses a poor set of parameters (e.g. we have not found a way to exchange data between mcrypt's Rijndael-256 and other crypto providers; e.g. ECB is not suited to recording multiple values)
• The CIVICRM_SITE_KEY has competing uses. You can get boxed-in to situations where you need to change for one reason (e.g. pro-actively rotating crypto keys) but must keep it for another reason (e.g. providing stable CiviCase IDs).

The saving grace is that this has only been used on one internal field. But the flip-side is that we have not been able to expand crypto storage to other fields.

After

The CRM_Utils_Crypt helper still exists exactly as before. This ensures that existing third-party extensions that use it will continue working as-is. But now there is another helper:

define('CIVICRM_CRED_KEYS', '::abcdefghijklmnopqrstuvwxyz ::123456789');

$encrypted = Civi::service('crypto.token')->encrypt('my-mail-password', 'CRED');
$decrypted = Civi::service('crypto.token')->decrypt($encrypted, '*');

New characteristics:

• The encryption key is a distinct setting, CIVICRM_CRED_KEYS, that can be managed separately.
• The CIVICRM_CRED_KEYS may list multiple, space-delimited keys.
  • When encrypting a new value, it uses the first key.
  • When decrypting an old value, it uses whichever key matches.
• The return value ($encrypted) is unambiguous. One can programmatically distinguish values which were:
  • Not encrypted
  • Encrypted with ::abcdefghijklmnopqrstuvwxyz
  • Encrypted with ::123456789
• One may programmatically register additional keys and ciphers.

Technical details

This patch-set actually introduces a few programmatic interfaces:

• CryptoRegistry (crypto.registry): Tracks the list of available keys and ciphers
• hook_civicrm_crypto: Runs when initializing the registry. Allows one to add more keys and ciphers.
• CryptoToken (crypto.token): Defines a data-format for the encrypted field values. Includes encrypt/decrypt methods.

This arrangement makes the system fairly extensible. For example, suppose you were developing a patch to the session-store and wanted to encrypt it with a distinct key. Part of the patch-set would say:

// Register some keys. Tag them as 'SESSION'.
function hook_civicrm_crypto($registry) {
  $registry->addSymmetricKey([
    'key' => '12345678901234567890123456789012',
    'suite' => 'aes-cbc-hs',
    'tags' => ['SESSION'],
    'weight' => -1, // Preferred for new encrypting new values
  ]);
  $registry->addSymmetricKey([
    'key' => 'abcdefghijklmnopqrstuvwxyz123456',
    'suite' => 'aes-cbc-hs',
    'tags' => ['SESSION'],
    'weight' => 10, // Available for decrypting old values
  ]);
}

// Encrypt/decrypt data using one of the 'SESSION' keys.
$encrypted = Civi::service('crypto.token')->encrypt('my-session-data, 'SESSION');
$decrypted = Civi::service('crypto.token')->decrypt($encrypted, '*');

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

@totten So this looks consistent with our discussions and looks ok to me, maybe if others @artfulrobot @pfigel have any ideas on this feedback would be appreciated

[totten]

jenkins, test this please

[mattwire]

@totten A quick comment on this re. hooks. Is there a good reason to add new "old-style" hooks instead of adding "new-style" events via symfony. I'm finding more and more that starting with symfony event listeners gives far more flexibility and, once you've got a basic example, are very easy to use.

[seamuslee001]

@mattwire for consistency sake I would presume, I'm sure that you are aware but even the hold style hooks are dispatched as events as per https://docs.civicrm.org/dev/en/latest/hooks/usage/symfony/#example-cividispatcher

[totten]

@mattwire @seamuslee001 I'm glad the new-style has gotten easier to use while increasing flexibility.

I guess my way of rationalizing the two naming conventions (hook_civicrm_foo vs civi.foo.event) is not so much old-vs-new... it's been more internal-vs-external or rich-vs-bland. This rationalization fits with the trade-offs where:

• hook_civicrm_foo (external/dual-dispatch): More people recognize it, it's more consistent with the overall corpus/tradition, and it has better documentation conventions (e.g. docblocks in CRM_Utils_Hook and a chapter in devdocs). But it's also more reductive (b/c features like priorities/removals/inspections often don't map elegantly across UFs), and if you've got edgy questions (ex: "What's the performance if the hook fires 100x per page-view?") it can be harder reason about.
• civi.foo.event (internal/single-dispatch): Fewer people recognize it, and our documentation conventions could use some work. But it is also thinner, more flexible/nuanced (allowing priorities/removal/inspections), and more internally-consistent.

For the hook added in this PR(hook_civicrm_crypto), it seemed un-demanding. It only needs to fire once in a page-view. I didn't see any special consideration to sway it toward one notation or the other. My personal approach has been to stick to hook_civicrm_foo by default (unless there's some circumstance to make civi.foo.event more appropriate).

There is a good argument to be made for phasing-out dual-dispatch (hook_civicrm_foo) in favor of a internal/singular dispatch (civi.foo.event) Maybe it's like: "Remove a layer of abstraction that's not really needed -- so that the system overall becomes simpler and more flexible." But that is potentially disruptive, and I think the recognizability/familiarity/tradition is an important counterpoint. If we want to make civi.foo.event be the default style, then that merits its own discussion-space and some community engagement?

[seamuslee001]

I'm going to merge this on the basis that it adds new functionality and doesn't break anything existing and that it has tests as well