7
\$\begingroup\$

I'm a beginner and am self taught and was hoping I could get help from others more educated than me to show me what bad practices I might be forming before I go on further.

P.S. This program is just for me to learn, I'm not actually storing passwords with it.

#!/usr/bin/env python
# -*- coding: utf-8 -*-
import sys

def openPassFile():
    try:
        passwordFile = open('hashwork.txt', 'a')
        return passwordFile
    except IOError as e:
        print("I/O Error({0}): {1}".format(e.errno, e.strerror))
        quit

def closePassFile(passwordFile):
    try:
        passwordFile.close()
    except IOError as e:
        print("I/O Error({0}): {1}".format(e.errno, e.strerror))
        quit

def randomValue(length):
    import random
    salt_chars = 'abcdefghijklmnopqrstuvwxyz0123456789'
    return ''.join(random.choice(salt_chars) for x in range(length))

def askforUsername():
    while True:
        print("Please enter the username you would like to use:")
        username = raw_input()
        return username

def askforPassword():
    import getpass, hashlib
    while True:
        print("What password would you like to create?")
        salt = randomValue(16)
        hashedPassword1 = hashlib.sha256(salt+getpass.getpass()).hexdigest()
        print("\nPlease enter password again.")
        hashedPassword2 = hashlib.sha256(salt+getpass.getpass()).hexdigest()
        if hashedPassword1 == hashedPassword2:
            return hashedPassword2, salt
            break
        else:
            print("Your passwords do not match. Please retry")

def storeInfo(username, hashedPass, salt):
    passwordFile = openPassFile()
    passwordFile.write(username + " | " + hashedPass + " | " + salt + "\n")
    closePassFile(passwordFile)

username = askforUsername()         
hashedPass, salt = askforPassword()
storeInfo(username, hashedPass, salt)
sys.exit()
\$\endgroup\$
5
\$\begingroup\$
  • SHA256 is not a secure password hash because it is fast. See How to securely hash passwords? on security.se for details.
  • It's not obvious that random is good enough to generate salts. While the requirments for salts are lower than those for keys, you still need a decent RNG. I recommend using SystemRandom.
\$\endgroup\$
1
\$\begingroup\$

I would start this by saying that you did a pretty good job on your first program. Even if there are things that can be improved, I like that you used short functions for every logic. Anyway, I can see a couple of things that can be changed in your code:

  • first, I'd recommend you to have a look at PEP8 documentation. It's very straight-forward

Function names:

Function names should be lowercase, with words separated by underscores as necessary to improve readability. mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

So you'd have:

openPassFile() -> open_pass_file() .. and so on

The same rules apply for variables.

Spacing:

Usually, between methods you should have two lines:

def openPassFile():
...


def closePassFile(passwordFile):
...

Imports:

Try importing modules at the beginning of the file. Avoid using imports each time a function it's called:

import getpass
import hashlib
import random
import sys

# rest of the code
# ...

Try using if __name__ == "__main__":. One of the reasons for doing this is that sometimes you write a module (a .py file) where it can be executed directly. Alternatively, it can also be imported and used in another module. By doing the main check, you can have that code only execute when you want to run the module as a program and not have it execute when someone just wants to import your module and call your functions themselves.

#...
if __name__ == "__main__":
    username = ask_for_username()
    hashedPass, salt = ask_for_password()
    store_info(username, hashedPass, salt)
    sys.exit()

Don't use quit to exit from a function. Some good tips you could follow:

  • Use exit() or quit() in the REPL.
  • Use sys.exit() in scripts, or raise SystemExit() if you prefer.
  • Use os._exit() for child processes to exit after a call to os.fork()

As @Racialz mentioned, I'm not a fan of open() when it comes to handling file handling. Just use with(). This way, you'd get rid off close_pass_file() as the with() statement handles closing the files by itself

That being said, your final code would look like this:

import getpass
import hashlib
import random
import sys


def open_pass_file():
    try:
        with open('hashwork.txt', 'a') as password_file:
            return password_file
    except IOError as e:
        print("I/O Error({0}): {1}".format(e.errno, e.strerror))
        sys.exit()


def random_value(length):
    salt_chars = 'abcdefghijklmnopqrstuvwxyz0123456789'
    return ''.join(random.choice(salt_chars) for x in range(length))


def ask_for_username():
    while True:
        print("Please enter the username you would like to use:")
        username = input()
        return username


def ask_for_password():
    while True:
        print("What password would you like to create?")
        salt = random_value(16)
        hashed_password1 = hashlib.sha256(salt + getpass.getpass()).hexdigest()
        print("\nPlease enter password again.")
        hashed_password2 = hashlib.sha256(salt + getpass.getpass()).hexdigest

        if hashed_password1 == hashed_password2:
            return hashed_password2, salt
        else:
            print("Your passwords do not match. Please retry")


def store_info(username, hashed_pass, salt):
    password_file = open_pass_file()
    password_file.write(username + " | " + hashed_pass + " | " + salt + "\n")


if __name__ == "__main__":
    username = ask_for_username()
    hashedPass, salt = ask_for_password()
    store_info(username, hashedPass, salt)
\$\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.