-
-
Notifications
You must be signed in to change notification settings - Fork 890
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
RSA: deal with openssl.cfg better #286
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -170,7 +170,7 @@ | |
/** | ||
* Default openSSL configuration file. | ||
*/ | ||
define('CRYPT_RSA_OPENSSL_CONFIG', dirname(__FILE__) . '/../openssl.cnf'); | ||
define('CRYPT_RSA_OPENSSL_CONFIG', 'openssl.cnf'); | ||
|
||
/**#@+ | ||
* @access public | ||
|
@@ -462,9 +462,17 @@ function Crypt_RSA() | |
{ | ||
if (!class_exists('Math_BigInteger')) { | ||
include_once 'Math/BigInteger.php'; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated/unnecessary change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yuck. Didn't realize I did that lol. Thanks for catching that! |
||
} | ||
|
||
$this->configFile = CRYPT_RSA_OPENSSL_CONFIG; | ||
$dir = '@cfg_dir@' . DIRECTORY_SEPARATOR . 'Crypt_RSA'; | ||
if (strpos($dir, '@') === false) { | ||
// PEAR installer was used to install the package | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty if block. ?_? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that in the orig commit but decided to keep it. The alternative would have a comment that says something like "if this condition wasn't met the PEAR installer was used to install the package", which is more wordy and if you're not careful you could miss the "n't" and think it was saying the exact opposite of what it really was. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could also interpret all sort of other things in a different way. Of course you have to read carefully and correctly, but this is a rather basic assumption. There should be proper comments and there should be no empty blocks. Having empty blocks is just ridicolous. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't say it was empty lol. There's no code there but there is a comment that satisfies the whole if / then paradigm. And from a performance perspective idk that it's any worse than a !==. I don't know that I would have thought to have done it that way but unless it's violating a coding standard or it's inconsistent with some other established way of doing things (eg. using fsockopen vs. socket_create; a departure I made from the orig ssh-agent PR) idk that I care that much. /shrug/ I guess you can change it if you want to idk. |
||
} else { | ||
// manual install | ||
$dir = dirname(__FILE__) . DIRECTORY_SEPARATOR . '..'; | ||
} | ||
$this->configFile = $dir . DIRECTORY_SEPARATOR . CRYPT_RSA_OPENSSL_CONFIG; | ||
|
||
if ( !defined('CRYPT_RSA_MODE') ) { | ||
// Math/BigInteger's openssl requirements are a little less stringent than Crypt/RSA's. in particular, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constant seems a bit (more) pointless now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems kinda pointless to me too tbh. /shrug/ I kept it for historical purposes. That the orig developer left it in place as well doesn't necessarily strike me as an endorsement on their part either.
If you want to remove it feel free. idk that, atm, I care enough about it to do it myself. It's not so much of an eye sore to me, atm, that it's an "omg must remove now" type thing but neither is it an "omg must keep" type thing either lol.