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

Use defuse/php-encryption v1.1 (which wasn't available through Composer) #8406

Merged
merged 6 commits into from Nov 13, 2015

Conversation

paragonie-scott
Copy link
Contributor

Fixes #8328 #8329

@paragonie-scott
Copy link
Contributor Author

Huh. Whitespace issues.

@mbabker
Copy link
Contributor

mbabker commented Nov 12, 2015

Don't mind that. PHPCS is set up to ignore known third party libraries, that path isn't one in the exclude list.

@paragonie-scott
Copy link
Contributor Author

I can move it to vendor/ if that will help.

@mbabker
Copy link
Contributor

mbabker commented Nov 12, 2015

I'll send you a PR in a few minutes (wrapping up paid work tasks). That'll take care of everything.

@brianteeman
Copy link
Contributor

Do we actually need to add this? If the only reason to add it is to provide a secure alternative to JCrypt, that if I understand correctly, we're not using in the core, for extensions isn't it enough just to have the readme like in #8407 recommending that the extensions use defuse/php-encryption and telling them where to get it so that they can bundle it with their extension.

Adding a library just in case someone might need to use it for their extension just doesnt seem right to me

@paragonie-scott
Copy link
Contributor Author

Getting defuse/php-encryption 1.1 (which is the only one that works with PHP 5.3) isn't as simple as composer require, since it wasn't added to composer until 1.2 was released. It might make it easier for folks who are running 5.3.10 if you bundle it.

That said, if you'd rather not want to maintain this code, you're right to not merge the PR.

@brianteeman
Copy link
Contributor

Joomla extensions typically do not install through composer

On 12 November 2015 at 22:21, Scott notifications@github.com wrote:

Getting defuse/php-encryption 1.1 (which is the only one that works with
PHP 5.3) isn't as simple as composer require, since it wasn't added to
composer until 1.2 was released. It might make it easier for folks who
are running 5.3.10 if you bundle it.

That said, if you'd rather not want to maintain this code, you're right to
not merge the PR.


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

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@mbabker
Copy link
Contributor

mbabker commented Nov 12, 2015

My suggestion is to put it in and look at a way to proxy JCrypt into it (even if means adding a new Cipher class that does the trick). Step one if this is going to be the encryption library going forward is to merge it now instead of wait for 4.0 and rip it all out at once.

@mbabker
Copy link
Contributor

mbabker commented Nov 12, 2015

@mbabker
Copy link
Contributor

mbabker commented Nov 13, 2015

@brianteeman I sent @paragonie-scott paragonie-scott#2 which implements the library as a new JCryptCipher class. So that'll get around the "we aren't using this" argument and actually strengthens the suggestion to folks using JCrypt to use this new object over the existing methods.

Add an implementation of JCryptCipher using the Crypto class
@paragonie-scott
Copy link
Contributor Author

I should probably update the text/example code in #8407 to match this usage.

@wilsonge
Copy link
Contributor

OK I've pinged this in our maintainers chat to test. Otherwise I'll get this tested tomorrow evening when I'm next free!

@roland-d
Copy link
Contributor

I have tested this item ✅ successfully on be3e3d2

I have tested this using this code

$cipher = new JCryptCipherCrypto();
$key = $cipher->generateKey(); // Store this for long-term use
$message = 'Joomla!Rocks';
$ciphertext = $cipher->encrypt($message, $key);
$decrypted = $cipher->decrypt($ciphertext, $key);

A key was generated, the ciphertext was generated and the message was decrypted again.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8406.

@zero-24
Copy link
Contributor

zero-24 commented Nov 13, 2015

I have tested this item ✅ successfully on be3e3d2

Works as expected:

$cipher = new JCryptCipherCrypto();
$key = $cipher->generateKey(); // Store this for long-term use

$message = 'Joomla!Rocks';
echo $message;
echo '<br>';

$ciphertext = $cipher->encrypt($message, $key);
echo $ciphertext;
echo '<br>';

$decrypted = $cipher->decrypt($ciphertext, $key);
echo $decrypted;

exit; 

Result

Joomla!Rocks
àœ™h°çg�íV|ܲ�p¸?Ã’a5ë¿Xë¼p¬Õ��¼Fç6ë æG¨œ�ÐÜ„QÂ�þEB�È×�™3�ÄÃcüˆ
Joomla!Rocks

Thanks.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8406.

@zero-24 zero-24 added this to the Joomla! 3.5.0 milestone Nov 13, 2015
@zero-24
Copy link
Contributor

zero-24 commented Nov 13, 2015

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8406.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 13, 2015
wilsonge added a commit that referenced this pull request Nov 13, 2015
Use defuse/php-encryption v1.1 (which wasn't available through Composer)
@wilsonge wilsonge merged commit 3b7b2d4 into joomla:staging Nov 13, 2015
@paragonie-scott paragonie-scott deleted the crypto branch November 15, 2015 00:59
@paragonie-scott
Copy link
Contributor Author

Excellent. 👍

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants