Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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()
share|improve this question
1  
SHA256 is not a secure password hash because it is fast. See How to securely hash passwords? on security.se for details. – CodesInChaos May 4 at 6:28
  • 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.
share|improve this answer

PEP 8 Style Guide: https://www.python.org/dev/peps/pep-0008/

Imports:

First off you import some things inside a function and on the same line, that's a little strange. Pep 8 says the following:

  • Imports should usually be on separate lines
  • Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

Breaks

In this code in askForPassword() your break never gets called and does nothing. The function stops as you return, anything below the return will never get evaluated.

if hashedPassword1 == hashedPassword2:
            return hashedPassword2, salt
            break

Quit

A lot of your functions at the top just end with quit. First of all quit is a function. You would call it like this quit(). This answer explains exactly why you shouldn't use quit: http://stackoverflow.com/questions/19747371/python-exit-commands-why-so-many-and-when-should-each-be-used.

But the gist is that you should use sys.exit() because it doesn't require the site module.

Also at the very bottom you have sys.exit() this line really does nothing. The program is going to exit on it's own. There's no need to have this line here

Reading and Writing Info

passwordFile.write(username + " | " + hashedPass + " | " + salt + "\n")

Storing information using your own delimiter is almost never the right way to do things like this. What if the username contains a |, what if the file gets corrupted? This also complicates things when you want to read the data back. One much better solution would be to JSON encode the information you want to store. This makes reading and writing this data much easier.

Comments

Some of the lines in your code that are a little vague to a new reader should be commented. For example

#create random string of characters for specified length
return ''.join(random.choice(salt_chars) for x in range(length))

Some functions

Your reading and writing of the text file is a little weird. You open the file in one function, then write to that file in another function, then close that file in another function. You should just make one function like store_info that does it all together.

def store_info(filepath, username, hashedPass, salt):
    with open(filepath, 'a') as f:
        filepath.write(username + "|" + hashedPass + "|" + salt + "\n")

This is a little nitpicky but PEP 8 also says Function names should be lowercase, with words separated by underscores as necessary to improve readability. So you could change your function names to use lowercase and underscores instead of mixedCase. mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

share|improve this answer

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)
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.