Python \ random password generation code

Question

I have written a python program for random password generation and as a newbie I would like my code to be reviewed by professionals and if they have any suggestions to improve it.

import string import random letters = string.ascii_letters digits = string.digits special_chars = string.punctuation letters_list = list(letters) digits_list = list(digits) special_list = list(special_chars) many_letter = int(input(f"how many letter ? \n")) many_digit = int(input(f"how many digits ? \n")) many_special = int(input(f"how many special chars ? \n")) #................................................... password = [] finalpassword = [] counter_l = 0 for l in letters_list : password.append(random.choice(letters_list)) counter_l += 1 if counter_l == many_letter : break counter_d = 0 for d in digits_list : password.append(random.choice(digits_list)) counter_d += 1 if counter_d == many_digit : break counter_s = 0 for s in special_list : password.append(random.choice(special_list)) counter_s += 1 if counter_s == many_special : break p_counter = 0 for p in password : finalpassword.append(random.choice(password)) p_counter += 1 if p_counter == len(password) : break print("".join(finalpassword)) 

Answer 1

Don't use random for security

Quoting from the documentation for random:

Warning: The pseudo-random generators of this module should not be used for security purposes. For security or cryptographic uses, see the secrets module.

The pseudo-random number generator implemented by random doesn't provide a truly random output and provides a predictable output, and as such isn't suited for password generation.

The secrets module works similarly to random, but is cryptographically secure.

Bugs

There are multiple bugs in your code caused by improper logic.

Wrong number of characters

I tried generating a password with 1 letter, 5 digits and 1 special character and got 2$32$23: no letter and 2 special characters. Another try gave 54j5614: no special character and 6 digits.

This happens because you first generate an alphabet with the expected number of each character type, then generate the final password by sampling repeatedly that alphabet, instead of generating the final password directly.

Improper loop handling

I tried generating a password with 0 letter, 1 digit and 1 special char and got: tNpoQQ7XQtxcortvERSypXphvNNcWxryttrpfpMyhhfoXpBoBcBBxP. Besides another occurrence of the previous bug (no special char), there are obviously too many letters.

Then I tried 1 letter, 1000 digits and 1 special char and got: |87b07bb17|9. Now, that's way too few digits, or characters overall. Besides the first bug, I expected a 1002-character-long password.

This happens because of the way you handle loops, mixing for loops and manually handling a loop counter.

The first bug is caused because you increment the counter before checking its value, meaning that by the time you check if counter_l == many_letter, counter_l is greater than 0.

The second bug comes from the fact that when you loop over a collection (for d in digits_list:), the code will break out of the loop before the counter reaches the specified amount.

The proper way to execute a loop exactly n times is:

for _ in range(n): # do stuff 

Shorter code, no counter needed. The _ in the above snippet is a valid variable name, and could be anything else, but is usually used in Python to indicate a throwaway variable, one which you don't use the value.

Best practices

Read the documentation for secrets, you will find example code for password generation in the "Recipes and best practices" section.

Try to understand how these simple examples work and why this approach works best, and to adapt them to your specifications.

Other remarks

Index directly into strings

There is no need to convert strings to list to be able to index individual characters. In Python, you can directly index into strings:

>>> 'abcd'[2] 'c' 

Unnecessary f-strings

You use f-strings for the input prompts, but don't use string interpolation. These should be simplified to regular strings.

Answer 2

As a general principle, humans are very bad at generating "random" sequences. And that's where automated password generators come in place.

Unfortunately, your algorithm has some very serious flaws.

First, you should look at some of the existing tools out there and the kinds of options they provide. It's generally something amongst the lines of:

• Length of password
• Character classes (not all sites allow all kinds of special characters)
• Minimum complexity
• Avoid ambiguous characters (for instance, the number 1 and the lower-case letter l are often hard to distinguish).

The above options are frequently found in many different password generators.

---

Do not under any circumstances add any custom rules on top of those unless you clearly understand what you are doing!

Since you're a new contributor, you made a genuine error and may not even realize so. Because it's not really that obvious to see without having a background in cryptology or sufficient experience.

So let me explain ...

---

What your code does is that it first generates N characters, then M digits, then K special characters.

The only good news is that you didn't just concatenate them together, but attempted to add a bit of "randomness" to it - or at least, so you thought ...

The problem is that you let the user freely choose all of N, M and K! This is a serious problem because humans are very bad at coming up with "truly random" sequences.

The "true randomness" of this will be somewhere in the range of min (N, M, K) + 3 - and NOTN + M + K as one would imagine.

In addition to this, you introduced an "accident waiting to happen" - because removing your finalpassword computation would very seriously undermine your algorithm's strength - and there isn't even a code-comment saying that this code must not under any circumstances be removed.

---

Let the user pick the total length N and the acceptable character set C, then do one single loop.

---

I think the most important thing for you to learn is that you don't necessarily "gain" any "randomness" by adding constraints - but could in fact lose quite a lot of it.

Answer 3

About Imports

Don't import a module and assign a variable name to the attribute of the module.

Don't do this:

import string letters = string.letters 

Instead import the attribute from the module, and if you need to change the name, use as to alias it, like this:

from string import ascii_letters as letters, digits, punctuation as special_chars 

Naming Variables

Don't name variables like many_letter, many_digit, many_special, variable names should be concise and expressive, you should know what the variable is for by looking at its name.

Instead use these: num_letters, num_digits, num_special_chars

Typecasting str to list

Don't convert str to list like this: letters_list = list(letters), it is absolutely unnecessary and a waste of execution time. str is already a sequence type, it is an ordered container containing the characters, 'abc' contains the same data as ['a', 'b', 'c'], they just have different methods associated with them, but all operations that can be performed on sequences can be performed on strs.

random supports choosing from strs, just use strs directly in your case.

About Functions

Don't use input to get variables: many_letter = int(input(f"how many letter ? \n"))

It is inefficient and not reusable, you should put your code into functions to increase reusability, like so:

def generate_password(num_letters, num_digits, num_special_chars): pass 

Password Generation

And this is where your code falls flat. Your code is absolutely NOT working as intended, it is objectively non-functional.

The first thing that comes to mind, almost all websites require passwords to contain both uppercase and lowercase letters when registering accounts, I have never found one that is an exception. And your code doesn't guarantee the result will contain both uppercase and lowercase.

Your code doesn't work properly, as other answers have already pointed out, and I won't reiterate.

And you shouldn't allow the user to choose what number of each type of characters a password can have, this is insecure, instead you should randomly choose the number of each category of characters, and only let the user set the length of the characters. And you should set a minimum length requirement, most websites require at least 8 characters long passwords.

Your for loop can be simplified to something like this:

chars = [] for _ in range(number): chars.append(random.choice(charset)) 

Then it could also be simplified further to this (list comprehension):

chars = [random.choice(charset) for _ in range(number)] 

Which is equivalent to this:

chars = random.choices(charset, k=number) 

The latter method is much more efficient than the former, but unfortunately as others pointed out, random is not cryptographically secure, the secrets standard library, while cryptographically secure, doesn't have a choices method.

And you should also shuffle the password to increase entropy.

---

Fixed code

import random import secrets from functools import reduce from operator import iconcat from string import ascii_lowercase, ascii_uppercase, digits, punctuation from typing import Generator, List CHARSETS = (ascii_lowercase, ascii_uppercase, punctuation, digits) def choose_charset(charset: str, number: int, unique: bool = False) -> List[str]: if not unique: return [secrets.choice(charset) for _ in range(number)] charset = list(charset) choices = [] for _ in range(number): choice = secrets.choice(charset) choices.append(choice) charset.remove(choice) return choices def split(number: int) -> Generator[int, None, None]: for a, b in ((4, 68), (3, 42), (2, 10)): n = max( 2 + secrets.randbelow( number // a - 1 ), number - b ) number -= n yield n if number > 10: raise ValueError('remaining number is too big') yield number def generate_password(length: int, unique: bool = False) -> str: if not isinstance(length, int) or not 8 <= length <= 94: raise ValueError('argument `length` should be an `int` between 8 and 94') password = reduce(iconcat, map(choose_charset, CHARSETS, split(length), [unique]*4), []) random.shuffle(password) return ''.join(password) if __name__ == '__main__': from itertools import product for i, _ in product(range(8, 16), range(3)): print(generate_password(i, True)) 

Output:

0?rMdW2. ^kXQ<40x 9t0KL:[i J58vRp7_[ {MA7o5e!9 b$2WC6h}3 `45UvdZ;08 91W"8,I!ca .1f2<zUT7( 42-g0y?9UP1 {>NoBk8406$ 3$kZ6_1S2Ez E846_1dA7e&3 t3@5wCu04}~N 0w9834^I&1Ok K43x~82`I10wh Cq7\Q1^d`390v i;085m<FNa42: E)$-2z1KA0L(y7 :9[`S8b7;t16J} 4[,W0Dhl3I75%1 36ejCJ5b20E|8_; jR17C8H&N,925vl 5Q8LK$f\%uy3;46 

---

Update

I have modified the code so that it now can generate passwords containing only unique characters, and I have limited the number of each type of characters the password can have, so that the number of characters per type the password can have is approximately the same.

Because I have added the functionality to generate only passwords containing unique characters, the length of passwords is limited to 94 characters. Because there are only 94 symbols in total, a password containing more than 94 characters is guaranteed to contain duplicate characters.