4
\$\begingroup\$

This is a Python implementation of a stream cipher encryption algorithm. This implementation originates from the one displayed on the TI-Basic wikidot cryptography page.

I am well aware that this code breaks the holy 80 character limit, so any suggestion on how to make the code more succinct would be appreciated. My main concern, however, is the security of the random module; I know that it's stated in the documentation that the module is not cryptographically secure, so I would appreciate alternatives which provide the same functionality.


My primary concern is security, however, I am also concerned about readability.

import random

def Crypt(string,key,encrypt=1):
    """
    This program will input a message, a key, and the
    decision to encrypt or decrypt the message.
    It will then output the ciphertext created by running
    the message through a stream cipher encryption algorithm.
    What is a stream cipher encryption algorithm?:
    A stream cipher encryption algorithm shifts each letter
    in the message along the alphabet by a pseudorandom
    amount determined by the inputted key.
    """
    random.seed(key)
    # This must be *2 so a letter shifted beyond the end of the alphabet will loop back to the beginning
    alphabet = 2 * " AaBbCcDdEeFfGgHhIiJjKkLlMmNnOoPpQqRrSsTtUuVvWwXxYyZz1234567890.;:,'?/|{}[]-=+_!@#$%^&*()<>`~"
    # I declare this in a variable so the program can work with a variable length alphabet
    lenalpha = int(len(alphabet)/2)
    if encrypt:
        # This will shift each letter up the alphabet by a pseudo-random amount based on the key
        return ''.join([alphabet[alphabet.index(string[p]) + lenalpha - int(lenalpha * random.random())] for p in range(len(string))])
    else:
        # This will shift each letter down the alphabet by a pseudo-random amount based on the key
        return ''.join([alphabet[alphabet.index(string[p]) - lenalpha + int(lenalpha * random.random())] for p in range(len(string))])

if __name__ == '__main__':
    message = input("Input your message: ")
    key = input("Input a key: ")
    finished_with_the_user = False
    while not finished_with_the_user:
        encrypt_question = input("Encrypt or decrypt a message?(1,0): ")
        if encrypt_question.isdigit():
            finished_with_the_user = True
            encrypt_question = int(encrypt_question)
        else:
            print("Please input a valid number.")
    print(Crypt(message, key, encrypt_question))
    input("Press enter to exit.")

Any suggested edits and edits that I think of will be applied below.

import random
import string

def shift(current_position, distance, direction: (0, 1)):
    direction = 1 if direction else -1
    return current_position + direction * distance


def stream_cipher(message, key, do_encrypt=True):
    """
    This function will receive a message, a key, and the
    decision to encrypt or decrypt the message.
    The function uses a stream cipher encryption algorithm 
    which replaces each letter in the message with a 
    pseudo-random character from a given character set.
    Example:
    >>> stream_cipher("This is a test", 1234)
    'wPwV~#5;:D"905'
    >>> stream_cipher('wPwV~#5;:D"905', 1234, False)
    'This is a test'
    """
    random.seed(key)
    # The character set must be multiplied by 2 so
    # a character shifted beyond the end of the 
    # character set will loop back to the beginning.
    characters = 2 * (
                      string.ascii_letters +
                      string.digits +
                      string.punctuation + ' '
                     )
    # I declare this in a variable so the 
    # program can work with a variable length character set
    lenchars = len(characters)//2
    # This will replace each character in the message
    # with a pseudo-random character selected from
    # the character set.
    return ''.join([characters[shift(characters.index(message[each_char]), lenchars - int(lenchars * random.random()), do_encrypt)] for each_char in range(len(message))])


def main():
    message = input("Input a message: ")
    key = input("Input a key: ")
    while True:
        do_encrypt = input("Encrypt or decrypt the message? (1,0): ")
        if do_encrypt in ('1', '0'):
            break
        print("Please input a valid number,\n"
              "either 0 for decryption or 1 for encryption.")
    print(encrypt(message, key, do_encrypt == '1'))
    input("Press enter to exit.")

if __name__ == '__main__':
    main()
\$\endgroup\$
5
\$\begingroup\$

I'm focusing on readability for my answer. Reading Python's style guide, PEP0008, will be a great help. But I'll highlight specific areas in your code here.

Function names should always be lowercase. Classes should use PascalCase, so your Crypt will look like it's a class. Comma separated sequences in Python should have spaces after each comma, just like they would when writing an english sentence. It's more readable.

def crypt(string, key, encrypt=1):

encrypt shouldn't be an int. It should be a boolean. I know that you get user input for it, but just pass a check to see if the user entered 1 or not.

crypt(message, key, encrypt_question == '1')

You can get your alphabet string programmatically instead of typing all the characters. The string module can be imported and will provide you with ways to get the uppercase and lowercase letters, digits and punctuation marks of a user's locale. This also makes your script more compatible with different languages. (Note: that if you use this you'll need to have a different name for the string parameter, I use often use s but single letter names are debateable)

alphabet = 2 * (string.letters + string.digits + string.punctuation + ' ')

On that note, alphabet is a bad name. You're including a lot more than the alphabet but the name makes it sound like numerical characters and punctuation are unsupported. characters is better, or chars.

You can use Python's floor division operator to avoid getting a float. It's just an extra slash and will make Python round down to an int. While I'm at it, add space on either side of operators like this. It's more readable.

lenalpha = len(alphabet) // 2

Don't repeat the comments here, they're practically identical anyway. Just make it less specific and put one before the if.

# Shift each letter along char by a pseudo-random amount based on the key
if encrypt:

Instead of while finished_with_the_user, just use while True and then break when the input is valid. Again about naming, is_encrypt, do_encrypt or maybe even encrypt would be better. The question part isn't helpful to know what the variable is for, it just tells you where the value came from, but we don't care about that. I also recommend making it clearer to the user what a valid input is if they provide a wrong one.

while True:
    encrypt_question = input("Encrypt or decrypt a message? (1,0): ")
    if encrypt_question in ('0', '1'):
        break
    print("Please input a valid number, either 0 for decrypt or 1 for encrypt.")
\$\endgroup\$
  • \$\begingroup\$ As the code is now, input() will always return a string which always tests as True except when it is empty (This would cause a bug later on). So if I wanted to make it into a bool (Without using an if statement) I'd have to change the input to an int, and then to a bool, which is useless if I know it's 0 or 1 since Python already treats 0 and 1 as False and True values, respectively. \$\endgroup\$ – Zenohm Sep 7 '15 at 19:15
  • \$\begingroup\$ I was suggesting passing it as encrypt_question == '1', which will just evaluate to a boolean. The trouble with int is that you generate errors if the user enters anything other than a simple integer. You had a test to prevent that, but just extra code since you're already coercing a string to an int then to a boolean, you may as well go straight to boolean. \$\endgroup\$ – SuperBiasedMan Sep 7 '15 at 19:22
  • \$\begingroup\$ I see what you mean with passing encrypt_question == '1', but no errors will be generated in either script because the input must be either 0, 1 (In your script) or 0, Any integer (In my script). Any string passed to the encrypt_question variable will evaluate as True as long as the string is not empty and any non-zero number will evaluate as True. \$\endgroup\$ – Zenohm Sep 7 '15 at 19:35
3
\$\begingroup\$

I find your documentation ambiguous and not clear:

This program will input a message, a key, and the decision to encrypt or decrypt the message.

Reading this, it looks like the program will supply x,y,z as input to something else (another program? a website?), careful reading shows that it receives these values as input.

There is no message in the method arguments: Crypt(string,key,encrypt=1), when describing what the arguments do, use the same name in code and docs.

You say program, but this is inside the docstring of a function. Again, precise terminology in tecnical documentation is not optional.

You say decision, I take that to mean Boolean that is True or False, but the default value for this parameter is 1, and considering 1 and 0 as boolean values is confusing and has been obsolete for at least 16 years, if not more. (C99)

It will then output the ciphertext created by running the message through a stream cipher encryption algorithm.

This whole phrase is not needed, as all the information in it could be conveyed more succinctly and with less noise by naming the function stream_cipher

What is a stream cipher encryption algorithm?:

This phrase adds no value to the documentation and should be removed, the reader wants to obtain information, not to play guess and answer arbitrary questions that he is not supposed to know.

A stream cipher encryption algorithm shifts each letter in the message along the alphabet by a pseudorandom amount determined by the inputted key.

This is vague: what do you mean by determined? Reading the code tells me that the key is used as the seed of the random number generator, and I would have stated so in the documentation.

...

There are no tests, nor examples of usage in your documentation. While your attempt to provide documentation is admirable, many people find it easier to understand what a piece of code does by example, so I suggest adding an example call, along with expected output. This will also enable automatic testing for simpler optimizations and simplifications.

\$\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.