0
\$\begingroup\$
  • The code itself is a random password generator which is encapsulated.
  • The other question about this code is whether it should be encapsulated on its main function. Or is it enough to exist in its own module with the main function?
def _generate(_what_to_generate_from : list[any]):
    """Generate random characters based on the list given

    Args:
        _what_to_generate_from (list[str]): The list of characters
    
    Raises:
        ValueError : If not the passed argument Is a list

    Returns:
        str : The random character's generated

    ----- This Functions Is not for prior use , It is only be used in generate_random_password function.
    """
    
    if not type(_what_to_generate_from) is list:
        raise ValueError(value_type_error_message.format('_what_to_generate_from'))

    _counter = 0
    _characters_list = []
    _characters_string = str()
    
    while _counter < OperationalVariables.input_desired_length:
        _first_random_number = randint(0,len(_what_to_generate_from) - 1)
        _second_random_number = randint(0, len(_what_to_generate_from) - 1)

        if _first_random_number == _second_random_number:
            _characters_list.append(_what_to_generate_from[_first_random_number])
            _counter += 1
        
    for _character in _characters_list:
        _characters_string = _characters_string + str(_character)
        
    return _characters_string
\$\endgroup\$
9
  • 3
    \$\begingroup\$ Just a note: that type hint any isn't correct. You should from typing import Any, as the lowercase version is actually the built-in function any. \$\endgroup\$
    – Linny
    Aug 30 at 3:16
  • 1
    \$\begingroup\$ What's with the proliferation of leading underscores? \$\endgroup\$
    – Reinderien
    Aug 30 at 3:16
  • \$\begingroup\$ @Reinderien We have no such thing called 'encapsulation' in python like there is in C# or... , But we do have a naming convention That is used to notify the code client or the own future self , That this function and all the component's inside it ( Variable , ETC ) are used for composition only ( Is-relationship reusage generally ) , And It should not be used in the global scope. \$\endgroup\$
    – KhodeNima
    Aug 30 at 3:33
  • 3
    \$\begingroup\$ @KhodeNima I think his gold python/python3.x badge earned over 3 years ago would say otherwise... \$\endgroup\$
    – Linny
    Aug 30 at 6:08
  • 4
    \$\begingroup\$ Leading underscores are used to name variables and attributes that are visible to the outside world but should be considered as "private." Examples would be certain global variables and class/instance attributes. Variables that are local to a function or method are not visible outside that function or method and therefore there is no need to suggest that they should be considered private (because they already are private). \$\endgroup\$
    – Booboo
    Aug 30 at 12:11

2 Answers 2

7
\$\begingroup\$

Security

The biggest red flag that I see is using random to generate passwords. This is warned against in the python docs on 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, which has examples for various usecases, such as token generation.

Now, why should we not use random? The answer is, with 638 characters (or 62 passwords, plaintext), an attacker can get the seed of random, and predict all passwords to come, what order they will appear, and the security of your application/program disappears. (A bit simplified, i agree, but you should never use random for security purposes) Also, if you are generating passwords and storing them in plaintext, that becomes a much more serious security vulnerability than the above. Please, follow the advice given at cwe.mitre.org

Bandit

If you are writing security-minded applications, or even want more security, I seriously recommend you use Bandit as a linter. Bandit can be run in vscode, as well as used as a linter by almost any modern IDE. Adding in the missing from random import randint, and running the code through py -m bandit code.txt

>> Issue: [B311:blacklist] Standard pseudo-random generators are not suitable for security/cryptographic purposes.
   Severity: Low   Confidence: High
   CWE: CWE-330 (https://cwe.mitre.org/data/definitions/330.html)
   More Info: https://bandit.readthedocs.io/en/1.7.5/blacklists/blacklist_calls.html#b311-random
   Location: bad.txt:25:31
24      while _counter < OperationalVariables.input_desired_length:
25          _first_random_number = randint(0,len(_what_to_generate_from) - 1)
26          _second_random_number = randint(0, len(_what_to_generate_from) - 1)

--------------------------------------------------
>> Issue: [B311:blacklist] Standard pseudo-random generators are not suitable for security/cryptographic purposes.
   Severity: Low   Confidence: High
   CWE: CWE-330 (https://cwe.mitre.org/data/definitions/330.html)
   More Info: https://bandit.readthedocs.io/en/1.7.5/blacklists/blacklist_calls.html#b311-random
   Location: bad.txt:26:32
25          _first_random_number = randint(0,len(_what_to_generate_from) - 1)
26          _second_random_number = randint(0, len(_what_to_generate_from) - 1)
27  

--------------------------------------------------

Code scanned:
    Total lines of code: 25
    Total lines skipped (#nosec): 0

Run metrics:
    Total issues (by severity):
        Undefined: 0
        Low: 2
        Medium: 0
        High: 0
    Total issues (by confidence):
        Undefined: 0
        Low: 0
        Medium: 0
        High: 2
Files skipped (0):

Perfomance

This is another topic of concern, because from reading the code below

_first_random_number = randint(0,len(_what_to_generate_from) - 1)
_second_random_number = randint(0, len(_what_to_generate_from) - 1)
    if _first_random_number == _second_random_number:

It becomes clear that you have an algorithmic complexity problem. What is that? Well, if you supply a big enough character list, the password generator can be used in a Denial-of-Service attack. Here's the gist: It you have a 5-long character list, your inner-loop has 1-in-25 chance of actually adding a character. This means it should take about 250 iterations to generate a password of length 10. If you have an 80 character list, it will take... 64,000 times. And if you have 128....163,840 times. This is in effect, DOS.

Taking advantage of secrets, the function becomes much simpler, and secure

import secrets
PASSWORD_LENGTH = 10

def _generate(_what_to_generate_from : list[str] | str):
    """Generate random characters based on the list given

    Args:
        _what_to_generate_from (list[str]): The list of characters
    
    Raises:
        ValueError : If the argument is not an iterable of strings

    Returns:
        str : The generated password.

    ----- This Functions Is not for prior use , It is only be used in generate_random_password function.
    """

    return ''.join(secrets.choice(_what_to_generate_from) for i in range(PASSWORD_LENGTH))
\$\endgroup\$
4
\$\begingroup\$

What you have posted is not complete since it is missing definitions and will not execute as is.

What is this function supposed to do?

The title of your post explains in slight detail what the function does but nowhere in the actual code is there a description. The name of the function, generate, is not descriptive: What does this function "generate." Surely a more descriptive name might be generate_password or generate_random_password.

Looking at your code, the main loop is:

    while _counter < OperationalVariables.input_desired_length:
        _first_random_number = randint(0,len(_what_to_generate_from) - 1)
        _second_random_number = randint(0, len(_what_to_generate_from) - 1)

        if _first_random_number == _second_random_number:
            _characters_list.append(_what_to_generate_from[_first_random_number])
            _counter += 1

What are you trying to accomplish by generating two indices and throwing away the second one? What is this code supposed accomplish? You may or may not end up with two successive characters of the password being the same. So what is the value in generating the second random number? This code is in great need of comments here.

Improved variable naming

As I mentioned in my comment to your post variables that are local to a function are not visible to code outside of the function and therefore there is no need to suggest that they should be considered "private" by naming them with leading underscores.

You should also use more descriptive variable names.

The function argument, what_to_generate_from, should be a string

First, a more descriptive name for this argument might be legal_characters.

Instead of having this argument be a list of single-character strings, it could more simply be just a single string. Otherwise, you should be checking that each element of the list really is a single-character string. The choice of making this argument a string also will result in more efficient code as we will see below.

Your code could be more efficient

You are generating a random integer and using that to index into your what_to_generate_from (i.e. legal_characters) list. Note that you could just as easily use this to index into the what_to_generate_from argument if it were a single multi-character string. Moreover, having this argument being a string allows us to more directly get the next password character by using the randint.choice function (see below).

If you are required by the function specification that this argument must be a list of strings, then I would suggest that it be converted into a single string to achieve the aforementioned efficiency with:

    what_to_generate_from = ''.join(what_to_generate_from)

By joining together the list elements into a single string also removes the limitation that each element be a single-character string.

You have an unnecessary counter variable whose value shadows the length of your characters_list list. I think it would be better for sake of conciseness and clarity to not use the counter variable at all.

Having generated a list of single-length characters that you want to append together to form the generated password, you have chosen to loop through the list performing string concatenation operations. A far more efficient method would be to use the built-in join method:

password = ''.join(characters_list)

Updated code with suggested changes

Since I cannot be sure what you were trying to accomplish by generating two random integers each time your loop, I have decided that in my updated version I would ensure that no two successive characters of the password are the same:

from random import choice

PASSWORD_LENGTH = 10

def generate_password(legal_characters : str):
    """Generate random characters based on the list given

    Args:
    legal_characters (str): The list of legal characters to be used

    Raises:
        ValueError : If the passed argument is not a str or its
                    length is less than 2

    Returns:
        str : The random character's generated

    ----- This Functions Is not for prior use , It is only be used in generate_random_password function.
    """

    if not type(legal_characters) is str:
        raise ValueError('Argument legal_characters must be a string')

    # To ensure that we do not have duplicate characters adjacent in the password:
    if len(legal_characters) < 2:
        raise ValueError('Argument legal_characters must be at least 2 characters long')

    password_chars = []

    while len(password_chars) < PASSWORD_LENGTH:
        password_char = choice(legal_characters)
        # Ensure that this is not a duplicate of the previously
        # generated character:
        if len(password_chars) == 0 or password_char != password_chars[-1]:
            password_chars.append(password_char)

    return ''.join(password_chars)

if __name__ == '__main__':
    print(generate_password('abcdef012345_'))

Prints:

03bdcfd1c5
\$\endgroup\$
0

Your Answer

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

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