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

[5.1] Remove mcrypt dependency #9041

Merged
merged 4 commits into from Jun 3, 2015

Conversation

Projects
None yet
5 participants
@ibrasho
Contributor

ibrasho commented Jun 3, 2015

To be honest, it's quite annoying to keep BC.

Can we implement a different encrypter for openssl, and keep the old one for BC?

Something like: If you have used the old encryptor, set 'app.encrypter' to 'mcrypt' to keep it working?

Also, can we remove or deprecate the setMode() method?

@ibrasho ibrasho changed the title from Remove mcrypt dependency to [5.1] Remove mcrypt dependency Jun 3, 2015

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 3, 2015

Member

Also, can we remove or deprecate the setMode() method?

👍 for deprecation in 5.1 and removal in 5.2.

Member

GrahamCampbell commented Jun 3, 2015

Also, can we remove or deprecate the setMode() method?

👍 for deprecation in 5.1 and removal in 5.2.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 3, 2015

Member

Can we implement a different encrypter for openssl, and keep the old one for BC?

👎 for that. A clean switch sounds much better.

Member

GrahamCampbell commented Jun 3, 2015

Can we implement a different encrypter for openssl, and keep the old one for BC?

👎 for that. A clean switch sounds much better.

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

We will need to update the value of 'app.cipher' in config/app.php.

Should we add a handling case of MCRYPT_RIJNDAEL_128?

Contributor

ibrasho commented Jun 3, 2015

We will need to update the value of 'app.cipher' in config/app.php.

Should we add a handling case of MCRYPT_RIJNDAEL_128?

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 3, 2015

Member

Maybe we should do this change on 5.2 instead of 5.1 @taylorotwell?

Member

GrahamCampbell commented Jun 3, 2015

Maybe we should do this change on 5.2 instead of 5.1 @taylorotwell?

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 3, 2015

Member

on 5.2 instead of 5.1

That said, I'd quite like to see this in 5.1.

Member

GrahamCampbell commented Jun 3, 2015

on 5.2 instead of 5.1

That said, I'd quite like to see this in 5.1.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 3, 2015

Member

Actually, could we just remove setMode in 5.1. It's only a minor breakage. Most people won't even notice it's gone.

Member

GrahamCampbell commented Jun 3, 2015

Actually, could we just remove setMode in 5.1. It's only a minor breakage. Most people won't even notice it's gone.

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

illuminate/encryption's composer.json surprisingly doesn't have ext-mcrypt as a requirement.

Contributor

ibrasho commented Jun 3, 2015

illuminate/encryption's composer.json surprisingly doesn't have ext-mcrypt as a requirement.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 3, 2015

Member

I would totally remove the setMode method.

Member

taylorotwell commented Jun 3, 2015

I would totally remove the setMode method.

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

It's in the contract, should I remove it from both? 😞

Contributor

ibrasho commented Jun 3, 2015

It's in the contract, should I remove it from both? 😞

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell
Member

taylorotwell commented Jun 3, 2015

Yes.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 3, 2015

Member

It's in the contract, should I remove it from both?

Yes, in both please. :)

Member

GrahamCampbell commented Jun 3, 2015

It's in the contract, should I remove it from both?

Yes, in both please. :)

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

Done.

Contributor

ibrasho commented Jun 3, 2015

Done.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 3, 2015

Member

It doesn't appear to decrypt data encrypted with previous installation. For example, use previous encryption implementation, use cookie session driver and put something in session, switch to new encryption implementation and you will receive an error when it tries to decrypt the cookie.

EDIT: NEVERMIND, user error.

Member

taylorotwell commented Jun 3, 2015

It doesn't appear to decrypt data encrypted with previous installation. For example, use previous encryption implementation, use cookie session driver and put something in session, switch to new encryption implementation and you will receive an error when it tries to decrypt the cookie.

EDIT: NEVERMIND, user error.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 3, 2015

Member

Should we add a handling case of MCRYPT_RIJNDAEL_128?

I think we shouldn't given the idea of this is to remove the dependence on mcrypt.

Member

GrahamCampbell commented Jun 3, 2015

Should we add a handling case of MCRYPT_RIJNDAEL_128?

I think we shouldn't given the idea of this is to remove the dependence on mcrypt.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 3, 2015

Member

I don't even think we should allow the cipher to be configurable at all honestly.

Member

taylorotwell commented Jun 3, 2015

I don't even think we should allow the cipher to be configurable at all honestly.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 3, 2015

Member

I don't even think we should allow the cipher to be configurable at all honestly.

👍 for removing the config. Maybe we should still allow it to be set in the class constructor if someone really wants to configure it?

Member

GrahamCampbell commented Jun 3, 2015

I don't even think we should allow the cipher to be configurable at all honestly.

👍 for removing the config. Maybe we should still allow it to be set in the class constructor if someone really wants to configure it?

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

👍 for removing the config. Maybe we should still allow it to be set in the class constructor if someone really wants to configure it?

I support this idea. 👍

Contributor

ibrasho commented Jun 3, 2015

👍 for removing the config. Maybe we should still allow it to be set in the class constructor if someone really wants to configure it?

I support this idea. 👍

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 3, 2015

Member

K, let's do that. We have a good secure default and most people probably shouldn't be messing with it.

Member

taylorotwell commented Jun 3, 2015

K, let's do that. We have a good secure default and most people probably shouldn't be messing with it.

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

This means setCipher() will be removed from both the class and contract and the tests should be removed too, right?

Contributor

ibrasho commented Jun 3, 2015

This means setCipher() will be removed from both the class and contract and the tests should be removed too, right?

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 3, 2015

Member

Yes, I think so.

Member

taylorotwell commented Jun 3, 2015

Yes, I think so.

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

Done.

Contributor

ibrasho commented Jun 3, 2015

Done.

@taylorotwell taylorotwell merged commit cb7d360 into laravel:5.1 Jun 3, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 3, 2015

Member

Thanks! ❤️

Member

taylorotwell commented Jun 3, 2015

Thanks! ❤️

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

Is it me, or the tests are failing?

Contributor

ibrasho commented Jun 3, 2015

Is it me, or the tests are failing?

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 3, 2015

Member

They are passing for me?

On Wed, Jun 3, 2015 at 3:13 PM, Ibrahim AshShohail <notifications@github.com

wrote:

Is it me, or the tests are failing?


Reply to this email directly or view it on GitHub
#9041 (comment).

Member

taylorotwell commented Jun 3, 2015

They are passing for me?

On Wed, Jun 3, 2015 at 3:13 PM, Ibrahim AshShohail <notifications@github.com

wrote:

Is it me, or the tests are failing?


Reply to this email directly or view it on GitHub
#9041 (comment).

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

https://travis-ci.org/laravel/framework/builds/65305403

I think it's because you merged before they finished?

Contributor

ibrasho commented Jun 3, 2015

https://travis-ci.org/laravel/framework/builds/65305403

I think it's because you merged before they finished?

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 3, 2015

Member

Yeh, they're passing for me too.

Member

GrahamCampbell commented Jun 3, 2015

Yeh, they're passing for me too.

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 3, 2015

Contributor

Can someone test updating a current app with this and seeing if it works?

On a fresh installation, switching from mcrypt works fine.
But I just tested another app and I'm not able to decrypt the session..

I can't tell why exactly, but I'm trying to figure it out.

Taylor, is this similar to what you faced?

Contributor

ibrasho commented Jun 3, 2015

Can someone test updating a current app with this and seeing if it works?

On a fresh installation, switching from mcrypt works fine.
But I just tested another app and I'm not able to decrypt the session..

I can't tell why exactly, but I'm trying to figure it out.

Taylor, is this similar to what you faced?

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 3, 2015

Member

Yes, but I fixed it by updating my cipher config variable (before it was
non-configurable)... I'll try it again.

On Wed, Jun 3, 2015 at 3:27 PM, Ibrahim AshShohail <notifications@github.com

wrote:

Can someone test updating a current app with this and seeing if it works?

On a fresh installation, switching from mcrypt works fine.
But I just tested another app and I'm not able to decrypt the session..

I can't tell why exactly, but I'm trying to figure it out.

Taylor, is this similar to what you faced?


Reply to this email directly or view it on GitHub
#9041 (comment).

Member

taylorotwell commented Jun 3, 2015

Yes, but I fixed it by updating my cipher config variable (before it was
non-configurable)... I'll try it again.

On Wed, Jun 3, 2015 at 3:27 PM, Ibrahim AshShohail <notifications@github.com

wrote:

Can someone test updating a current app with this and seeing if it works?

On a fresh installation, switching from mcrypt works fine.
But I just tested another app and I'm not able to decrypt the session..

I can't tell why exactly, but I'm trying to figure it out.

Taylor, is this similar to what you faced?


Reply to this email directly or view it on GitHub
#9041 (comment).

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 3, 2015

Member

It still works for me. I was able to move seamlessly from 5.0 to 5.1's
encrypter.

On Wed, Jun 3, 2015 at 3:27 PM, Taylor Otwell taylorotwell@gmail.com
wrote:

Yes, but I fixed it by updating my cipher config variable (before it was
non-configurable)... I'll try it again.

On Wed, Jun 3, 2015 at 3:27 PM, Ibrahim AshShohail <
notifications@github.com> wrote:

Can someone test updating a current app with this and seeing if it works?

On a fresh installation, switching from mcrypt works fine.
But I just tested another app and I'm not able to decrypt the session..

I can't tell why exactly, but I'm trying to figure it out.

Taylor, is this similar to what you faced?


Reply to this email directly or view it on GitHub
#9041 (comment).

Member

taylorotwell commented Jun 3, 2015

It still works for me. I was able to move seamlessly from 5.0 to 5.1's
encrypter.

On Wed, Jun 3, 2015 at 3:27 PM, Taylor Otwell taylorotwell@gmail.com
wrote:

Yes, but I fixed it by updating my cipher config variable (before it was
non-configurable)... I'll try it again.

On Wed, Jun 3, 2015 at 3:27 PM, Ibrahim AshShohail <
notifications@github.com> wrote:

Can someone test updating a current app with this and seeing if it works?

On a fresh installation, switching from mcrypt works fine.
But I just tested another app and I'm not able to decrypt the session..

I can't tell why exactly, but I'm trying to figure it out.

Taylor, is this similar to what you faced?


Reply to this email directly or view it on GitHub
#9041 (comment).

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Jun 4, 2015

Contributor

And for people still using MCRYPT_RIJNDAEL_256 (upgrading from L4), they need to re-encrypt all their files/encrypted strings? Or extend the ServiceProvider themselves?
Should probably at least be documented in the upgrade guide and provide a work-around, right?

Contributor

barryvdh commented Jun 4, 2015

And for people still using MCRYPT_RIJNDAEL_256 (upgrading from L4), they need to re-encrypt all their files/encrypted strings? Or extend the ServiceProvider themselves?
Should probably at least be documented in the upgrade guide and provide a work-around, right?

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 4, 2015

Contributor

@barryvdh : I agree. This need to be added to the upgrade guide.

Something like the following in AppServiceProvider should be enough.

    public function register()
    {
        $this->app->singelton('encrypter', function ($app) {
            return new Encrypter($config->get('app.key'), $config->get('app.cipher', 'AES-256-CBC'));
        });
    }
Contributor

ibrasho commented Jun 4, 2015

@barryvdh : I agree. This need to be added to the upgrade guide.

Something like the following in AppServiceProvider should be enough.

    public function register()
    {
        $this->app->singelton('encrypter', function ($app) {
            return new Encrypter($config->get('app.key'), $config->get('app.cipher', 'AES-256-CBC'));
        });
    }
@crynobone

This comment has been minimized.

Show comment
Hide comment
@crynobone

crynobone Jun 4, 2015

Contributor

@taylorotwell is there any reason we don't include the 2nd param by default based on @ibrasho example because this is also a problem with one of my production code (which was upgraded since 4.0).

Contributor

crynobone commented Jun 4, 2015

@taylorotwell is there any reason we don't include the 2nd param by default based on @ibrasho example because this is also a problem with one of my production code (which was upgraded since 4.0).

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Jun 4, 2015

Contributor

Or just check the 'old' default version and convert that

if ($config->get('app.cipher') === MCRYPT_RIJNDAEL_256) {
    $cipher = 'AES-256-CBC';
} else {
    $cipher = 'AES-128-CBC';
}
Contributor

barryvdh commented Jun 4, 2015

Or just check the 'old' default version and convert that

if ($config->get('app.cipher') === MCRYPT_RIJNDAEL_256) {
    $cipher = 'AES-256-CBC';
} else {
    $cipher = 'AES-128-CBC';
}
@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 4, 2015

Member

Because we no longer want to make changing the default easy. Though, this simple ioc binding isn't that hard if you really want it.

Member

GrahamCampbell commented Jun 4, 2015

Because we no longer want to make changing the default easy. Though, this simple ioc binding isn't that hard if you really want it.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 4, 2015

Member

if ($config->get('app.cipher') === MCRYPT_RIJNDAEL_256) {

That line will break if you don't have mcrypt installed.

Member

GrahamCampbell commented Jun 4, 2015

if ($config->get('app.cipher') === MCRYPT_RIJNDAEL_256) {

That line will break if you don't have mcrypt installed.

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Jun 4, 2015

Contributor

You can just remove the config also. And instead of the definition, 'rijndael-256' is also an option.

But extending the SP is also an option, but it should be documented properly.

Contributor

barryvdh commented Jun 4, 2015

You can just remove the config also. And instead of the definition, 'rijndael-256' is also an option.

But extending the SP is also an option, but it should be documented properly.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 4, 2015

Member

But extending the SP is also an option, but it should be documented properly.

We're open to PRs for that. :)

Member

GrahamCampbell commented Jun 4, 2015

But extending the SP is also an option, but it should be documented properly.

We're open to PRs for that. :)

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 4, 2015

Contributor

I'm working on cleaning up the encryption functions.

OpenSSL does the padding and base64 encoding by itself. I just need to make sure it's working seamlessly.

Contributor

ibrasho commented Jun 4, 2015

I'm working on cleaning up the encryption functions.

OpenSSL does the padding and base64 encoding by itself. I just need to make sure it's working seamlessly.

@crynobone

This comment has been minimized.

Show comment
Hide comment
@crynobone

crynobone Jun 4, 2015

Contributor

Or just check the 'old' default version and convert that

Having Illuminate\Encryption\EncryptionServiceProvider provide second parameter with the value of app.cipher should be enough since it already available in the class. For new installation app.cipher is not available and hence it use the default. But for those upgrading from earlier version they can just update the config with the new value.

Contributor

crynobone commented Jun 4, 2015

Or just check the 'old' default version and convert that

Having Illuminate\Encryption\EncryptionServiceProvider provide second parameter with the value of app.cipher should be enough since it already available in the class. For new installation app.cipher is not available and hence it use the default. But for those upgrading from earlier version they can just update the config with the new value.

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jun 4, 2015

Contributor

How does this look?
#9054

Contributor

ibrasho commented Jun 4, 2015

How does this look?
#9054

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jun 4, 2015

Member

Looks really good. :)

Member

GrahamCampbell commented Jun 4, 2015

Looks really good. :)

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