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

Password Generation Library #3208

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
8 participants
@sarciszewski

sarciszewski commented Aug 27, 2014

(I'm mostly creating this PR so Travis will run the tests. Cough.)

Features:

  • Generate a random password

Other changes made by this PR:

  • Added test cases
@narfbg

This comment has been minimized.

Contributor

narfbg commented Aug 27, 2014

I suppose you haven't looked at system/core/compat/ ... :)

@sarciszewski

This comment has been minimized.

sarciszewski commented Aug 27, 2014

No, I somehow didn't notice this. I suppose I should nuke the PBKDF2 parts... would this be any good as just a solid "random strings for passwords" library?

@narfbg

This comment has been minimized.

Contributor

narfbg commented Aug 27, 2014

Yea, when I mentioned a password library earlier I was thinking more of a password generator library/helper with options for different types of passwords.
If it is to be named CI_Password, it should just wrap around the password_*() functions (although I'm not a fan of that, kind of ties you to the library's specifics while the functionality stays the same).

@sarciszewski

This comment has been minimized.

sarciszewski commented Aug 27, 2014

Understood. What would be a good name for it then? CI_Password_generator?

@narfbg

This comment has been minimized.

Contributor

narfbg commented Aug 27, 2014

CI_Password_generator would be according to the styleguide.

Btw, take your time ... I didn't expect to see that in just a few hours. :)

@sarciszewski sarciszewski reopened this Aug 27, 2014

@sarciszewski sarciszewski changed the title from Password Generation and Hashing Library to Password Generation Library Aug 27, 2014

@sarciszewski

This comment has been minimized.

sarciszewski commented Aug 28, 2014

Okay, the tests are passing. At this point, I'd like to open the floor for criticism, review, suggestions, etc.

I'm also considering adding public funciton diceware($words) that would generate a diceware passphrase with a given number of words (minimum 5). However, this would require storing an array with 7776 elements somewhere-- either in configuration, a text file, or in the script itself. Anyone have thoughts on how to proceed with this? (Or, should we leave it alone?)

@narfbg

This comment has been minimized.

Contributor

narfbg commented Aug 28, 2014

You need to revert all changes to CI_Security - I've fixed the ctype_digit() issue (thanks for that one) and we already have hash_equals() (system/core/compat/hash.php) for constant time comparisons.

Also needs to comply with the styleguide ... stuff like TRUE vs true, where to put opening braces, etc.

On diceware() ... no, I don't think we should store such huge arrays anywhere within CI.

I'll leave inline comments on the patch for other details.

$this->security = new CI_Security();
$rules = config_item('password_rules');
if (!empty($rules)) {
$this->default_rules = $rules;

This comment has been minimized.

@narfbg

narfbg Aug 28, 2014

Contributor

If you'll be overwriting this, it shouldn't have 'default' in its name. :)

This comment has been minimized.

@narfbg

narfbg Aug 28, 2014

Contributor

Should also be a protected property.

* @return int
*/
protected function _random_positive_int() {
$buf = $this->security->get_random_bytes(PHP_INT_SIZE);

This comment has been minimized.

@narfbg

narfbg Aug 28, 2014

Contributor

Remove the $security property from this class and do this:

get_instance()->security->get_random_byes(PHP_INT_SIZE)

It's the only place you're using it.

$rules = array_merge($this->default_rules, $rules);
$charset = '';
$rules['lower'] AND $charset .= 'abcdefghijklmnopqrstuvwxyz';

This comment has been minimized.

@narfbg

narfbg Aug 28, 2014

Contributor

In similar expressions, we don't use just the raw variable ... it could trigger PHP errors if the var is missing. Also, AND -> &&:

isset($rules['lower']) && $charset .= 'abcdefghijklmnopqrstuvwxyz';

I'd also use another name instead of $charset ... you know what people will confuse it with. :)

* @param int $max Maximum value
* @return int
*/
public function random_number($min, $max)

This comment has been minimized.

@narfbg

narfbg Aug 28, 2014

Contributor

Call it get_random_number(), or get_random_integer() and cast the return value to (int).

protected function _random_positive_int() {
$buf = $this->security->get_random_bytes(PHP_INT_SIZE);
if($buf === false) {
trigger_error('Random number failure', E_USER_ERROR);

This comment has been minimized.

@narfbg

narfbg Aug 28, 2014

Contributor

return FALSE or use show_error() (CI-specific)

trigger_error() is only used in system/core/compat/ to mimic PHP's behavior as much as possible.

*/
public function generate(
$length = 16,
$rules = array()

This comment has been minimized.

@narfbg

narfbg Aug 28, 2014

Contributor
array $rules = NULL

Then a simple isset() would tell you if the parameter was used, while it would be otherwise guaranteed to be an array. :)

// --------------------------------------------------------------------
/**
* Return a random integer in the range [0, PHP_INT_MAX)

This comment has been minimized.

@narfbg

narfbg Aug 28, 2014

Contributor

Functionally irrelevant of course, but you're opening with a square bracket and closing with parenthesis :D

This comment has been minimized.

@sarciszewski

sarciszewski Aug 28, 2014

Math notation for ranges. Means up to but not including :)

@sarciszewski

This comment has been minimized.

sarciszewski commented Sep 16, 2014

Uh oh, there's a merge conflict? Hmm.

@ivantcholakov

This comment has been minimized.

Contributor

ivantcholakov commented Dec 16, 2014

Well?

@sarciszewski sarciszewski force-pushed the sarciszewski:develop branch from 689bcb8 to ee86430 Dec 17, 2014

@sarciszewski

This comment has been minimized.

sarciszewski commented Dec 17, 2014

Squashed and resolved the merge conflict.

@sarciszewski sarciszewski force-pushed the sarciszewski:develop branch 6 times, most recently from fd7337e to f5124cf Dec 17, 2014

* @param string $b The other string being compared
* @return boolean
*/
public function compare($a, $b)

This comment has been minimized.

@narfbg

narfbg Dec 17, 2014

Contributor

We have hash_equals() in system/core/compat/, you don't need this. :)

@narfbg

This comment has been minimized.

Contributor

narfbg commented Dec 17, 2014

You deffinately need to rework this to match our styleguide, add a changelog message and documentation.

public function test___construct()
{
$CI =& get_instance();
$this->assertTrue($CI>security instanceof CI_Security);

This comment has been minimized.

@narfbg

narfbg Dec 17, 2014

Contributor

Oddly, this is not a syntax error, but still erroneous. :)

Edit: And, um ... this test is kind of pointless. :)

@ircmaxell

This comment has been minimized.

ircmaxell commented Dec 19, 2014

@ivantcholakov I wrote a blog post recently about that topic: Educate, Don't Mediate.

I think the rationale for replicating a security critical library in you code is flawed. If you don't have the skills to maintain such a library, then you're actively doing your user base harm by including it. Instead, educate the user base on how to do it properly.

The rest of the industry has adopted the component approach, because it has nothing but advantages once you get past the initial learning curve. And that learning curve is not that difficult.

You should be helping users over that learning curve. Not telling them they don't need to learn it because you'll just provide your own alternative.

At the moment all the code is CodeIgniter's only, it is some kind of tradition from the past.

Complacency kills. Not realizing that the past was flawed, and continuing it will actively harm people.

I could care less about the project and the direction that it takes. What I care about is the users who the project will take with them. They deserve to learn the better way, not just the historical one.

The same problem is about the supported PHP version, it is too late for changes.

It was too late for changes 4 years ago when PHP 5 was EOL. That's when it should have been done.

So you say "too late for changes". I say "Better late than never".

@narfbg

This comment has been minimized.

Contributor

narfbg commented Dec 19, 2014

Boy, that escalated quickly ...

@ircmaxell So, are you -1 on this because of the minimum version requirement or because you don't believe such a library should be a framework's part instead of a stand-alone component?

I can agree with the latter.

@sarciszewski It doesn't even come close to what the CI styleguide suggests.

@ivantcholakov

This comment has been minimized.

Contributor

ivantcholakov commented Dec 19, 2014

@ircmaxell

"Educate, Don't Mediate" - Teaching is not my business, I serve my clients. CodeIgniter is not an educational toy, it serves business, it provides solutions. For mature people education is personal investment and personal responsibility.

"If you don't have the skills to maintain such a library, then you're actively doing your user base harm by including it." - I understand every line of the proposed code. When I wrote "qualified persons", I ment a good procedure, a formality. It is good code to be re-checked, but I see that you are reluctant to do so. Thank you very much for sharing your thoughts anyway.

@sarciszewski

This comment has been minimized.

sarciszewski commented Dec 19, 2014

It doesn't even come close to what the CI styleguide suggests.

Oh. Is it a whitespace issue? A capitalization issue? I don't know what I'm overlooking.

@narfbg

This comment has been minimized.

Contributor

narfbg commented Dec 19, 2014

Well, it's a lot of stuff, that's why I said it doesn't come close. :) I'll leave some inline comments ...

public function __construct()
{
$rules = config_item('password_rules');
if (!empty($rules)) {

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

Spaces around the ! character.

We also always put curly braces on a new line.

* types ('digit', 'upper', 'lower', 'special')
*/
public function create_password(
$length = 16,

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

We don't list arguments like this ... function signatures are always a single line.

$rules['special'] = FALSE;
}
// Include lowercase characters?

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

These comments are a bit redundant, it's pretty clear what's going on in the code. :)

}
// Include lowercase characters?
$rules['lower'] && $keyspace .= 'abcdefghijklmnopqrstuvwxyz';

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

Might as well change this (and the similar ones below) to:

empty($rules['lower']) OR $keyspace .= 'abcdefghijklmnopqrstuvwxyz';

... and then you can remove the checks between lines 85 and 97 altogether.

// Include other ASCII printable special characters?
$rules['special'] && $keyspace .= '`~!@#$%^&*()+=_-[]{}:;\'"\\|,./<>?';
if ($rules === '') {

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

Are you sure about === '' here? Can it ever be an empty string? I'd switch it to empty($rules).

// With no keyspace to work with, we cannot return anything.
return FALSE;
}
return $this->get_random_string($length, $keyspace);

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

Put newlines prior to returns

// integer between 0 and 9.
$j = $this->get_random_number(0, strlen($keyspace) - 1);
if ($j === FALSE) {

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

You're obviously using 4-space indentation instead of tabs here. :)

// There was no room for variation!
return $min;
}
++$range;

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

4 spaces here too ... you've got that a few more times.

++$range;
// 7776 -> 13
$bits = ceil(log($range)/log(2));

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

Put spaces around / ... the rule in general is to put a single space on each side of an operator, except for concatenation.

$buf = $CI->security->get_random_bytes(PHP_INT_SIZE);
if($buf === FALSE) {
show_error('An unknown random number failure has occurred.');
return FALSE;

This comment has been minimized.

@narfbg

narfbg Dec 19, 2014

Contributor

show_error() does an exit() call (or die(), not sure) ... this return is dead code. :)

@defuse

This comment has been minimized.

defuse commented Dec 19, 2014

FWIW I have a side-channel resistant PHP password generator here that you're free to draw inspiration from: https://github.com/defuse/php-passgen

@ircmaxell

This comment has been minimized.

ircmaxell commented Dec 21, 2014

@narfbg The latter. The version discussion was a side-track (I still think it's incredibly important, as you can see from my recent blog post, but unrelated to this issue).

But I think fundamentally the component route is a far safer one, and strongly believe that we don't need duplicate functionality everywhere, especially in a security context.

@sarciszewski

This comment has been minimized.

sarciszewski commented Dec 21, 2014

Okay, I'm going to close this PR and move the discussion towards incorporating an independent component in #3432

@ivantcholakov

This comment has been minimized.

Contributor

ivantcholakov commented Dec 21, 2014

Personally I have no problem with this decision. But something must be done. I saw this https://github.com/bcit-ci/CodeIgniter/wiki/Random-Password-Generator

@ircmaxell

This comment has been minimized.

ircmaxell commented Dec 22, 2014

@ivantcholakov completely fair. But to fix that, educate it. Fix the wiki post, educate users. Teach them. That's how you fix this in the long term...

@sarciszewski

This comment has been minimized.

sarciszewski commented Dec 22, 2014

Eww, yeah. So...

  • Should we fall back to php-passgen by @defuse?
  • Is there a better alternative library to use?
@ivantcholakov

This comment has been minimized.

Contributor

ivantcholakov commented Dec 23, 2014

@sarciszewski A long series of day-offs is comming, I feel uncomfortable to bother people.

@narfbg

This comment has been minimized.

Contributor

narfbg commented Dec 23, 2014

@sarciszewski I see nothing wrong in linking to either @defuse's or @ircmaxell's password-generating libraries.

However, I also think bundling a copy would be a mistake and I'd rather recommend them via documentation or on that wiki page.

@sarciszewski

This comment has been minimized.

sarciszewski commented Dec 23, 2014

Makes sense to me.

@dmyers2004

This comment has been minimized.

dmyers2004 commented Dec 23, 2014

I would rather see these as composer packages. Since CI 3 has composer support already.
I think that we should leverage that much like we where trying with sparks.
Maybe a wiki section for "codeIgniter ready" composer packages or something?
You could add a single line to composer, grab the package, and start using it in 3 minutes (if you're a slow typer and connected over 56k)

@sarciszewski

This comment has been minimized.

sarciszewski commented Dec 23, 2014

"I would rather see these as composer packages."

Sure, let's just encourage more developers to pipe data retrieved over a network to a scripting language interpreter!

(I'm working on making Composer do asymmetric signature verification, but until it's ready I don't recommend using Composer.)

@ircmaxell

This comment has been minimized.

ircmaxell commented Dec 23, 2014

FWIW: composer requires 5.3.2... So isn't adding composer support raising your minimum? ;-)

@dmyers2004

This comment has been minimized.

dmyers2004 commented Dec 23, 2014

Good points.
Using composer with CodeIgniter 3 isn't "required" (therefore 5.3.2 isn't required) but it is already in CI 3. So perhaps a disclaimer in the manual?

http://www.codeigniter.com/userguide3/general/autoloader.html?highlight=composer

Perhaps a better approach would be to include "with composer" and "without composer" as part of your "CodeIgniter Ready" instructions? For those concerned about the security issues and/or have a version of PHP which is to old. You lose the autoloader but as long as it's 5.2.4 compatible you should still be able to attach and use it.

@ivantcholakov

This comment has been minimized.

Contributor

ivantcholakov commented Jan 29, 2017

@sarciszewski

This password library should be published at GitHub, it is a well done job that is going to be lost. If you don't have the time, would you agree if I become a maintainer of this library under the MIT license on your name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment