New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove hard dependency on mcrypt #5496

Merged
merged 11 commits into from Dec 27, 2014

Conversation

Projects
None yet
9 participants
@markstory
Member

markstory commented Dec 26, 2014

From #5440 a few key distros (redhat) will be dropping mcrypt support. We can and should switch to openssl earlier. openssl is more widely maintained and easier to use.

I've left the default to be mcrypt mostly out of backwards compatibility issues. Because our earlier implementation of encrypt() did not use PKCS#7 the data encrypted by it cannot be decrypted with openssl. For people with both extensions, it would be best if their code didn't break.

markstory added some commits Dec 23, 2014

Start splitting up encryption code.
Using a pluggable system will let us reduce our reliance on mcrypt and
remove the hard requirement on mcrypt. It will also enable us to have an
SSL adapter for more maintained library implementations.
Work more on crypto instances.
OpenSSL does not support 256bit blocksized AES, this means we cannot
provide a compatible implementation or rijndael.

Make the engine() method accept an instance so that developers can force
mcrypt/openssl as necessary.
Fix and implement cross engine decryption.
Sort out the nasty problems that were preventing Mcrypt and OpenSSL
backends from decrypting each others ciphertext.

Add a test to show how each engine can decode its own and the other's
cipher texts.
Add test cases for mcrypt/openssl.
Also include a test to show that 2.x data can still be decrypted now.
Fix failing tests.
I cocked up the tests the first time around.
Default to mcrypt.
This will be the least incompatible change for users who have both
openssl and mcrypt installed. Encrypted data from old mcrypt cannot be
decrypted with openssl as the padding scheme is incorrect (null bytes
instead of PKCS#7).

@markstory markstory added this to the 3.0.0 milestone Dec 26, 2014

Show outdated Hide outdated src/Utility/Security.php Outdated
Show outdated Hide outdated src/Utility/Crypto/OpenSsl.php Outdated
Show outdated Hide outdated src/Utility/Crypto/OpenSsl.php Outdated
@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Dec 26, 2014

Member

I've left the default to be mcrypt mostly out of backwards compatibility issues.

That's what migration guides are for :P

Member

ADmad commented Dec 26, 2014

I've left the default to be mcrypt mostly out of backwards compatibility issues.

That's what migration guides are for :P

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Dec 26, 2014

Member

I've left the default to be mcrypt mostly out of backwards compatibility issues.
That's what migration guides are for :P

@ADmad Migrating encrypted data is generally not a simple/easy process so I'd like to avoid forcing people's hand if possible. mcrypt still works fine, and if it is installed/installable on your distro then there is really no reason to switch.

Member

markstory commented Dec 26, 2014

I've left the default to be mcrypt mostly out of backwards compatibility issues.
That's what migration guides are for :P

@ADmad Migrating encrypted data is generally not a simple/easy process so I'd like to avoid forcing people's hand if possible. mcrypt still works fine, and if it is installed/installable on your distro then there is really no reason to switch.

markstory added some commits Dec 26, 2014

@josegonzalez

This comment has been minimized.

Show comment
Hide comment
@josegonzalez

josegonzalez Dec 26, 2014

Member

@markstory I think the point is that people can easily configure mcrypt if they have existing data and not need to migrate, but I can easily see that being something people don't read and then get mad about.

Member

josegonzalez commented Dec 26, 2014

@markstory I think the point is that people can easily configure mcrypt if they have existing data and not need to migrate, but I can easily see that being something people don't read and then get mad about.

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Dec 26, 2014

Member

@ADmad Migrating encrypted data is generally not a simple/easy process so I'd like to avoid forcing people's hand if possible. mcrypt still works fine, and if it is installed/installable on your distro then there is really no reason to switch.

I didn't mean migrating encrypted data. I meant stating in the migration guide that the default crypto has changed and if they are upgrading they should configure Security class to use Mycrpt instead.

Those migrating will have to carefully read about the changes anyway. Eg. for Cookies also the default encryption has changed.

Member

ADmad commented Dec 26, 2014

@ADmad Migrating encrypted data is generally not a simple/easy process so I'd like to avoid forcing people's hand if possible. mcrypt still works fine, and if it is installed/installable on your distro then there is really no reason to switch.

I didn't mean migrating encrypted data. I meant stating in the migration guide that the default crypto has changed and if they are upgrading they should configure Security class to use Mycrpt instead.

Those migrating will have to carefully read about the changes anyway. Eg. for Cookies also the default encryption has changed.

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Dec 26, 2014

Member

Ideally, there is also already a commented out bootstrap code ready to be used:

/**
 * The default crypt engine in CakePHP 3.x is Openssl
 * Comment in this code to use Mcrypt as default crypt engine (especially when upgrading a 2.x app):
 * 
 * Security::engine(new Mcrypt());
 */
Member

dereuromark commented Dec 26, 2014

Ideally, there is also already a commented out bootstrap code ready to be used:

/**
 * The default crypt engine in CakePHP 3.x is Openssl
 * Comment in this code to use Mcrypt as default crypt engine (especially when upgrading a 2.x app):
 * 
 * Security::engine(new Mcrypt());
 */
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 26, 2014

I'm +1 on making OpenSSL the default. As mentioned above, people already have to read carefully to migrate and changing it back to mcrypt is just a single line change. For new installations, it won't make a difference and making OpenSSL the default more clearly states that it is the best way going forward.

thaJeztah commented Dec 26, 2014

I'm +1 on making OpenSSL the default. As mentioned above, people already have to read carefully to migrate and changing it back to mcrypt is just a single line change. For new installations, it won't make a difference and making OpenSSL the default more clearly states that it is the best way going forward.

markstory added some commits Dec 27, 2014

Switch default crypto implementation.
Because openssl will be better in the long term, make it the default
implementation used if the openssl extension is installed. While this
will cause some short term pain, app developers can switch the app
around by including one line in their bootstrap.
@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Dec 27, 2014

Member

Ok you guys have convinced me, I've updated the default engine, and will add a commented out config option to the app skeleton.

Member

markstory commented Dec 27, 2014

Ok you guys have convinced me, I've updated the default engine, and will add a commented out config option to the app skeleton.

Show outdated Hide outdated src/Utility/Crypto/Mcrypt.php Outdated
Show outdated Hide outdated src/Utility/Crypto/OpenSsl.php Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 27, 2014

@markstory thanks! I appreciate your concern for users that "miss" that part during migration, just think this is the better way going forward.

Haven't checked, but has a "big red note" being added to the migration guide too?

thaJeztah commented Dec 27, 2014

@markstory thanks! I appreciate your concern for users that "miss" that part during migration, just think this is the better way going forward.

Haven't checked, but has a "big red note" being added to the migration guide too?

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Dec 27, 2014

Member

@thaJeztah The whole 3.0 migration guide can have a red background :trollface:

Member

ADmad commented Dec 27, 2014

@thaJeztah The whole 3.0 migration guide can have a red background :trollface:

@bravo-kernel

This comment has been minimized.

Show comment
Hide comment
@bravo-kernel

bravo-kernel Dec 27, 2014

Contributor

Open question: should the mcrypt check on the app home page be updated as well, maybe even checking openssl? If so, I can do that.

Contributor

bravo-kernel commented Dec 27, 2014

Open question: should the mcrypt check on the app home page be updated as well, maybe even checking openssl? If so, I can do that.

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Dec 27, 2014

Member

@bravo-kernel Yes it should be updated. First check for openssl and show green, if unavailable check for mcrypt and show green or red if both are unavailable.

Member

ADmad commented Dec 27, 2014

@bravo-kernel Yes it should be updated. First check for openssl and show green, if unavailable check for mcrypt and show green or red if both are unavailable.

@bravo-kernel

This comment has been minimized.

Show comment
Hide comment
@bravo-kernel

bravo-kernel Dec 27, 2014

Contributor

ok, will do

Contributor

bravo-kernel commented Dec 27, 2014

ok, will do

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Dec 27, 2014

Member

Any other feedback on this? I can work on the docs changes tonight.

Member

markstory commented Dec 27, 2014

Any other feedback on this? I can work on the docs changes tonight.

@markstory markstory self-assigned this Dec 27, 2014

@AD7six

This comment has been minimized.

Show comment
Hide comment
@AD7six

AD7six Dec 27, 2014

Member

👍

Member

AD7six commented Dec 27, 2014

👍

1 similar comment
@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark
Member

dereuromark commented Dec 27, 2014

👍

lorenzo added a commit that referenced this pull request Dec 27, 2014

Merge pull request #5496 from cakephp/no-mcrypt
Remove hard dependency on mcrypt

@lorenzo lorenzo merged commit af1d3ec into 3.0 Dec 27, 2014

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci The Travis CI build passed
Details

@lorenzo lorenzo deleted the no-mcrypt branch Dec 27, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment