Category: Utiltity Scripts
Author/Contact Info coded by munkyeetr. This is my first Perl script, so I am looking for some feedback; what could I do better and more efficien
Description: Command line script that can generate multiple random passwords of any length. User can exclude character-types by using command line options. I believe I have handled foreseeable errors, and that the code is well commented. REQUIRED MODULES: Getopt::Long
#!/usr/bin/perl

use strict;
use warnings;
use Getopt::Long;
Getopt::Long::Configure ("bundling");

# FUNCTION: GetRandNum()
# ---------------------------------------------------------
sub GetRandNum() {
    my $lower=33;
    my $upper=126;

    my $random = int(rand( $upper-$lower+1 )) + $lower;

    return $random;
}
# ---------------------------------------------------------

# SUB: DisplayUsage()
# ---------------------------------------------------------
sub DisplayUsage() {
    print "\nUsage: genpass [-OPTIONS] LENGTH\n";
    print "Generate secure passwords LENGTH characters long.\n\n";    
    print "\t-s, --symbols\t\tExclude symbol characters.\n";
    print "\t-n, --numbers\t\tExclude number characters.\n";
    print "\t-u, --uppercase\t\tExclude uppercase characters.\n";
    print "\t-l, --lowercase\t\tExclude lowercase characters.\n\n";
    print "\t-q(X)\t\t\tCreate X number of passwords.\n\n";
    print "\t--help\t\t\tDisplay this usage screen.\n\n";
    print "Report bugs, comments, and questions to jbrown_home\@yahoo.
+ca\n\n";
}
# ---------------------------------------------------------

# DECLARE OPTION FLAGS
# ---------------------------------------------------------
my $symbols='0';        # all self-explanatory
my $numbers='0';
my $uppercase='0';
my $lowercase='0';
my $help='0';
my $qty='0';

# PARSE AND SET COMMAND-LINE OPTIONS
# ---------------------------------------------------------
GetOptions( 's|S|symbols' => \$symbols,
        'n|N|numbers' => \$numbers,
        'u|U|uppercase' => \$uppercase,
        'l|L|lowercase' => \$lowercase,
        'q|Q:i' => \$qty,
        'help' => \$help, );

## START INPUT CHECKING HERE
# ---------------------------------------------------------
my $kill='0';    # flag to stop the script if input is invalid (or --h
+elp is used)
my $errcount=0;    # count error messages    
my @errmsg;    # error message array

# If -q option was used to set a quantity of passwords, make sure it c
+ontains at
# least a value of 1 so that a password can be generated
# ---------------------------------------------------------
if ($qty lt '0' || $qty eq '0') {
    $qty='1';
}

# Check for --help option, kill script
# (This is not really an error, but I put it here because of the $kill
+ flag)
# ---------------------------------------------------------
if ($help eq '1') {
    $kill='1';        
}

# Check that user hasn't excluded all character-types, warn user, kill
+ script
# ---------------------------------------------------------
if (($symbols eq '1') && ($numbers eq '1') && ($uppercase eq '1') && (
+$lowercase eq '1')) {
    $errmsg[$errcount]="\n** 0x1: At least 1 character-type must be in
+cluded";    
    $kill='1';
    $errcount++;
}

# Check that user has passed only 1 argument (LENGTH), warn user, kill
+ script
# ---------------------------------------------------------
if ($#ARGV > 0 || $#ARGV < 0) {
    $errmsg[$errcount]="\n** 0x2: Incorrect number of arguments passed
+";
    $kill='1';
    $errcount++;
}
# Check for only numeric input in LENGTH argument, warn user, kill scr
+ipt
# ---------------------------------------------------------
else { 
    if($ARGV[0] !~ /^[0-9]+$/) {
        $errmsg[$errcount]="\n** 0x3: Invalid input. LENGTH argument m
+ust be a numeric value";
        $kill='1';
        $errcount++;
    }
}

# If any of the above tests triggered the $kill flag, display errors a
+nd kill script
# ---------------------------------------------------------
if ($kill eq '1' && $help eq '1') {        # If triggered by the --hel
+p option
    DisplayUsage();    
    exit();
}
elsif ($kill eq '1') {                 # If triggered by an error
    print "\n** GENPASS ERROR ----------------------------------------
+-----------------";
    print "\n** ".$errcount." Error(s) found";  # show number of error
+s    
    foreach my $err (@errmsg) {         # display error messages
        print $err;
    }
    print "\n**\n** Type genpass --help for command usage\n";
    print "** --------------------------------------------------------
+---------------\n\n";
    exit();                     # exit script
}
# ---------------------------------------------------------
## END INPUT CHECKING HERE

## MAIN SCRIPT STARTS HERE
# ---------------------------------------------------------
my $pwd="";        # holds password        
my $length=$ARGV[0];    # length argument passed by user

while ($qty gt '0') {        # while we have passwords to generate
    $pwd=&GenPass($length); # generate a password
    print $pwd;        # display the password
    $qty--;            # we have 1 less to make
}
# ---------------------------------------------------------
## MAIN SCRIPT ENDS HERE

# FUNCTION: GenPass()
# DESCRIPTION: Generates a password of variable length based on option
+s passed on the command line.
# ARGUMENTS: Integer value "Length" assigned to $pwdlen
# RETURNS: String $password
# ---------------------------------------------------------
sub GenPass() {

my $pwdlen=$_[0];    # password length
my $count=0;        # a counter
my $password="";    # string to hold the password as it's being genera
+ted
my $isgood=0;        # validity flag
my $num=0;        # will hold random numbers

# loop until our password is the desired length
    while ($count < $pwdlen) {

        $isgood=0;        # reset the isgood flag
        $num=GetRandNum();    # get a random number

    # test the random number to see if it is a valid character
    # and change the isgood flag if it hasn't been excluded by the use
+r...

        if (($num >= 33 && $num <= 47) || ($num >= 58 && $num <= 64) |
+| ($num >= 91 && $num <= 95) || ($num >= 123 && $num <= 126)) {
            if ($symbols eq '0') {    # symbols        
                $isgood=1;    
            }
        } elsif (($num >= 48 && $num <= 57)) {
            if ($numbers eq '0') {    # numbers
                $isgood=1;    
            }
        } elsif (($num >= 65 && $num <= 90)) {
            if ($uppercase eq '0') { # uppercase    
                $isgood=1;    
            }
        } elsif (($num >= 97 && $num <= 122)) {
            if ($lowercase eq '0') { # lowercase
                $isgood=1;    
            }
        }
        else {
            ; #do nothing
        }
    # test the isgood flag, and if the character is good
        if ($isgood == 1) {
            $password=$password.chr($num);     # append the character 
+to the password string
            $count++;            # increment the count
        }
    # or else do nothing, and loop again...
    
    }

# return the finished password to main script
    return $password."\n";
}
# ---------------------------------------------------------
__END__
Replies are listed 'Best First'.
Re: genpass Password Generator
by graff (Chancellor) on Jun 02, 2007 at 17:39 UTC
    Very clear, well thought out, and quite readable ++ no problem. Just a tad on the verbose side (using string comparisons where numeric comparisons will do, using more complicated conditions where simpler ones will do, using an unnecessary counter variable to track the size of an array, trivial nit-picky things of that sort).

    There would be a lot less code to do the same thing if you start with a string that has all characters available for use in passwords, then remove classes of characters according to the given options:

    my $pswd_chars = join( '', map {chr} ( 0x21 .. 0x7e )); $pswd_chars =~ s/\d+// if ( $numbers ); $pswd_chars =~ s/[A-Z]+// if ( $uppercase ); $pswd_chars =~ s/[a-z]+// if ( $lowercase ); $pswd_chars =~ s/[_\W]+//g if ( $symbols ); if ( length( $pswd_chars ) == 0 ) { push @errmsg, "\n *** You eliminated all possible characters."; }
    (update: added a couple of missing "+" to the regexen)

    Also, if someone wants to use this to create passwords to be handed out to clients (i.e. via email or web transactions), another useful option would be to eliminate confusable characters:

    $pswd_chars =~ tr/1Il0O//d if ( $confusable );
    As for random selection based on the remaining characters:
    sub GenPass { my ( $pswd_chars, $pswd_len ) = @_; my $limit = length( $pswd_chars ); my $pswd = ''; for ( 0..$pswd_len-1 ) { $pswd .= substr( $pswd_chars, rand( $limit ), 1 ); } return $pswd; }
    Last suggestion: definitely check out POD, and Pod::Usage -- you'll fall in love with that.
Re: genpass Password Generator
by ww (Archbishop) on Jun 02, 2007 at 12:20 UTC
    Wiser monks than I may well offer suggestions in the vein you suggested, but ++.

    Update: However, unless I misread this, your randomization does NOT account for the differing quantities of available/extant "numbers (10)," "symbols (depends on character set)," "UC_letters (ditto)," and "lc_letters (ditto again)." If I'm correct, that may induce a pattern of output that would be detectable over a relatively small sample, and thus might reduce the work involved in breaking the p/w generated. OTOH, that might actually make breaking the p/w harder. Comments, anyone?   </update>

    Also, the comment at line 81 doesn't seem quite right, in the overall context since to achieve exclusions, such as -N, the invocation must have multiple arguments -- one for length, and one for the exclusion.

    One other trivial and highly personal observation (YMMV), I find the use multiple dashes as leading- and trailing-sub/function/etc delimiters (eg, lines like 9, 18, 21 & 33) "intrusive" (for lack of a better word) and somewhat un-lazy. FW(little)IW, a pair of newlines generally achieves - for me - the kind of clarity you're seeking (for which, another ++).

Re: genpass Password Generator
by GrandFather (Saint) on Jun 02, 2007 at 23:06 UTC

    Heredocs are a nice way to tidy up help messages and such. Consider:

    sub DisplayUsage { print <<" USAGE"; Usage: genpass [-OPTIONS] LENGTH Generate secure passwords LENGTH characters long. -s, --symbols\t\tExclude symbol characters. -n, --numbers\t\tExclude number characters. -u, --uppercase\t\tExclude uppercase characters. -l, --lowercase\t\tExclude lowercase characters. -q(X)\t\t\tCreate X number of passwords. --help\t\t\tDisplay this usage screen. Report bugs, comments, and questions to jbrown_home\@yahoo.ca USAGE }

    Note also that the prototype has been omitted from the sub. Apart from exceptional cases, prototypes are not useful and tend to lead to subtle and hard to find bugs.

    Note too that the white space is an important part of the here document tag. There is a nasty and subtle trap here - the white space must match exactly for the end of the document to be recognised.

    I'd be inclined to rewrite the password generation sub a little: ;)

    sub GenPass { my ($pwdlen, %flags) = @_; my $password=""; my $num; while (length ($password) < $pwdlen) { $num = GetRandNum (); if (($num >= 33 && $num <= 47) || ($num >= 58 && $num <= 64) | +| ($num >= 91 && $num <= 95) || ($num >= 123 && $num <= 126) ) { next if $flags{symbols} eq '0'; } if (($num >= 48 && $num <= 57)) { next if $flags{numbers} eq '0'; } if (($num >= 65 && $num <= 90)) { next if $flags{uppercase} eq '0'; } if (($num >= 97 && $num <= 122)) { next if $flags{lowercase} eq '0'; } redo; # Invalid character, try again } continue { $password .= chr($num); } return "$password\n"; }

    Main points are:

    • Don't prototype the sub
    • Don't use global variables (note the %flags hash in particular)
    • Don't comment the obvious. A well named variable like $password doesn't need a comment to tell the reader that it contains the password.
    • Look for ways to reduce nesting. In this case continue does the trick.
    • Define variables as locally as possible so that their scope is more obvious.
    • Avoid duplicating information (in this case note that $count is not required)

    DWIM is Perl's answer to Gödel
Re: genpass Password Generator
by Anonymous Monk on Jun 04, 2007 at 03:02 UTC
    Thank you everyone for your comments, critiques and suggestions. I have looked over my code and made changes where they made sense to me.

    Some of the suggestions left me with nothing to do but cut and paste the code in the reply (see the GenPass function, the use of a flags hash, the heredocs tip.) I did, of course, look over what the code did and saw why it was a better way, because unlike some who post questions on forums, I have no problem doing the work, I'm not looking for other people to do the work for me. I am here to learn. I just want that to be clear.

    I think the biggest efficiency tip for me was putting all the characters in a string, then using regex to pull excluded types out of the string. Not only did that alone cut nearly 50 lines of code from the script, but it also cut out all the extraoneous loops that tested "dead" value characters between 33 and 126.

    One thing that I may look into is incorporating a switch to handle (or offer) different character sets.

    Again, thank all you monks, and here I post the updated version. Cheers:

    #!/usr/bin/perl use strict; use warnings; use Getopt::Long; Getopt::Long::Configure ("bundling"); ## PARSE AND SET COMMAND-LINE OPTIONS ## ----------------------------------------------------- my %flags=('symbols', 0, 'numbers', 0, 'uppercase', 0, 'lowercase', 0, + 'confusable', 0, 'help', 0, 'qty', 1); GetOptions( 's|S|symbols' => \$flags{symbols}, 'n|N|numbers' => \$flags{numbers}, 'u|U|uppercase' => \$flags{uppercase}, 'l|L|lowercase' => \$flags{lowercase}, 'c|C|confusable' => \$flags{confusable}, 'q|Q:i' => \$flags{qty}, 'help' => \$flags{help}, ); # Set password characters, excluding those flagged on the command-line my $pwdchars = join( '', map {chr} ( 0x21 .. 0x7e )); $pwdchars =~ s/\d+// if ( $flags{numbers} ); $pwdchars =~ s/[A-Z]+// if ( $flags{uppercase} ); $pwdchars =~ s/[a-z]+// if ( $flags{lowercase} ); $pwdchars =~ s/[_\W]+//g if ( $flags{symbols} ); $pwdchars =~ tr/1Il0O//d if ( $flags{confusable} ); # If user triggered the --help option flag, display and exit if ($flags{help}) { &DisplayUsage(); exit(); } ## START VALIDATE INPUT ## ----------------------------------------------------- my $kill=0; # flag to stop the script if input is invalid (or - +-help is used) my @errmsg; # error message descriptions # If -q option was used to set a quantity of passwords, make sure it c +ontains at # least a value of 1 so that a password can be generated if ($flags{qty} == 0 || $flags{qty} < 0) { $flags{qty}=1; } # Check that user hasn't excluded all character-types, warn user, kill + script if ( length($pwdchars) == 0) { push @errmsg, "** 0x1: At least 1 character-type must be included" +; $kill=1; } # Check that user has passed only 1 argument (LENGTH) other than optio +ns flags, warn user, kill script if ($#ARGV > 0 || $#ARGV < 0) { push @errmsg, "** 0x2: Incorrect number of arguments passed"; $kill=1; } # Check for only numeric input in LENGTH argument, warn user, kill scr +ipt if ($ARGV[0] !~ /^[0-9]+$/) { push @errmsg, "** 0x3: Invalid input. LENGTH argument must be +a numeric value"; $kill=1; } # If any of the above validation tests triggered the $kill flag... if ($kill == 1) { print "\n** GENPASS ERROR ---------------------------------------- +-----------------"; print "\n** ".@errmsg." Error(s) found"; # display number of +errors foreach my $err (@errmsg) { # display error messages print "\n".$err; } print "\n**\n** Type genpass --help for command usage\n"; print "** -------------------------------------------------------- +---------------\n\n"; exit(); # exit script } ## END VALIDATE INPUT ## START MAIN SCRIPT ## ----------------------------------------------------- # From 1 to qty for ( 1..$flags{qty} ) { print &GenPass( $ARGV[0] )."\n"; } exit(); ## END MAIN SCRIPT ## FUNCTION DEFINITIONS ## ----------------------------------------------------- sub GenPass() { my ($pwdlen) = @_; my $limit = length( $pwdchars ); my $pwd = ''; for ( 0..$pwdlen-1 ) { $pwd .= substr( $pwdchars, rand( $limit ), 1 ); } return $pwd; } # use Here-Documents to display usage text sub DisplayUsage { print <<" USAGE"; Usage: genpass [-OPTIONS] LENGTH Generate secure passwords LENGTH characters long. -s, --symbols\t\tExclude symbols. -n, --numbers\t\tExclude numbers. -u, --uppercase\t\tExclude uppercase letters. -l, --lowercase\t\tExclude lowercase letters. -c, --confusable\tExclude confusable characters like: l,I,1,0,O -q(X)\t\t\tCreate X number of passwords. --help\t\t\tDisplay this usage screen. Report bugs, comments, and questions to jbrown_home\@yahoo.ca USAGE } __END__
      Sorry, I wasn't logged in when I posted that last...That was me, munkyeetr.

        A few more comments mixing in random order matters of personal style and more substantial things:

        Putting all of the above (and a few minor cosmetic touches) together:

        Some tests:

        One thing that I may look into is incorporating a switch to handle (or offer) different character sets.

        That, IMO, would be asking for a lot of trouble that you really don't need, in order to supply a "feature" that virtually all sensible users really don't want.

        Frankly, the world is just not ready yet for passwords containing non-ASCII characters. Not only do we lack stable/consistent conventions for their keyboard entry, but their encoding and transmission are still mired in an embarrassingly wide range of "alternative methods" and obscure constraints. Forget about it.