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.

Sign up for GitHub

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

Jump to bottom

RSA: deal with openssl.cfg better #286

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

RSA: deal with openssl.cfg better #286

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a789914
RSA: deal with openssl.cfg better
terrafrost Mar 6, 2014
d1d3420
RSA: remove extra white space
terrafrost Mar 7, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions phpseclib/Crypt/RSA.php
View file
Open in desktop
Expand Up @@ -170,7 +170,7 @@
/**
* Default openSSL configuration file.
*/
define('CRYPT_RSA_OPENSSL_CONFIG', dirname(__FILE__) . '/../openssl.cnf');
define('CRYPT_RSA_OPENSSL_CONFIG', 'openssl.cnf');
Copy link
Member

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.

Copy link
Member Author

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.


/**#@+
* @access public
Expand Down Expand Up @@ -464,7 +464,14 @@ function Crypt_RSA()
include_once 'Math/BigInteger.php';
}

$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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty if block. ?_?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Expand Down