6
\$\begingroup\$

I am a system admin, not a developer, so this might be pretty horrible code. This is a password generator. The point of the attempt was to become more familiar with Django at a lower level. I current host Django sites other developers write and sometimes it would help to have a better understanding of how the sites work at a lower level. I realize a password generator should run client side not server side but a highly secure, strong password generator was not my primary concern in writing this. It just seemed like an appropriately sized problem to jump into this with.

Any feedback on either Python or Django best practices would be appreciated. The logic of the password generation is not my primary concern however I would be willing to hear any thoughts on that as well.

Since I am not using a database or any models I am sure I will be told some other framework is probably more appropriate but this was just an iterative step in learning Django. My next project will contain a database.

views.py

from django.http import HttpResponse
from django.shortcuts import render, render_to_response
from django.views import generic

import datetime
import random
import string

from .forms import PasswordGeneratorForm


def generate_password(request):
  '''Generate a random password. Uses request POST data to allow user
     to set length and character set.'''

  pw_charset_lower = list(string.ascii_lowercase)
  pw_charset_upper = list(string.ascii_uppercase)
  pw_charset_digit = list(string.digits)
  pw_charset_special = list(string.punctuation)
  pw_charset_similar = ['o', 'O', '0', 'I', 'l', '1', '|']

  if request.method == 'POST':
    form = PasswordGeneratorForm(request.POST)

    if form.is_valid():
      pw_len = form.cleaned_data['length']

    if request.POST.get('avoid_similar', False):
      for x in pw_charset_similar:
          if x in pw_charset_lower: pw_charset_lower.remove(x)
          if x in pw_charset_upper: pw_charset_upper.remove(x)
          if x in pw_charset_digit: pw_charset_digit.remove(x)
          if x in pw_charset_special: pw_charset_special.remove(x)

    charset = []
    password = ""

    if request.POST.get('use_upper', False):
      charset = charset + pw_charset_upper
      password = password + random.choice(pw_charset_upper)
    if request.POST.get('use_lower', False):
      charset = charset + pw_charset_lower
      password = password + random.choice(pw_charset_lower)
    if request.POST.get('use_digits', False):
      charset = charset + pw_charset_digit
      password = password + random.choice(pw_charset_digit)
    if request.POST.get('use_special', False):
      charset = charset + pw_charset_special
      password = password + random.choice(pw_charset_special)

    while pw_len > len(password):
      password += random.choice(charset)

    pw_list = list(password)
    random.shuffle(pw_list)
    pw_string = ''.join(pw_list)

  else:
    form = PasswordGeneratorForm()
    pw_string = "Your password will show here"
    pw_len = 12

  return render(request, 'password/index.html', { 'password' : pw_string,
                                                  'length' : pw_len,
                                                  'form' : form})

forms.py

from django import forms

class PasswordGeneratorForm(forms.Form):
  length = forms.IntegerField(min_value=4, max_value=128)
  use_upper = forms.BooleanField(initial=True, required=False)
  use_lower = forms.BooleanField(initial=True, required=False)
  use_digits = forms.BooleanField(initial=True, required=False)
  use_special = forms.BooleanField(initial=True, required=False)
  avoid_similar = forms.BooleanField(initial=True, required=False)

index.html

{% load static %}

<link rel="stylesheet" type="text/css"
                       href="{% static 'password/style.css' %}" />

<html>

  <head>
    <title>Password Generator</title>
  </head>

  <body>
    <form action="{% url 'password:index' %}" method="post">
    {% csrf_token %}
      <br>
      <div id=password>
      <output name="password" size="{{ length }}">{{ password }}</output>
      </div>
      <br>
      <ul>
      {{ form.as_ul }}
      </ul>
      <br>
      <input type="submit" value="Generate New Password">
    </form>
  </body>

</html>
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Are you using 3.6? If so secrets. If not os.urandom or random.SystemRandom. The Python standard library doesn't joke around with warnings, there are reasons they're there. \$\endgroup\$
    – Peilonrayz ♦
    Commented Jan 12, 2017 at 9:18

2 Answers 2

Reset to default
6
\$\begingroup\$

Don't reinvent the wheel

Since you're handling a security feature, you shouldn't roll your own. As often with python, Django comes bateries included. And since they already need to generate random passwords (when prepopulating SECRET_KEY for instance), chances are they already have a more secure way of doing.

Indeed, django.utils.crypto.get_random_string() uses random.SystemRandom instead of random when available and try its best to avoid predictability when not. Chances are it will keep up with Python's security features in future releases.

Thus you just need to call get_random_string with the desired output length and a string of characters to choose from.

View

I said you should pass a string to get_random_string but any sequence of character would do. However, turning strings into lists here is really not needed. Strings already have all required features: they are iterables, indexables and have a len. Keep it simple.

You also handle forms a bit wrong: grabbing values from request.POST directly defeat the purpose of using forms in the first place and especialy form validation. Besides, what happens when the form is not valid (form.is_valid() returns False)? You'll get a NameError on pw_len.

From the documentation, a classic layout for form validation is as follow:

def generate_password(request):
    # if this is a POST request we need to process the form data
    if request.method == 'POST':
        # create a form instance and populate it with data from the request:
        form = PasswordGeneratorForm(request.POST)
        # check whether it's valid:
        if form.is_valid():
            # process the data in form.cleaned_data as required
            # ...
            # redirect to a new URL:
            return HttpResponseRedirect('/thanks/')

    # if a GET (or any other method) we'll create a blank form
    else:
        form = PasswordGeneratorForm()

    return render(request, 'password/index.html', {'form': form})

However, here you don't want to redirect to a success page, you want to stay on the same. We can't rely on early returns to stay DRY. An other common way is to use form = PasswordGeneratorForm(request.POST or None) without checking for request.method and rely on the fact that PasswordGeneratorForm(None).is_valid() will return False.

Style

From PEP8:

  • Order your imports: standard library, other system packages, personal modules;
  • Use 4 space per indentation level, and stay consistent: no 4 and then 2.
  • Never put code after a colon.
  • Extract your constant from your function, and name them all caps.

Proposed improvements

import string

from django.shortcuts import render
from django.utils.crypto import get_random_string 

from .forms import PasswordGeneratorForm


SIMILARS = {'o', 'O', '0', 'I', 'l', '1', '|'}


def generate_password(request):
    '''Generate a random password. Uses request POST data to allow user
    to set length and character set.
    '''

    form = PasswordGeneratorForm(request.POST or None)
    if not form.is_valid():
        context = {'password' : 'Your password will show here', 'length': 12, 'form': form}
    else:
        data = form.cleaned_data
        charset = ''
        if data['use_lower']:
            charset += string.ascii_lowercase
        if data['use_upper']:
            charset += string.ascii_uppercase
        if data['use_digits']:
            charset += string.digits
        if data['use_specials']:
            charset += string.punctuation

        if data['avoid_similar']:
            # keeping a list is fine here, get_random_string only need an indexable object
            charset = [c for c in charset if c not in SIMILARS]

        length = data['length']
        password = get_random_string(length, charset)
        context = {'password': password, 'length': length, 'form': form}

  return render(request, 'password/index.html', context)

We can also see that if no option is selected, the view will have a hard time terminating. It would be wise to add a custom validation step to your form to ensure that at least one option is selected.

\$\endgroup\$
7
  • \$\begingroup\$ I would probably make charset a set, at least because it already has set in the name. You would have to change the += string.xxx to |= set(string.xxx). \$\endgroup\$
    – Graipher
    Commented Jan 12, 2017 at 16:13
  • \$\begingroup\$ @Graipher I used to believe it too, and then I learnt. Also using sets does not add any value since every charset contains disjoint characters. The only benefit would be to turn the listcomp into a set difference... Meh... The join is not even necessary, as a list would be perfectly fine for get_random_string, I just added it to be consistent with the data types. \$\endgroup\$
    – 301_Moved_Permanently
    Commented Jan 12, 2017 at 17:13
  • \$\begingroup\$ True, it does not really matter as long as they are disjoint. If you mess up, though, and add a character class twice (for whatever reason), the set does not care but the string makes the password generator biased (which would be very hard to find). \$\endgroup\$
    – Graipher
    Commented Jan 12, 2017 at 17:20
  • \$\begingroup\$ Also, see my comment in that comment thread you linked to regarding making it a generator expression (I think one should do it, because str.join could get a better implementation than calling tuple on it one day) \$\endgroup\$
    – Graipher
    Commented Jan 12, 2017 at 17:22
  • \$\begingroup\$ @Graipher There, no more join ;) but I agree and will keep using genexpr in join calls now. \$\endgroup\$
    – 301_Moved_Permanently
    Commented Jan 12, 2017 at 17:54
3
\$\begingroup\$

The code in the post is insecure because it uses the built-in function random.choice to select the characters in the password. If you had read the documentation for the random module then you would have seen, in a big pink box,

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 problem is that, by default, the random module provides a pseudo-random number generator, using the Mersenne Twister algorithm, which is not designed to be secure, and an attacker who can capture enough output from the Mersenne Twister algorithm (for example, by sending many password generation requests to your web site) can reconstruct the internal state of the algorithm and so be able to perfectly predict the random numbers it will generate in future.

(Mathias Ettinger touched on this in his answer, and so did Peilonrayz in comments, but I think it's important to be completely explicit about this problem.)

\$\endgroup\$
1
  • \$\begingroup\$ To note secrets was added in 3.6, before that the warning was: "The pseudo-random generators of this module should not be used for security purposes. Use os.urandom() or SystemRandom if you require a cryptographically secure pseudo-random number generator." \$\endgroup\$
    – Peilonrayz ♦
    Commented Jan 13, 2017 at 12:25

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.