Honestly, I think it's a bit silly that phpMyAdmin itself uses mcrypt when phpseclib uses mcrypt as well. Quoting http://phpseclib.sourceforge.net, "For purposes of speed, mcrypt is used if it's available". I think mcrypt support from phpMyAdmin ought to be dropped, instead relying on phpseclib's mcrypt implementation (and failing that phpseclib's pure-PHP implementation). And instead of adding OpenSSL support to phpMyAdmin I think it ought to be added to phpseclib. There's an open pull request for phpseclib that does this but it seems like it's not yet finished or something: https://github.com/phpseclib/phpseclib/pull/259 Maybe put pressure on them to get it done?
Last edit: playitoff 2014-07-22
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Honestly, I am not a friend of phpseclib. It is yet another PHP library that tries to solve stuff in PHP code that is available with better performance and in already existing non-PHP libraries such as mcrypt and openssl. If I am not mistaken, there is no performance gain in using phpseclib vs. php-mcrypt or php-openssl (due to its nature obviously). Why should one use phpseclib (where also no IV is used according to the phpMyAdmin code) rather mcrypt if mcrypt is available? Just wrapping it into phpseclib doesn't make sense either.
Given that I am a downstream maintainer, I dislike the "bundle everything to phpMyAdmin"-policy anyway. Linux distributions like Fedora (or also others) need to unbundle these libraries. And at that point using directly a library rather yet another implementation (as a wrapper) that anyway falls back to mcrypt is IMHO not a real improvement - aside from unnecessary performance loss by calling phpseclib rather mcrypt ;-)
Aside from the phpseclib vs. mcrypt thing I really would like to see php-openssl support additionally - because it does a) not hurt and does not b) bundle yet another library to phpMyAdmin and c) it is more performant than phpseclib in case mcrypt is unavailable. Finally php-openssl is usually always available; I didn't manage to find standard binary PHP packages without it (if I didn't overlook one)?!
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Honestly, I am not a friend of phpseclib. It is yet another PHP library that tries to solve stuff in PHP code that is available with better performance and in already existing non-PHP libraries such as mcrypt and openssl.
phpseclib uses mcrypt. that's what the OP said. it'll use it's own pure-PHP implementation if mcrypt is not available.
Last edit: Jim Wigginton 2014-07-23
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Yes, but he would like to see only phpseclib instead of mcrypt/openssl support directly in phpMyAdmin - if I am not completely mistaken. However I would like to be able to completely avoid phpseclib when packaging phpMyAdmin. The phpseclib is a separate software/library that should to be unbundled and thus separately maintained (when proper packaged) hence my suggestion for additional OpenSSL support without dropping anything. Other projects, such as Roundcube meanwhile prefer php-openssl, then php-mcrypt and then a pure PHP implementation as the last fallback. And this is exactly would I ideally like to see in phpMyAdmin, too.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Robert,
the patch looks good to me. However I got a warning about the IV not being the correct size (my cookie contained a mcrypt-based IV). As a workaround I added three lines like this:
if (function_exists('openssl_decrypt') && PHP_VERSION_ID >= 50304) {
if (strlen($this->_blowfish_iv) < 16) {
$this->createBlowfishIV();
}
Marc, thank you very much for reviewing the patch.
When reading your workaround I remembered to http://trac.roundcube.net/changeset/6c1c60f3b9/github, thus the following might be better to maintain (rather calculating the IV length manually if the cipher method changes in the future)? Replace 'AES-128-CBC' by 'BF-CBC' depending on your decision at the rest of the patch.
if (function_exists('openssl_decrypt') && PHP_VERSION_ID >= 50304) {
if (strlen($this->_blowfish_iv) < openssl_cipher_iv_length('AES-128-CBC')) {
$this->createBlowfishIV();
}
Comments? Or is that too much waste (even it should be minimal) of CPU time?
Robert,
I agree with your suggestion to compute the length.
Good news, I got the feedback from 2 other team members and your patch is not too late for 4.3.0.
However, someone will have to adjust test/classes/plugin/auth/PMA_AuthenticationCookie_test.php because testCookieEncrypt() and testCookieDecrypt() do not take care of the new library. testAuthSetUser() is also failing.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
summary: Patch: PHP OpenSSL support (additionally to PHP Mcrypt and phpseclib) --> (ok 4.3.0-alpha2) PHP OpenSSL support for cookie encryption/decryption
status: open --> resolved
Group: Needs_decision --> Next_release
Priority: 5 --> 1
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Honestly, I think it's a bit silly that phpMyAdmin itself uses mcrypt when phpseclib uses mcrypt as well. Quoting http://phpseclib.sourceforge.net, "For purposes of speed, mcrypt is used if it's available". I think mcrypt support from phpMyAdmin ought to be dropped, instead relying on phpseclib's mcrypt implementation (and failing that phpseclib's pure-PHP implementation). And instead of adding OpenSSL support to phpMyAdmin I think it ought to be added to phpseclib. There's an open pull request for phpseclib that does this but it seems like it's not yet finished or something: https://github.com/phpseclib/phpseclib/pull/259 Maybe put pressure on them to get it done?
Last edit: playitoff 2014-07-22
Honestly, I am not a friend of phpseclib. It is yet another PHP library that tries to solve stuff in PHP code that is available with better performance and in already existing non-PHP libraries such as mcrypt and openssl. If I am not mistaken, there is no performance gain in using phpseclib vs. php-mcrypt or php-openssl (due to its nature obviously). Why should one use phpseclib (where also no IV is used according to the phpMyAdmin code) rather mcrypt if mcrypt is available? Just wrapping it into phpseclib doesn't make sense either.
Given that I am a downstream maintainer, I dislike the "bundle everything to phpMyAdmin"-policy anyway. Linux distributions like Fedora (or also others) need to unbundle these libraries. And at that point using directly a library rather yet another implementation (as a wrapper) that anyway falls back to mcrypt is IMHO not a real improvement - aside from unnecessary performance loss by calling phpseclib rather mcrypt ;-)
Aside from the phpseclib vs. mcrypt thing I really would like to see php-openssl support additionally - because it does a) not hurt and does not b) bundle yet another library to phpMyAdmin and c) it is more performant than phpseclib in case mcrypt is unavailable. Finally php-openssl is usually always available; I didn't manage to find standard binary PHP packages without it (if I didn't overlook one)?!
phpseclib uses mcrypt. that's what the OP said. it'll use it's own pure-PHP implementation if mcrypt is not available.
Last edit: Jim Wigginton 2014-07-23
Yes, but he would like to see only phpseclib instead of mcrypt/openssl support directly in phpMyAdmin - if I am not completely mistaken. However I would like to be able to completely avoid phpseclib when packaging phpMyAdmin. The phpseclib is a separate software/library that should to be unbundled and thus separately maintained (when proper packaged) hence my suggestion for additional OpenSSL support without dropping anything. Other projects, such as Roundcube meanwhile prefer php-openssl, then php-mcrypt and then a pure PHP implementation as the last fallback. And this is exactly would I ideally like to see in phpMyAdmin, too.
Robert,
the patch looks good to me. However I got a warning about the IV not being the correct size (my cookie contained a mcrypt-based IV). As a workaround I added three lines like this:
if (function_exists('openssl_decrypt') && PHP_VERSION_ID >= 50304) {
if (strlen($this->_blowfish_iv) < 16) {
$this->createBlowfishIV();
}
Comments?
Also, do you agree with our DCO?
https://github.com/phpmyadmin/phpmyadmin/blob/master/DCO
Marc, thank you very much for reviewing the patch.
When reading your workaround I remembered to http://trac.roundcube.net/changeset/6c1c60f3b9/github, thus the following might be better to maintain (rather calculating the IV length manually if the cipher method changes in the future)? Replace 'AES-128-CBC' by 'BF-CBC' depending on your decision at the rest of the patch.
if (function_exists('openssl_decrypt') && PHP_VERSION_ID >= 50304) {
if (strlen($this->_blowfish_iv) < openssl_cipher_iv_length('AES-128-CBC')) {
$this->createBlowfishIV();
}
Comments? Or is that too much waste (even it should be minimal) of CPU time?
And yes, I do agree with your DCO at https://github.com/phpmyadmin/phpmyadmin/blob/master/DCO.
Robert,
I agree with your suggestion to compute the length.
Good news, I got the feedback from 2 other team members and your patch is not too late for 4.3.0.
However, someone will have to adjust test/classes/plugin/auth/PMA_AuthenticationCookie_test.php because testCookieEncrypt() and testCookieDecrypt() do not take care of the new library. testAuthSetUser() is also failing.
I adapted the patch for 4.3, including the unit tests.
Last edit: Marc Delisle 2014-11-18