7
\$\begingroup\$

A program needs a password strength estimator but has several implementation restrictions:

  • Nothing outside of Python's standard library may be used.
  • A database of common passwords may not be consulted.
  • The code must be simple, self-contained, and easy to verify.

As for the interface of the password strength estimator, several requirements are provided:

  • Empty passwords must have a strength of zero.
  • Passwords may only use ASCII whitespace, alphanumeric, and punctuation characters.
  • Passwords with other characters must have a strength less than zero.
  • Otherwise, strength must be estimated as the bit-space needed to store the password.

In keeping with the caveats listed above, the following function appears to do as required:

import math
import string


def estimate_quality(password):
    if not password:
        return 0
    types = ([False, frozenset(string.whitespace)],
             [False, frozenset(string.ascii_lowercase)],
             [False, frozenset(string.ascii_uppercase)],
             [False, frozenset(string.digits)],
             [False, frozenset(string.punctuation)])
    for character in password:
        for index, (flag, group) in enumerate(types):
            if character in group:
                types[index][0] = True
                break
        else:
            return -1
    space = sum(len(group) for flag, group in types if flag)
    return math.ceil(math.log2(space) * len(password))

Is it possible to refactor the code to improve upon the third implementation restriction?

\$\endgroup\$
  • 1
    \$\begingroup\$ Otherwise, strength must be estimated as the bit-space needed to store the password. – You are not planning to store the password in plaint text, are you? \$\endgroup\$ – Richard Neumann Nov 22 '17 at 15:50
  • \$\begingroup\$ @RichardNeumann No, the password is used to generate an initialization vector and a cipher key and is never stored. Incremental decryption can be used to verify if the password was correct. \$\endgroup\$ – Noctis Skytower Nov 22 '17 at 15:56
  • 1
    \$\begingroup\$ " Incremental decryption can be used to verify if the password was correct." - No, no, no, no!!! You really, really need to feed the password into a password hashing function like PKBDF2, bcrypt, or scrypt. Your password database will leak at some point, and without an expensive hash function, attackers can just run the most popular hundred million passwords through the encryption algorithm and pick out everyones password - which they can then try on higher value sites like gmail/facebook/etc. \$\endgroup\$ – Martin Bonner Nov 22 '17 at 17:58
  • 1
    \$\begingroup\$ On point 1, yup, that was an incorrect assumption. On point 2, you must have something that is stored, and is unique to each user, otherwise there is no way to verify the user has supplied the correct password. I still worry that you are inventing a DIY scheme, rather than using a tried and tested scheme - but here is not the place to have that discussion. If you want to check your scheme, you could ask about it on security.stackexchange.com. \$\endgroup\$ – Martin Bonner Nov 22 '17 at 19:10
  • 1
    \$\begingroup\$ @MartinBonner That something is each individual message built by the user. It is a standalone GUI application that stores nothing but its settings on disk. Messages are transferred via the clipboard, and security on the OS is not a concern. To be honest, the program is a thought toy. Reference: Wabol Talk \$\endgroup\$ – Noctis Skytower Nov 22 '17 at 20:12
10
\$\begingroup\$
  • Your flags shouldn't be in the types definition.
  • types should be a constant, move it out of the function.
  • It'd be simpler if you used set.__isub__ and set.__and__, and looped through the types array.

    It leaves all the unwanted values in the password too. Allowing for possible improvements on point 3.

  • You can join the sum and the base for loop together, removing the need for a types list.

TYPES = (
    frozenset(string.whitespace),
    frozenset(string.ascii_lowercase),
    frozenset(string.ascii_uppercase),
    frozenset(string.digits),
    frozenset(string.punctuation)
)

def estimate_quality(password):
    if not password:
        return 0

    password_set = set(password)
    space = 0
    for group in TYPES:
        if password_set & group:
            space += len(group)
        password_set -= group

    if password_set:
        return -1

    return math.ceil(math.log2(space) * len(password))
\$\endgroup\$
4
\$\begingroup\$

I would split this in 2 extra methods: 1 checking for the allowed characters, the other to check whether the char_type is used in the password

import itertools
import math
import string

default_allowed_char_types = (
    frozenset(string.whitespace),
    frozenset(string.ascii_lowercase),
    frozenset(string.ascii_uppercase),
    frozenset(string.digits),
    frozenset(string.punctuation),
)

a tuple of frozensets to prevent mutation

def check_allowed_characters(password, allowed_types=default_allowed_char_types):
    allowed_chars = set(itertools.chain(*allowed_types))
    return all(ch in allowed_chars for ch in password)

def type_used_in_password(password, char_type):
    return char_type.isdisjoint(password)

def calculate_strength(password, allowed_types=default_allowed_char_types):
    if not password:
        return 0
    if not check_allowed_characters(password, allowed_types):
        return -1
    space = sum(len(char_type) for char_type in allowed_types if type_used_in_password(password, char_type=type))
    return math.ceil(math.log2(space) * len(password))

Each of those 2 subfunctions can be easily tested individually

\$\endgroup\$
  • 4
    \$\begingroup\$ Putting the one-liner char_type.isdisjoint(password) into an own function with the even longer name type_used_in_password(password, char_type) does not seem sensible to me. \$\endgroup\$ – Richard Neumann Nov 22 '17 at 15:46
  • \$\begingroup\$ @RichardNeumann At first I had a different implementation for that part. When improving that function I just didn't think about taking it out. Now you can decide to inline this or leave this abstraction to test this part separately \$\endgroup\$ – Maarten Fabré Nov 22 '17 at 15:53
  • \$\begingroup\$ If you follow my method, you can change check_allowed_characters to return not functools.reduce(operator.sub, TYPES, set(password)). Which IMO is a little easier to understand. \$\endgroup\$ – Peilonrayz Nov 22 '17 at 16:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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