Password Generator Python

Question

I decided to make a password generator to get more familiar with Python.

I had to break up the code into two boxes due to weird Stack Exchange highlighting with the triple quotes.

How would you improve this program on the grounds of:

• Readability/cleanness,
• Performance, and
• Security.

Feel free to include any other comments you have on it that may not fit into those three categories.

import string
import random
import secrets
import argparse
import MyFormatter
alphaL = list(string.ascii_lowercase)
alphaU = list(string.ascii_uppercase)
numeric = list(string.digits)
special = list("!@#$%^&*")
special2 = list("""~`!@#$%^&*()+=_-{}[]\|:;"'?/<>,.""")
parser = argparse.ArgumentParser(
 formatter_class=MyFormatter.MyFormatter,
 description="Generates a password",
 usage="",
)
parser.add_argument("-lc", "--lower", type=int, default=1, help="Minimum number of lowercase alpha characters")
parser.add_argument("-uc", "--upper", type=int, default=1, help="Minimum number of uppercase alpha characters")
parser.add_argument("-n", "--numeric", type=int, default=1, help="Minimum number of numeric characters")
parser.add_argument("-s", "--special", type=int, default=1, help="Minimum number of special characters")
parser.add_argument("-se", "--extended", action = 'store_const', default = False, const= True, help="Toggles the extendard special character subset. Passwords may not be accepted by all services")
parser.add_argument("-l", "--length", type=int, default=20, help="Length of the generated password")
args = parser.parse_args()
length = args.length
minimums = [
 args.lower,
 args.upper,
 args.numeric,
 args.special,
]
password = ""
for i in range(0, minimums[0]) :
 password += secrets.choice(alphaL)
 
for i in range(0, minimums[1]) :
 password += secrets.choice(alphaU)
for i in range(0, minimums[2]) :
 password += secrets.choice(numeric)
if args.extended :
 subset = alphaL + alphaU + numeric + special2
 for i in range(0, minimums[3]) :
 password += secrets.choice(special2)
elif minimums[3] :
 subset = alphaL + alphaU + numeric + special
 for i in range(0, minimums[3]) :
 password += secrets.choice(special)
for i in range(0, 100) :
 random.shuffle(subset)
 
for i in range(len(password), length) :
 password += secrets.choice(subset)
for i in range(0, 100) :
 password = ''.join(random.sample(password, len(password)))
print("Password: ", password)

• Jan Kuiken: I cannot import MyFormatter, is it a module of yours? If so, please add it to the question.
• Kadragon: It is a module of mine, and I can add it if you'd like, but it just does some formatting for the help command and doesn't add anything to the program itself. I'll try to remember to replace "MyFormater.MyFormater" with "HelpFormatter"

Answer 1

Already pointed out by other users

• There is no need to make lists from the strings as any sequence will do
• minimums - you make bad names where the existing names were better

The if clause

if args.extended :
 subset = alphaL + alphaU + numeric + special2
 for i in range(0, minimums[3]) :
 password += secrets.choice(special2)
elif minimums[3] :
 subset = alphaL + alphaU + numeric + special
 for i in range(0, minimums[3]) :
 password += secrets.choice(special)

is erroneous. The check for elif minimums[3] shall be replaced by an else:

Also - just because you already switch on args.extended you are not required to pack all variation on that argument in there. You prepare subset for the logically next function (choice from compound set) before even performing the current one (choice from special). That results in less readability. There is nothing wrong in switching two times if required. However in this case you should simply switch

if args.extended:
 special = """~`!@#$%^&*()+=_-{}[]\|:;"'?/<>,."""
else:
 special = "!@#$%^&*"

Then the choice from special/special2 collapses to the standard pattern.

Names

It was already mentioned that the minimum list is a name worse than the args attributes (which could be even more descriptive). Also the names for your imports are bad and camel-cased. why not simply follow the existing naming scheme?

ascii_lowercase = string.ascii_lowercase

or even more straight

from string import ascii_lowercase
from string import ascii_uppercase
from string import digits

An extra bad name is subset

subset = alphaL + alphaU + numeric + special

which is clearly a union or superset

The standard pattern

You do

for i in range(0, minimums[0]) :
 password += secrets.choice(alphaL)

the recommended way to character wise join a string is

password += "".join(secrets.choice(ascii_lowercase) for _ in range (0, args.lower)

Cargo cult

You do

for i in range(0, 100) :
 random.shuffle(subset)

which is pointless. It is pointless to shuffle a 100 times. there is no extra quality in randomness compared to a single shuffle. It is even more pointless to do that before apply secrets.choice(subset). You could safely sort subset before choice().

Final shuffle

You do

for i in range(0, 100):
 print("Password: ", password)
 password = ''.join(random.sample(password, len(password)))

which I interpret as final shuffle to eliminate the order of the subsets ascii_lowercase, ascii_uppercase, ...

Again you shuffle a 100 times which will not add any value compared to a single shuffle. To be cryptographically safe you should implement a shuffle based on secrets.choice.

• Kadragon: If clause: I see how that is cleaner, thank you. Names: once again thank you. Pattern: Fundamentally, why is this better? Behind the scenes, what's the difference? Cargo Cult/Final shuffle: I assumed a few more shuffles (Since the program is still effectively instant) would be btter, which I was wrong. Thank you.
• stefan: join is more efficient - see stackoverflow.com/questions/39312099/…. that is neglible if joining two strings but makes a difference if joining in a loop.

Answer 2

Redundant casts

alphaL = list(string.ascii_lowercase)
alphaU = list(string.ascii_uppercase)
numeric = list(string.digits)
special = list("!@#$%^&*")
special2 = list("""~`!@#$%^&*()+=_-{}[]\|:;"'?/<>,.""")

Are you sure that casts to list are needed here? For example, string.ascii_lowercase is a str, which is already a sequence of strings (each one character). secrets.choice says:

Return a randomly-chosen element from a non-empty sequence.

So it will be able to handle the strings directly. Casting to a list is actually regressive, since list is mutable but these sequences should not be.

Latin

It's really pedantic, but maximums should be maxima.

Named arguments

There's no point to the minimums list. Every time you reference it, you reference one of its elements by index; so just reference the arguments from which it was initialized and get rid of the list.

Partially insecure entropy

Whereas you use secrets for some entropy, you use random in other cases, namely

random.shuffle(subset)
random.sample(password, len(password))

This is not secure and needs to be replaced with secrets calls.

• Kadragon: Redundant casts: I didn't know python handled strings that way. Thank you Latin: Thank you Redundant predicate: If a user requests 0 special characters it would return false. But then that doesn't generate the subset currently which needs to be fixed partially insecure: To my knowledge, there's no shuffle and sample available within secrets and I see no documentation of such methods.
• Reinderien: I misinterpreted what you're doing with minimums; I've edited my answer to indicate that it shouldn't exist at all.
• Reinderien: there's no shuffle and sample available within secrets - correct; you would need to implement equivalent behaviour from the primitives available in secrets.

Answer 3

Possible bug

It looks like if args.extended is False (the default) and args.special is set to zero (using -s 0), then subset won't get defined and the subsequent call random.shuffle(subset) would throw an exception.

clarity

What is the benefit of the list minimums? args.lower is clearer than minimums[0]. Better yet, the dest argument to parser.add_argument lets you specify the a more descriptive name of the variable, like so:

parser.add_argument("-lc", "--lower", dest="min_lowercase",
 type=int, default=1, 
 help="Minimum number of lowercase alpha characters")

• Kadragon: Possible Bug: Yes, that is an issue, thanks for pointing it out. Clarity: It was from my original approach, which I decided against. Then when changing over to the current itteration I didn't think about it.