10
\$\begingroup\$

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))
     
    
\$\endgroup\$
3
  • 1
    \$\begingroup\$ As a small remark, the last part, where finalpassword is constructed, can be replaced by the random.shuffle method. \$\endgroup\$
    – Jan Kuiken
    May 16, 2023 at 16:10
  • 1
    \$\begingroup\$ If you want any extra inspiration, I made a password generator that makes "gibberish" passwords (i.e. the generated passwords tend to be pronounceable). It's not the best for code quality, which is the point of your post here, but it's not horrible and it might give you ideas for your own implementation. github.com/adholmgren/password_gen \$\endgroup\$ May 17, 2023 at 12:03
  • \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. \$\endgroup\$
    – pacmaninbw
    May 18, 2023 at 19:29

3 Answers 3

21
\$\begingroup\$

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.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ I fully agree with everything you wrote - and it looks like we both wrote out answers at the same time. In fact, both of our answers are correct, only tackling the problem from a different perspective. To truly help the OP out, I think he should first look at my - mostly generic concerns with his algorithm - then at your answer that addresses the Python specifics. \$\endgroup\$ May 16, 2023 at 8:39
8
\$\begingroup\$

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 NOT N + 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.

\$\endgroup\$
4
  • \$\begingroup\$ "Avoid ambiguous characters" – What the password generator I am using does in addition to that is to print letters, numbers, and symbols in three different colors. (But be aware of accessibility issues!!!) \$\endgroup\$ May 16, 2023 at 21:56
  • \$\begingroup\$ @JörgWMittag That's not a good solution for, for example, lI|! or oO0 (depending on the font) or even ,.;:´' and kK. I'm sure there are more situations in which these characters can be confused with others. \$\endgroup\$ May 16, 2023 at 23:44
  • \$\begingroup\$ This is good feedback, but I would also mention that even though extra constraints reduce the overall entropy there should still be plenty, at least in a decent sized password, and on top of that websites themselves force a certain amount of constraints -- the worst is when they put a maximum limit, as if a hasher can't handle more than 14 characters. There is the also xkcd password standpoint: more complex passwords can actually reduce entropy when users make obvious hacks or simplifications xkcd.com/936 \$\endgroup\$ May 17, 2023 at 12:12
  • \$\begingroup\$ @AndrewHolmgren Character enforcement has always reduced complexity. Forcing "Special character" just means !. Forcing a number just means 1. Anything weird, like combination of special character and numbers, patterns, or specific length, results in completely removing all security because it doesn't fit the pattern that I can memorize, so I write it down. \$\endgroup\$
    – Nelson
    May 18, 2023 at 3:08
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.