Skip to content
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

[WIP] Use external libraries for encrypt #698

Open
wants to merge 21 commits into
base: 3.4/develop
Choose a base branch
from

Conversation

enov
Copy link
Contributor

@enov enov commented Aug 17, 2016

This is a work-in-progress PR to remove crypto ops from the Kohana Encrypt class and instead interface it with external libraries.

Related to #686

Changes so far:

  • Suggest defuse/php-encryption
  • Kept a simple API with almost full BC, ex: Encrypt::instance()->encrypt($plaintext);
  • Methods encode/decode became encrypt/decrypt but this can be reverted back
  • Uses config based initialization.
  • Drivers should implement Kohana_Crypto interface and should name their classes in the format Encrypt_Drivername
  • Old mcrypt based Encrypt is still there, marked as deprecated, named Encrypt_Legacy to facilitate migration of data

This is still work in progress, but I wish I'd have a feedback sooner to adjust accordingly. I'd rebase/squash some commits when done.

Thank you for reviewing.

Cheers!

@acoulton
Copy link
Member

I'll have a proper look tomorrow/friday but from a quick skim this looks reasonable, thanks for the work.

I'd keep the method names the same - we're already pushing a chunk of breaking change and I don't think there's a huge benefit to renaming.

I would suggest removing the ability to encrypt from Encrypt_Legacy altogether. It could either not have the Kohana_Crypto interface, or could stay as is but throw from encrypt. We really just want to provide a way for the user to decrypt their existing secrets and re-encrypt them with the new driver.

End users who really don't care about security can always reimplement an Encrypt_Legacy pretty easily with the code from the 3.3 branch so I don't think we should make it easy for them.

@enov
Copy link
Contributor Author

enov commented Aug 18, 2016

Thanks @acoulton for the quick review. I will resume work once you have a deeper look into this.

I'd keep the method names the same - we're already pushing a chunk of breaking change and I don't think there's a huge benefit to renaming.

Alright.

I would suggest removing the ability to encrypt from Encrypt_Legacy altogether. It could either not have the Kohana_Crypto interface, or could stay as is but throw from encrypt. We really just want to provide a way for the user to decrypt their existing secrets and re-encrypt them with the new driver.

I suggest that we take a softer approach by raising an error within Encrypt_Legacy::encode():

trigger_error('Legacy driver is not considered secure anymore. Use Defuse driver instead.', E_USER_WARNING);

Users need to silence it manually with error_reporting(E_ALL & ~E_USER_WARNING); in case they don't care about security.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants