13
\$\begingroup\$

I have just created an encryption program with a friend. I wanted to know if anyone here could give me back any feedback on it.

Here is how the program works:

Imagine you want to encrypt the word 'hi'.

Depending on the key generated with the program, the computer will choose between 2 strings of two letters randomly for every letter that needs to be encrypted. The strings are embedded in an encryption/decryption key.

Example: 'h' when encrypted will be either 'tn' or 'io', and 'i' when encrypted will be either 'ac' or 'vu'.

So when 'hi' will be encrypted, it can be:

'tnac'; 'tnvu'; 'ioac'; 'iovu'

It is chosen randomly by the computer, so it is impossible to predict what outcome will be.

#######################################################################
#                                                                     #
#                  AGENCRYPTION program V.1.1                         #
#            Made by Elio Hayman and Valentino Magniette              #
#                           Have fun                                  #
#                                                                     #
#######################################################################

version = 'Version 1.1'
from random import *
import tkinter as tk

current_key = ''

account = ''
in_debug = False
def debug(text):
    if in_debug:
        print('[Debug]', text) 

key = ''
def decrypt(crypted):
    debug(crypted)
    global current_key
    key = current_key
    if key == '':
        print('[Error] Please insert a key before utilisation. To do so, do readkey <yourKey>.')
        return
    basemsg = ''
    key_list = []
    while not key == '':
        key_list.append(key[:5])
        key = key[5:]
    new_key = []
    for i in key_list:
        new_key_thing = []
        new_key_thing.append(i[0])
        new_key_thing.append(i[1:3])
        new_key_thing.append(i[3:])
        new_key.append(new_key_thing)
    key = new_key
    searchlength = 1
    msgleft = crypted
    found = False
    while len(msgleft) > 0:
        searchlength = 0
        found = False
        while found == False:
            if searchlength == 0 and len(msgleft) > 0:
                for x in key:
                    if found == False:
                        if msgleft[0] in x[1:]:
                            basemsg = basemsg + x[0]
                            msgleft = msgleft[1:]
                            found = True
            elif searchlength > 0 and len(msgleft) > 0:
                for x in key:
                    if found == False:
                        if msgleft[:searchlength-1] in x[1:]:
                            basemsg = basemsg + x[0]
                            msgleft = msgleft[searchlength-1:]
                            found = True
            searchlength += 1
    basemsg = basemsg.replace('^', ' ')
    return basemsg
def encrypt(message):
    global current_key
    key = current_key
    if key == '':
        print('[Error] Please insert a key before utilisation. To do so, do readkey <yourKey>.')
        return
    message = message.replace(' ', '^')
    endmsg = ''
    key_list = []
    while not key == '':
        key_list.append(key[:5])
        key = key[5:]
    new_key = []
    for i in key_list:
        new_key_thing = []
        new_key_thing.append(i[0])
        new_key_thing.append(i[1:3])
        new_key_thing.append(i[3:])
        new_key.append(new_key_thing)
    for char in message:
        for x in new_key:
            if x[0] == char:
                endmsg = endmsg + x[randint(1, len(x)-1)]
                break
    return endmsg

def readkey(input_key):
    all_chars = 'aabcdefghijklmnopqrstuvwxyz1234567890&²é~"#\'{([-|è`_\\çà@)]=}°+.+-*/,?;:!§ù%*µ$£¤^¨ABCDEFGHIJKLMNOPQRSTUVWXYZ'
    key_list = []
    if len(input_key) == 779:
        print('Key loading...')
        parts = list()
        for i in range(109):
            key_part = input_key[:5]
            letter = key_part[0]
            part1 = key_part[1:3]
            part2 = key_part[3:5]
            list_thing = list()
            list_thing.append(letter)
            list_thing.append(part1)
            list_thing.append(part2)
            parts.append(input_key[:5])
            input_key = input_key[5:]

        print('Key inserted')
        print('Caution, this key will only stay in the program as long as it is running or when replaced by another one')
    else:
        print('[Error] This key is unsupported by the program. Make sure it is correct and that you are using the latest version.')
        return None
    return ''.join(parts)
def genkey():
    all_chars = 'aabcdefghijklmnopqrstuvwxyz1234567890&²é~"#\'{([-|è`_\\ç^à@)]=}°+.+-*/,?;:!§ù%*µ$£¤^¨ABCDEFGHIJKLMNOPQRSTUVWXYZ'
    char_list = list('aabcdefghijklmnopqrstuvwxyz1234567890&²é~"#\'{([-|è`_\\ç^à@)]=}°+.+-*/,?;:!§ù%*µ$£¤^¨ABCDEFGHIJKLMNOPQRSTUVWXYZ')
    shuffle(char_list)
    all_chars = ''.join(char_list)
    key = list()
    security = 2
    for x in range(len(all_chars)*security):
        valid = False
        while valid == False:
            char1 = all_chars[randint(0,35)]
            char2 = all_chars[randint(0,35)]
            if not (char1 + char2) in key:
                key.append(char1 + char2)
                valid = True
    speshul_chars_letters = list()
    for i in range(117):
        valid = False
        while valid == False:
            char1 = all_chars[randint(0,35)]
            char2 = all_chars[randint(0,35)]
            if not (char1 + char2) in key or (char1 + char2) in speshul_chars_letters:
                speshul_chars_letters.append(char1 + char2)
                valid = True
    key_list = []
    for i in all_chars:
        chars = [i]
        for xx in range(security):
            chars.append(key[0])
            del key[0]
        key_list.append(chars)
    key_text = ''
    key_text_list = []
    for y in key_list:
        key_text_list.append(''.join(y))
    speshul_chars_letters_text = ''.join(speshul_chars_letters)
    return ''.join(key_text_list) + speshul_chars_letters_text

import tkinter as tk
from time import *
mw = tk.Tk()

mw.title('AGE v1.1')
mw.geometry("800x500")
mw.resizable(0, 0)

back = tk.Frame(master=mw,bg='#24b1db')
back.pack_propagate(0)
back.pack(fill=tk.BOTH, expand=1)

mode = 0

def purgescreen():
    if mode == 1:
        encryption_text_input.destroy()
        encryption_text_output.destroy()
        encrypt_main.destroy()
    elif mode == 2:
        decryption_text_input.destroy()
        decryption_text_output.destroy()
        decrypt_main.destroy()
    elif mode == 3:
        keygen_text_output.destroy()
        keygen_main.destroy()
        directapply_main.destroy()
    elif mode == 4:
        keyread_text_input.destroy()
        keyread_main.destroy()
    elif mode == 5:
        info_main.destroy()
        info_copyright.destroy()
        info_website.destroy()
        info_terms.destroy()
        info_version.destroy()
    elif mode == 6:
        help_main.destroy()
    elif mode == 7:
        welcome_main.destroy()

def encrypt_shortcut():
    message = encryption_text_input.get("1.0",'end-1c')
    encrypted = encrypt(message)
    encryption_text_output.delete("1.0","end")
    encryption_text_input.delete("1.0","end")
    encryption_text_output.insert("1.0",encrypted)

def encryption_button():
    global encryption_text_input
    global encryption_text_output
    global encrypt_main
    global mode
    purgescreen()
    mode = 1
    encryption_text_input = tk.Text(back, height=10, width=70)
    encryption_text_input.place(relx=.25 , rely=0)

    encryption_text_output = tk.Text(back, height=10, width=70)
    encryption_text_output.place(relx=.25, rely=.6)

    encrypt_main = tk.Button(master=back, text='Encrypt', command=encrypt_shortcut)
    encrypt_main.config(height=3, width=15)
    encrypt_main.place(relx=.5, rely=.4)

def decrypt_shortcut():
    message = decryption_text_input.get("1.0",'end-1c')
    decrypted = decrypt(message)
    decryption_text_output.delete("1.0","end")
    decryption_text_input.delete("1.0","end")
    decryption_text_output.insert("1.0",decrypted)

def decryption_button():
    global decryption_text_input
    global decryption_text_output
    global decrypt_main
    global mode
    purgescreen()
    mode = 2
    decryption_text_input = tk.Text(back, height=10, width=70)
    decryption_text_input.place(relx=.25 , rely=0)

    decryption_text_output = tk.Text(back, height=10, width=70)
    decryption_text_output.place(relx=.25, rely=.6)

    decrypt_main = tk.Button(master=back, text='Decrypt', command=decrypt_shortcut)
    decrypt_main.config(height=3, width=15)
    decrypt_main.place(relx=.5, rely=.4)

def keygen_shortcut():
    key = genkey()
    key = ''.join(key)
    keygen_text_output.delete("1.0",'end')
    keygen_text_output.insert("1.0",key)

def apply_shortcut():
    key = keygen_text_output.get("1.0","end-1c")
    global current_key
    current_key = key

def keygen_button():
    global keygen_text_output
    global keygen_main
    global mode
    global directapply_main
    purgescreen()
    mode = 3
    keygen_text_output = tk.Text(back, height=15, width=70)
    keygen_text_output.place(relx=.25 , rely=.4)

    keygen_main = tk.Button(master=back, text='Generate Key', command=keygen_shortcut)
    keygen_main.config(height=3, width=15)
    keygen_main.place(relx=.4, rely=.2)

    directapply_main = tk.Button(master=back, text='Apply Key', command=apply_shortcut)
    directapply_main.config(height=3, width=15)
    directapply_main.place(relx=.6, rely=.2)

def keyread_shortcut():
    key = keyread_text_input.get("1.0","end-1c")
    keyread_text_input.delete("1.0","end")
    global current_key
    current_key = key

def keyread_button():
    global keyread_text_input
    global keyread_main
    global mode
    purgescreen()
    mode = 4

    keyread_text_input = tk.Text(back, height=15, width=70)
    keyread_text_input.place(relx=.25, rely=.1)

    keyread_main = tk.Button(master=back, text='Insert Key', command=keyread_shortcut)
    keyread_main.config(height=3, width=15)
    keyread_main.place(relx=.5, rely=.7)

def info_button():
    global info_main
    global info_copyright
    global info_website
    global info_version
    global info_terms
    global mode
    purgescreen()
    mode = 5

    info_main = tk.Label(master=back, text='This program is made by \n Valentino Magniette and iWaroz.', bg='#24b1db')
    info_main.config(height=3, width=70)
    info_main.place(relx=.25, rely=.1)

    info_copyright = tk.Label(master=back, text='Copying this program or resseling it \n without permission from its creators is forbidden.', bg='#24b1db')
    info_copyright.config(height=3, width=70)
    info_copyright.place(relx=.25, rely=.3)

    info_terms = tk.Label(master=back, text='AGencryption and its creators are not responsible for any legal problems regarding \n  encrypting sensible documentation that the authorities want decrypted. ', bg='#24b1db')
    info_terms.config(height=3, width=70)
    info_terms.place(relx=.25, rely=.5)

    info_website = tk.Label(master=back, text='You can get the program for free at: http://realtasting.com/AGE-Version1.exe', bg='#24b1db')
    info_website.config(height=3, width=70)
    info_website.place(relx=.25, rely=.7)

    info_version = tk.Label(master=back, text='Version 1.1', bg='#24b1db')
    info_version.config(height=3, width=70)
    info_version.place(relx=.25, rely=.9)

def help_button():
    global help_main
    global mode
    purgescreen()
    mode = 6

    help_main = tk.Label(master=back, text='If any help is needed. \n Go to our discord with the link \n https://discord.gg/YVDBudA', bg='#24b1db')
    help_main.config(height=3, width=50)
    help_main.place(relx=.35, rely=.5)

global welcome_main
purgescreen()
mode = 7

welcome_main = tk.Label(master=back, text='Welcome to AGE \n This is a program which is used for encryption \n You can encrypt an unlimited ammount of text securely for free \n To start, you will need an encryption/decryption key for it to work \n Have fun!!', bg='#24b1db')
welcome_main.config(height=10, width=50)
welcome_main.place(relx=.35, rely=.35)

encryption_main = tk.Button(master=back, text='Encryption', command=encryption_button)
encryption_main.config(height=4, width=20)
encryption_main.place(relx=.095, rely=.07, anchor="c")

decryption_main = tk.Button(master=back, text='Decryption', command=decryption_button)
decryption_main.config(height=4, width=20)
decryption_main.place(relx=.095, rely=.21, anchor="c")

generator_main = tk.Button(master=back, text='Key Generator', command=keygen_button)
generator_main.config(height=4, width=20)
generator_main.place(relx=.095, rely=.35, anchor="c")

reader_main = tk.Button(master=back, text='Key reader', command=keyread_button)
reader_main.config(height=4, width=20)
reader_main.place(relx=.095, rely=.49, anchor="c")

information_main = tk.Button(master=back, text='Information', command=info_button)
information_main.config(height=4, width=20)
information_main.place(relx=.095, rely=.63, anchor="c")

help_main = tk.Button(master=back, text='Help', command=help_button)
help_main.config(height=4, width=20)
help_main.place(relx=.095, rely=.77, anchor="c")

quit_main = tk.Button(master=back, text='Quit', command=mw.destroy)
quit_main.config(height=4, width=20)
quit_main.place(relx=.095, rely=.91, anchor="c")

mw.mainloop()
```
\$\endgroup\$
  • 11
    \$\begingroup\$ The encryption is not secure; it could be defeated using frequency analysis. But I'm guessing you knew it might not be secure and did it for fun anyway. \$\endgroup\$ – user253751 Nov 10 '19 at 1:38
  • \$\begingroup\$ Here is how the program works describing this is not half bad. If you added the purpose of this code (may be as mundane as completing a practice programming project), it may be easier to give advice (such as putting information in the source code makes it less likely that code snippets float around lacking that information). \$\endgroup\$ – greybeard Nov 10 '19 at 8:10
18
\$\begingroup\$

First, congratulations! Being able to produce something usable with another person is an incredibly important skill, and especially in programming. Please don't worry about the amount of suggestions below – I'm treating this like any other Python review.

Specific suggestions

  1. Module documentation, like the lines at the top of your file, is usually written within triple double quotes, like this:

    """
    AGENCRYPTION […]
    """
    

    You can also use this to write multi-line strings without having to embed \n escape characters within them.

  2. It would be handy to be able to set the debugging flag from the command line rather than having to modify the code. You can do that by using either argparse to add a --debug flag or by checking whether os.environ["DEBUG"] is not empty.
  3. from [something] import * is a handy shortcut, but it makes the code harder to read. Consider when you're looking at a line containing a call to randint somewhere. You might want to see how randint is implemented, to understand whether it's doing what you expect. To do so in a simple text editor you would have to first search for the string "randint", and once you confirm that it's not defined locally you'd have to look through each of the * imports to see where it's defined. If you from random import randint instead it's really easy to find.
  4. Using global can also be handy, but it makes the interactions between the various parts of your code much harder to understand. The standard fix for this is to simply remove a single mention of the global keyword and instead pass parameters around until each piece of code gets exactly what it needs from the caller. You can then repeat this until there are no more globals.
  5. The string consisting of all valid key characters is repeated a few times. If you pull it out into a constant such as KEY_CHARACTERS = "[…]" at the top of your file you can just refer to that in the rest of your code.
  6. This program mixes GUI (TK) code with encryption code. It would be good to split the GUI code into its own file, and importing the relevant functions there. This makes it possible to understand and use the encryption code on its own, for example if you want to create a GUI with a different toolkit.
  7. If you try to import your file in another file Python will actually run the code. This which means the code isn't actually reusable. The way to work around this is to create a main function in your file and call it only when your script is run from the shell:

    if __name__ == "__main__":
        main()
    

    Your main function actually does the work of instantiating and running the GUI, which means the various functions can be imported without side effects.

  8. "".join(key) should be unnecessary, because genkey already returns a string.
  9. The various modes should probably be an enum with values like ENCRYPTION and DECRYPTION - that way the "magic numbers" assigned to mode are instead human readable variable references such as modes.ENCRYPTION.
  10. You probably already know this, but I need to warn about it just in case: you should not use this code to transfer actual secrets. Cryptography is really, really difficult, and there are existing systems like PGP (available from tools like gnupg) to do this. If you want to know how they work there is a lot of advanced maths and difficult code involved.

Tool support suggestions

  1. black can automatically format your code to be more idiomatic. It'll do things like adjusting the vertical and horizontal spacing, while keeping the functionality of the code unchanged.
  2. isort can group imports (built-ins first, then libraries and finally imports from within your project) and sort them.
  3. flake8 can give you more hints to write idiomatic Python:

    [flake8]
    max-complexity = 4
    ignore = W503,E203
    
  4. I would then recommend validating your type hints using a strict mypy configuration:

    [mypy]
    check_untyped_defs = true
    disallow_any_generics = true
    disallow_untyped_defs = true
    ignore_missing_imports = true
    no_implicit_optional = true
    warn_redundant_casts = true
    warn_return_any = true
    warn_unused_ignores = true
    

    This ensures that anyone reading the code (including yourself) understand how it's meant to be called, which is very powerful in terms of modifying and reusing it. For example, it looks like the genkey signature should be def genkey() -> str: to show that it returns a string, and debug's signature should be def debug(text: str) -> None: to show that it takes a string (as opposed to for example bytes or StringIO) and does not return anything.

General suggestions

  1. Using a free IDE like PyCharm Community or Visual Studio Code is incredibly helpful to write non-trivial code. I can't possibly go into detail here, but if you explore an IDE you'll find literally hundreds of helpful features, such as highlighting unused variables, automatically pulling out variables and inlining them, and spell checking.
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ "The standard fix for this is to simply remove a single mention of the global keyword and instead pass parameters around until each piece of code gets exactly what it needs from the caller". I'd say the given code is a great example of how classes can vastly simplify the code and make it more structured and avoid the use of global.. having a dozen parameters on a function isn't particularly great either. \$\endgroup\$ – Voo Nov 10 '19 at 12:37
  • 1
    \$\begingroup\$ @Voo Good point. I avoided mention of OOP here because I thought it would be appropriate for a follow-up review once OP gets rid of all the globals. Moving directly from globals to OOP is two steps in one go IMO, and that's often harmful in terms of coming up with a good structure. \$\endgroup\$ – l0b0 Nov 10 '19 at 18:59
12
\$\begingroup\$

In addition to the great answer already provided by l0b0, a few comments.

1. code structure

Some blocks could be extracted to functions. For instance, the following block inside encrypt:

    new_key_thing = []
    new_key_thing.append(i[0])
    new_key_thing.append(i[1:3])
    new_key_thing.append(i[3:])
    new_key.append(new_key_thing)

Could be refactored to:

new_key.append(do_transformation(i))

Re-iterating on the new code, you could use Python's map operation, which is very handy, or you could use Python's cool feature of list comprehension. Here's an example of refactoring (note that I also renamed from i, which is more used to be an index number to k, which indicates it's a key):

newkey = [do_transformation(k) for k in key_list]

2. Magic numbers

In some places the code includes magic numbers like 3, 5 and more. Instead of using those numbers as are, try to give them meaningful names.

For instance, dealing with magic number 5 (I hope I got the intention correctly, but even if that's not the case, you can see what the general idea is):

CHUNK_SIZE = 4 

while not key == '':
    key_list.append(key[:(CHUNK_SIZE + 1)])
    key = key[(CHUNK_SIZE + 1):]
| improve this answer | |
\$\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.