Add support for an encryption provider in an extension

[eileenmcnaughton]

Overview

This is a proposal for https://lab.civicrm.org/dev/core/issues/236 - it doesn't seek to answer the question of how to provide encryption - only how to move that decision out of core.

I threw together https://github.com/eileenmcnaughton/org.civicrm.mcrypt and ran through the functions in a unit test.

Note that we ONLY ACTUALLY ENCRYPT ONE FIELD in core! That field is smtp_provider & if an imap account is set up it won't be encrypted there - so this feels kinda optional. I am guessing from @adixon interest there might be some usage outside of core & hence this has been written to provide no breakage & with the expecation that we would later deprecate & remove the lines that still handled mcrypt for people without the extension. We'd also need to consider shipping the extension until such time as we have a preferred extension for new builds.

This also punts any upgrade / migration related issues.

Before

Only Mcrypt possible

After

Extensions could add other providers. Only mcrypt exists as an extension provided encrypter :-)

Technical Details

The approach is simply to add a setting - which can be set by an extension and an interface.

Not sure if any inputs / outputs other than string make sense

Comments

I'm not actually vested in this - it just seemed like 40 minutes spent demonstrating in code would be easier than 40 mins spent explaining in a comment https://lab.civicrm.org/dev/core/issues/236 & it seems merging this would unblock stuff.

This looks like a good plugin https://github.com/defuse/php-encryption/blob/master/docs/Tutorial.md

[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]

@eileenmcnaughton i think sparkpost might also use the crypt function i think

[eileenmcnaughton]

@seamuslee001 yeah - well this won't change anything for existing users initially

[davejenx]

com.cividesk.email.sparkpost uses it here:
https://github.com/cividesk/com.cividesk.email.sparkpost/search?q=crypt&unscoped_q=crypt

[eileenmcnaughton]

OK I think we made a mistake adding a check to core on this TBH. still it doesn't matter in the first stage of this approach

[eileenmcnaughton]

Just thinking further about this I think we should

1. Remove the Check from core
2. Alter the one place where core sometimes encrypts a password to give a message if it will not be encrypted (note that not all smtp site users even use a password)
3. Sparkpost should perhaps take a copy of the current implementation of encryption & the check into the extension & not depend on core for encryption as core is not currently able to provide that service in a reliable cross-php-version way & it makes sense to move the service out of core as it is only trivially used by core (@nganivet )

I think this & the associated extension are still part of the longer term solution

@totten

[seamuslee001]

@eileenmcnaughton if we were to do that would we not then need to have an upgrade script that de-encrpyts on the upgrade?

[eileenmcnaughton]

@seamuslee001 so I'm proposing a deprecation path - ie

1. we merge this which will actually change nothing by itself
2. we start shipping the linked extension
3. we enable the linked extension on existing installs
4. we don't enable on new installs & possibly after some time stop shipping it & people who want it can get it
5. extensions can add it as a dependency if they want it - or implement their own variant
6. after some time we remove all mcrypt handling from core

[seamuslee001]

I'm more looking at points 1 and 2 in your previous comment, if we are removing the check / signifiying it won't be encrypted then won't we need to decrpyt the currently encrypted passwords?

[eileenmcnaughton]

@seamuslee001 sorry - by 'check' I mean the system check. Currently we are telling people they need mcrypt when in fact a very small proportion of sites need it. We should only tell those who need it that they need it - ie. the extension can implement the check & core can implement it only on one page

[nganivet]

@eileenmcnaughton Thanks for the ping.

All - not quite understanding the reasoning behind creating this extension. Why don't we simply replace mcrypt with openssl encryption in CRM_Utils_Crypt, and prefix any SSL-encrypted password in DB with SSL- or another marker?

This way on decryption we know which method to decrypt with, and if we decrypt an mcrypt-encrypted password we can re-encrypt it in the database with openssl. After a few releases, remove all mcrypt code from core and the migration has been done completely transparently - even for extensions.

[eileenmcnaughton]

@nganivet I think it makes more sense to make it possible to make encryption code swappable than to try to pick the winner. It's a movable feast IMHO - I don't think core should necessarily choose which one to use. There is an issue when people change from one to another - but I think that can be managed in disable / enable routines & messages (it's not a frequent process or one that affects many sites )

[nganivet]

@eileenmcnaughton Then let's make a hook available, and then extension authors can implement whatever they need but core has at least a default way to encrypt passwords. By pre-fixing encrypted passwords with encryption method we resolve all migration issues transparently. It seems like a huge step backwards to tell people that they have to download an extension to make CiviCRM store passwords encrypted, and a huge painto tell people that use SparkPost that there is now an additional dependency.

[eileenmcnaughton]

@nganivet it would also be trivial for sparkpost to simply copy the existing encryption file into sparkpost & use that - the encryption class is not part of the api so it's not supported for extensions per se

[nganivet]

@eileenmcnaughton Are you suggesting that extensions should not use any CRM class? This is ridiculous. Why duplicate in every extension services that are already provided in core? The plan I am proposing is KISS in that it does change any existing core classes, provides a migration path transparent for the end user and allows extensions to modify encryption method if desired. So why not adopt it?

[eileenmcnaughton]

Well it's not every extension. As far as we know it's one. And core barely uses encryption. We could prefix by ecncryption class though - that makes sense

[eileenmcnaughton]

From a core pov removing the system check & only putting an message on the one place encryption is needed solves our 7.2 problem

[nganivet]

Thanks. I really think providing encryption for passwords that are stored in the database makes sense to have in core. We all do backups and ship these off-site, having passwords in clear text in these backups is definitely a security risk. We should on the opposite try to encrypt more passwords rather than remove encryption service from core. I am very surprised IMAP passwords are not encrypted. And what about payment processor keys? Are these not encrypted?

[eileenmcnaughton]

nope they aren't!

[nganivet]

Whooppsy, that might be a biggy! I'll report this to the security team so it can be discussed in a wider circle.

[mlutfy]

Can you wrap in ts ?

[eileenmcnaughton]

done

[nganivet]

@mlutfy Per @eileenmcnaughton 's comments above I thought there was an agreement to not merge this and instead proceed with prefixing the encrypted information with encryption method and transparently re-encrypting with new method on decryption. This would ensure a seamless migration, including for future changes, without adding a requirement on installing an extension just to securely store passwords.

[eileenmcnaughton]

Actually I didn't agree per se. I concluded that adding prefixing in might make sense but still think it should be done in an extension. I think this & the associated extension are the right approach but I had decided to close this in favour of "Removing the system check & only putting an message on the one place encryption is needed solves our 7.2 problem" or possibly retaining the system check but only displaying it if smtp passwords exist

[nganivet]

@eileenmcnaughton @mlutfy Them I am sorry but do not think this can work as is in this PR since:

• an end-user adds an encryption extension, all the passwords stop working (since the decryption does not recognize the encryption was not done with the extension)
• in a later release the PHP mcrypt functions are removed, all the passwords stop working again (since the passwords are double-encrypted and decrypted: once with the extension, then with the mcrypt functions
• an end-user wants to change encryption method, this does not work since only one encryption plugin is possible and there is no migration path

It would be so simple to implement prefixing and auto-migration in core, and worry about pluggable encryption later on ... if ever needed at all! Why not do KISS on this one?

[eileenmcnaughton]

ok - I'm closing this in favour of refining the core messaging.

[mlutfy]

Nicolas raises valid concerns, but I don't think they should be blockers. Furthermore, we can address all those concerns in future PRs. We need to phase out mcrypt, and this seems like a good first step. For people on old versions of PHP (less than 7.2), the PR is harmless.

For people on 7.2 (disclosure: including me), they would be able to start using php-sodium. People on PHP 7.2 currently roll out CiviCRM without any encryption mechanism. We will increasingly get support questions and they will have to manually fix their passwords.

In any case, I'll roll-out to our production servers and I'll try to publish a php-sodium implementation if @mfb does not beat me to it. ;)

[eileenmcnaughton]

@mlutfy OK I've re-opened this.

The main reason I closed it was that I didn't want to put more time into it but if you are happy to review then it's here for review

[eileenmcnaughton]

I've added a separate PR to fix the warning #12733

[nganivet]

@mlutfy @eileenmcnaughton I oppose this change for the following reasons:

• we cannot reasonably ask our users to download an extension in order to safely store passwords within CiviCRM. This should be a basic core feature, period.
• it seems silly to add dependencies not only to the SparkPost extension, but also to all payment processor extensions (those that currently do not should be asked to leverage this core feature)

I suggest we instead use CRM-21085 (#10879), for which @seamuslee001 has already done significant work, with the caveat that we prefix the encrypted value with an algorithm indicator in order to facilitate further migrations.

I will volunteer to add this prefixing to Seamus's PR if no-one wants to spend more time on this one.

[totten]

Recapping the main reason we're here:

• The implementation of CRM_Utils_Crypt depended on mcrypt, which is going away.
• CRM-21085 Replace mcrypt functions where possible with openssl versio… #10879 provided a more portable implementation (without mcrypt) -- except that it's not a drop-in replacement because it changes the crypto settings.
• We made a non-trivial effort to reproduce the crypto settings and failed. There's something weird about them. Fixing the mcrypt dependency problem basically requires that we change the crypto settings.

At the same time, there are some legit critiques of the existing framework:

• (Critique-1) It doesn't let you change the crypto algorithm. That's not a huge problem for most folks (esp if we use the portable phpseclib), but maybe we lose out on some more edge-casey things (e.g. integration with some advanced hardware security module; e.g. integration with some crypto framework in a CMS ecosystem).
• (Critique-2) There's generally no clear way to re-key ciphertext. We have a special case of this right now, but more generally security folks advise that you periodically rotate keys. Solving this likely requires some mix of code and/or docs.
• (Critique-3) All encrypted data must be encrypted exactly the same way -- e.g. if you wanted to encrypt both the SMTP password and the user-sessions/form-state, then you'd have to encrypt exactly the same way. You're not able to use different keys or ciphers.

The current PR tackles Critique-1 (swappable crypto providers). This allows one to safely change the crypto settings for new sites or for admins who can work through a window of ambiguity/breakage. This is good at letting downstream decide between AES-128-ECB and AES-256-CBC, but it leaves the migration/rekey problem open... so we still haven't closed the ticket. I imagine this is intended as partial-solution/first-step.

OTOH, if you tackled Critique-2 (rekey workflow), then it could solve the problem for old+new sites (with or without swappable crypto providers). That seems more like the "critical path" to me.

But, even if swappable crypto isn't strictly "critical path", it's still addressing a valid critique and does allow some users to get past mcrypt, so I can't really argue that it's a bad idea to address that.

Aside:

• Like Nicolas, I'm inclined more toward a hook/event approach (example pseudocode) than a setting/class approach.
• If the idea is to make the smallest injection point in core -- and let some extension iteratively work out the real contract -- then I'd lean toward making no pretense about a long-term supported crypto-contract in core. Rather, do something manifestly hacky (like the flexmailer constants) so that third-parties get a clear signal to direct the energy into that extension. It's silly to add a setting and then require admins to toggle-through it.
• But I wouldn't draw a hard line in the sand over these asides.

[eileenmcnaughton]

test this please

[nganivet]

@totten Honestly the only reason we are here is to support PHP 7.2, which has obsoleted mcrypt.

Then, rather than look at it from a practical / end-user focused perspective and finding a 'simple and elegant' solution to this, the engineers that we all are looked around and found new toys: hey, let's use that shinny new library, but let's also make possible that our engineer friends also have their own library if they wanted to, or even a crypto module (eyes wide open ... cool!!!!!), so let's package this with extensions (double rainbow ...) and create hooks ('cause that will be so much fun!).

The reality is that our end users don't care a single bit with all these engineering marvels, they just need to have their passwords store securely in the database, with minimal fuzz (we hate when security gets in the way).

So please let's stop building hammers to squat flies, or in that case storage for a single SMTP password, and build something 'simple and elegant' that:

• does not require users to re-enter their passwords
• does not require anything additional for CiviCRM to be secure
• can withstand a future change of crypto in core

The outlined solution fails on all above points, which again are the only things our users care about.

PS. Sorry for the long and hard rant, no offense intended to anyone, sometimes we just have to stop being engineers and think about our end-user's needs.

[eileenmcnaughton]

From my point of view the 7.2 issue was resolved by making the language more appropriate in the system check - which has now been merged. #12733

[davejenx]

My preference, if feasible, is to have some worthwhile form of encryption provided by core and to be encrypting more core things rather than less: I agree it's dodgy to be storing payment processor passwords in plain text, for example.

[mlutfy]

Closing the PR, because of one hard -1 (Nicolas), and one soft -1 (Tim). I'm also OK with phasing out encryption, because it's too much effort for little benefit. I agree it's dodgy, but we are scope creeping what is a PHP 7.2 compatibility issue. Currently, only one password is actually encrypted, and there's an easy workaround (storing the setting in civicrm.settings.php).