3
\$\begingroup\$

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()
\$\endgroup\$

3 Answers 3

2
\$\begingroup\$
  • 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.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks! This is exactly what I was looking for. I'm pretty sure urandom is preferable to random, though. The docs for random suggest using urandom. \$\endgroup\$
    – meaot
    May 22, 2015 at 21:21
  • \$\begingroup\$ Hm, turns out, you're absolutely right about urandom vs random. Thanks for pointing out, I updated my post. \$\endgroup\$
    – janos
    May 22, 2015 at 21:58
2
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ String addition is concatenation — it won't result in a binomial distribution. \$\endgroup\$ May 23, 2015 at 17:11
  • \$\begingroup\$ digit = 0 then digit += ord(os.urandom(1)) is arithmetic adding, not string addition (concatenation). \$\endgroup\$
    – Snowbody
    May 23, 2015 at 17:21
  • \$\begingroup\$ You're right. It's doing a bit of both. \$\endgroup\$ May 23, 2015 at 17:25
  • \$\begingroup\$ @Snowbody I'm trying to avoid modulo bias. The inspiration for this method is this answer from the question I link to in my post. Taking ord(1 byte)%6 would be biased (as I understand it), since 256%6 != 0. The first multiple of 256 that is evenly divisible by 6 is 1536 so by taking the ord of 1 byte 6 times, adding them together, then taking the mod I can avoid the otherwise inherent bias. The reason I asked about it in my question is that I'm not sure my reasoning is logically sound. Is there a better way to generate values between 1 and 6? \$\endgroup\$
    – meaot
    May 24, 2015 at 21:12
  • \$\begingroup\$ So don't do ord(1 byte)%6 then. One way would be to create a bit buffer for the randomness and use 2 or 3 at a time as appropriate, five times. Or, realize you're really after a number from 1 to \$6^5=7776\$ and create it similarly, then convert to base-6. \$\endgroup\$
    – Snowbody
    May 26, 2015 at 2:47
0
\$\begingroup\$

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.

\$\endgroup\$

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.