Copy&pasting code multiple times does not make it more secure. If we clean up your mutate_seed
method, we'd end up with this:
def mutate_seed(self):
for x in range(self.seed):
number_or_letter = r.randint(0, 1)
if number_or_letter:
self.mutated_seed.append(r.randint(0, 9))
else:
lower_or_higher = r.randint(0, 1)
if lower_or_higher:
self.mutated_seed.append(r.choice(self.alphabet_lower))
else:
self.mutated_seed.append(r.choice(self.alphabet_upper))
Quick Notes on the changes:
I don't request random values which I don't end up using. Throwing away values from the random number generator does not make those numbers that you use more secure.
I don't use single-letter variable names or abbreviations like con
or mut
that obscure the meaning.
random.choice
is so simple and nice – consider using it.
This solution features far less duplicate code. Did you calculate r.ranint(1, 6)
as an analogy for six-sided dice?
Now the code is cleaned up so that we can do some more substantial reviewing.
There is no use in separating between numbers, lowercase letters, and uppercase letters. They all should be in the same character pool so that each character has the same probability of occuring at each position in the password. With your current methods, a digit is much more likely to occur than any given letter, simply because there are less digits than letters.
So we should have something like
lower = [chr(x) for x in range(ord('a'), ord('z') + 1)] # I can't be bothered to type
upper = [chr(x) for x in range(ord('A'), ord('Z') + 1)] # out all letters manually
digits = [str(x) for x in range(0, 9)]
character_pool = lower + upper + digits
...
password = [random.choice(character_pool) for _ in range(0, length)]
The interface of your class is horrible. We have to create an instance, then call the mutate_seed
method, then access an instance variable, then must join that array to a string until we can finally display it. Consider packing all of this functionality into a single, convenient function instead:
def make_password(length, *collections_of_characters):
# join all character collections into a single set
characters = set()
for collection in collections_of_characters:
characters.update(str(c) for c in collection)
characters = list(characters)
password = [random.choice(characters) for _ in range(0, length)]
return "".join(password)
Then using this is as simple as
password = make_password(10,
"0123456789",
"abcdefghijklmnopqrstuvwxyz",
"ABCDEFGHIJKLMNOPQRSTUVWXYZ")
print(password)
If you need a certain set of characters more often, put those into a list:
alphanumeric_chars = ["0123456789",
"abcdefghijklmnopqrstuvwxyz",
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"]
password = make_password(10, *alphanumeric_chars)
print(password)
or create a wrapper function:
def make_alphanumeric_password(length):
return make_password(length,
"0123456789",
"abcdefghijklmnopqrstuvwxyz",
"ABCDEFGHIJKLMNOPQRSTUVWXYZ")
print(make_alphanumeric_password(10))
Not all abstractions map cleanly to classes.
Your code uses the word “seed” quite often. A seed is the initial state of a random number generator. When set to the same seed each time, the random number generator will produce the same sequence of numbers. In your class, “seed” seems to mean “length” instead. Use clear, obvious names for your variables rather than using technical lingo if you aren't sure what it means – although a quick trip to Wikipedia would have cleared that up.
You issue a few Windows-only system commands such as cls
. Unless this is central to the working of your application, don't manipulate the console, such as setting the color or clearing the screen. Users can do this themselves if they want to. Removing these commands also makes your code usable on other operating systems like OS X or Linux.
You are using the random
library as a random number generator. The numbers produced by this library aren't really random, they are just pseudo-random. This is usually not an issue as long as a really random seed is chosen, but because the number sequence is derived algorithmically, it should not be used for really really important stuff. Most passwords are only important and not really really important, so this should be fine ;-) but it's good to remember that real randomness is something different.
That is one aspect where the security of your program is not optimal, but good enough. Writing the password to a file is however absolutely irresponsible. When the password is only displayed on the command line, we can assume that it will only be immediately visible until the screen is cleared or other content has caused it to scroll out of sight. It will be held in memory for a while longer, but at least a rebooting of the system will remove any trace of this.
A plain text file however is much more persistent (it stays there after a reboot). It is also much easier for other users to access that password. Don't store secrets at all, or if you have to store them, secure them properly.
You should consider writing your Python code so that it is usable both as a module, and a script. For this, we first write all definitions that should be usable as a module, and at the end add an if __name__ == '__main__'
section, which will only be executed if the file was invoked as a script. Now we can both do:
# in some other program:
import make_password as mp
random_hex = mp.make_password(10, "0123456789abcdef")
or
python make_password.py
to use the functionality of your code. Designing for code reuse is a generally good idea.
random
implements, i imagine. \$\endgroup\$os.urandom()
orSystemRandom
if you require a cryptographically secure pseudo-random number generator." \$\endgroup\$