Diceware Passphrase Generator

Question

I've written a simple Diceware passphrase generator based on this Stack Overflow question and the top answer.

Briefly, in the Diceware method, a die is rolled 5 times for each word in the passphrase and the results matched to the corresponding word in the word list which is of the form:

16226   cask
16231   casket
16232   cast
16233   caste
16234   cat
16235   catch

I'm still very much a Python and programming beginner and appreciate any feedback, but my main concerns are:

• Is the method I use to generate keys biased?
• Is my handling of command line arguments correct or could it be done better?
• Is the way I've broken my code down into individual functions reasonable or should certain functions be combined or broken apart?
• Are there any other glaring weaknesses?

import os
import sys

WORDS = "diceware_wordlist.txt"

def exists_arg():
    """Checks if argv input was given."""
    if len(sys.argv) == 1:
        return False
    else:
        return True

def test_arg(arg):
    """Checks if input is of the required form and returns
    various failure cases.
    """
    try:
        arg = int(arg)
    except ValueError:
        return 'Please enter an integer.'
    if arg < 5:
        return 'Length must be >= 5.'
    elif arg > 10:
        return 'Length must be <= 10'
    else:
        return 0, arg

def load_dict():
    """Returns a dictionary of the number to word mappings
    in the Diceware words file.
    """
    word_dict = {}
    with open(WORDS, 'r', 0) as in_file:
        for line in in_file:
            tmp = line.split()
            key = tmp[0]
            value = tmp[1]
            word_dict[key] = value
    return word_dict

def generate_key():
    """Takes 6 bytes one at a time from the OS random device,
    converts them to integers, adds them together, and takes 
    the modulo 6 value as a piece of a 5 digit key.

    Returns a key for a value in the Diceware dictionary.
    """
    key = ''
    for i in range(5):
        digit = 0
        for j in range(6):
            digit += ord(os.urandom(1))
        key += str(digit%6 + 1)
    return key

def generate_passphrase(length):
    """Returns a randomly chosen passphrase based on desired length."""
    word_dict = load_dict()
    passphrase = ''
    for i in range(1, length + 1):
        key = generate_key()
        passphrase += word_dict[key] + ' '
    return passphrase

def main():
    flag = True
    while True:
        if flag and exists_arg():
            arg = sys.argv[1]
            flag = False
        else:
            arg = raw_input('Length of desired passphrase: ')
        check = test_arg(arg)
        if check[0] == 0:
            print generate_passphrase(check[1])
            return
        else:
            print check

if __name__ == '__main__':
    main()

Answer 1

• Is my handling of command line arguments correct or could it be done better?

It would be better to not reinvent the wheel and use argparse instead.

• Is the way I've broken my code down into individual functions reasonable or should certain functions be combined or broken apart?

It's quite fine the way you did it. Every function seems to do just one thing, it's good like that.

• Are there any other glaring weaknesses?

Instead of this:

if len(sys.argv) == 1:
    return False
else:
    return True

Write like this:

return len(sys.argv) != 1

---

Instead of this:

with open(WORDS, 'r', 0) as in_file:

I'd recommend simply:

with open(WORDS) as in_file:

---

Instead of this:

for i in range(1, length + 1):
    key = generate_key()
    passphrase += word_dict[key] + ' '

This is simpler:

for _ in range(length):
    key = generate_key()
    passphrase += word_dict[key] + ' '

I replaced the loop variable i with _, because it's a common convention when the loop variable is not used.

---

Lastly, I recommend to switch to start using Python 3 instead of 2.7.

Answer 2

The die-rolling code:

for i in range(5):
    digit = 0
    for j in range(6):
        digit += ord(os.urandom(1))
    key += str(digit%6 + 1)

seems suboptimal to me. It seems needlessly complicated to generate an integer from 1 to 6 this way. You make 6 calls to os.urandom() (why 6 and not more or less?), approximating a normal distribution with peak at 765, then take the value mod 6. It wastes a lot of urandom's randomness -- you're using 48 bits of randomness for each die roll, you should only need 2.5 bits per die roll.

Answer 3

In response to @Snowbody's suggestions I changed my implementation of generate_key to use a bitstream via the bitstring module:

def generate_key():
    """Generate a key to select a word from WORDS. Uses random bytes from
    /dev/urandom to create a binary stream which is read to populate the key.

    Returns a key to select a word from WORDS.
    """
    bit_stream = ConstBitStream(bytes=urandom(2))
    key = ''
    while len(key) < 5:
        try:
            rand_uint = bit_stream.read('uint:3')
        except ReadError:
            bit_stream = ConstBitStream(bytes=urandom(1))
            rand_uint = bit_stream.read('uint:3')
        if rand_uint not in (0, 7):
            key += str(rand_uint)
    return key

This implementation calls urandom as little as possible and wastes the minimum in generated random bits. I'm not sure, however, if dropping 0's and 7's read from the bitstream is allowable from a cryptographic perspective.