Object oriented python

Question

I am starting my adventure with Python and object oriented programming.

I wrote a script for salting and hashing password. My main goal is practice with OOP, not cryptography. I know there is no error handling.

Wonder how improve my code it and make more object oriented. It would be nice if someone more experienced can take a look and give me some advice how to make it better. Any help for newbie would be great!

Description:

The script is not really useful in real life.

1st part:
Hash and salt password and next encode to base64 with added salt (needed for future comparing hashes). Algorithm: base64( SHA256(password + salt) + salt) Salt is X random bytes where X can be specify but by default it is 16.

2nd part (some kind of authorization):
Compare input hash (e.x. from database) with new created hash from input plain password. New salt is created from salt taken from input hash.

Code:

import base64
import hashlib
import os


class Hashing(object):

    # base64( SHA256(password + salt) + salt)

    # generate new salt (default 16 bytes)
    def generate_new_salt(self, salt_ln=16):

        self.new_salt = os.urandom(salt_ln)

        print(f'new salt: {self.new_salt}')

        return self.new_salt

    # get salt from hash
    def get_old_salt(self, input_hash, salt_ln=16):

        self.old_salt = base64.b64decode(input_hash)[-salt_ln:]

        print(f'old salt: {self.old_salt}')

        return self.old_salt

    # compute hash using parameters
    def compute_hash(self, password, salt):

        self.salt = salt
        self.enc_password = password.encode()

        # hashing SHA256(password + salt)
        hash_object = hashlib.sha256(self.enc_password + salt)

        # add salt to hash and encode to base64
        hash_b64 = base64.b64encode(hash_object.digest() + salt)

        print(f'new_hash: {hash_b64}')
        return hash_b64

    # create hash from new or old salt
    def create_hash(self, password, salt_ln=16,old_salt=None):

        if old_salt:    #if old salt then use it
            self.salt = old_salt
        else:           #else generate new salt
            self.salt = Hashing().generate_new_salt(salt_ln)


        hash = Hashing().compute_hash(password, self.salt)

        return hash

    # compare input hash with created using salt get from input
    def compare_hashes(self, password, old_hash, salt_ln=16):

        self.enc_password = password.encode()

        #get salt from input hash
        self.old_salt = Hashing().get_old_salt(old_hash, salt_ln)

        #recreat input hash
        re_hash = Hashing().create_hash(password,salt_ln, self.old_salt)

        print(f'Compare: old_hash: {old_hash}')
        print(f'Compare: new_hash: {re_hash}')

        #compare
        if re_hash.decode() == old_hash.decode():
            return True
        else:
            return False



#code below is just for testing

NewSalt = Hashing().generate_new_salt()

Hash = Hashing().create_hash('pass')

OldSalt = Hashing().get_old_salt(Hash)

CompareHash = Hashing().compare_hashes('pass', Hash)

if CompareHash:
    print('HASHES THE SAME')
else:
    print('NOT THE SAME')

print(CompareHash)

Questions:

1. What is a real difference between these two? With and w/o self.X works

   def generate_new_salt(self, salt_ln=16):
           self.new_salt = os.urandom(salt_ln)
           print(f'new salt: {self.new_salt}')
           return self.new_salt
   

   def generate_new_salt(self, salt_ln=16):
       new_salt = os.urandom(salt_ln)
       print(f'new salt: {new_salt}')
       return new_salt
   

2. I have problem with passing default value of salt length, salt_ln = 16. For me it does not look nice when it is replicated in every method. Is there any way to do it more global?

Answer 1

Questions

1. self is the instance of whatever class the method is defined on. In your example the method is generate_new_salt which is defined on the Hashing class.

   When you use my_hashing = Hashing() you're creating an instance and binding it to the variable my_hashing. When you then call the method, via something like my_hashing.generate_new_salt(...) then self is the same as my_hashing. They both point to the same instance of Hashing.

   >>> class Foo:
           def comp(self):
               return my_foo == self
   
   
   >>> my_foo = Foo()
   >>> my_foo.comp()
   True
   

   This means that self.foo = ... binds ... to the instance. This allows you to then use self.foo to get the value, ..., back at a later date. Without having to pass it as an argument. If you have no reason to use self.foo rather than just foo, then using self.foo is not a good idea.

2. This comes from two different but linked things.

   • You're trying to be too helpful. A lot of your code is not doing much, and is just calling other functions.

     This is a common problem when programming, and so I would recommend you follow the KISS principle. If you don't need the helper now, then you don't need it at all.

   • You're not really using objects to their potential. If you wrap all of your methods in staticmethod decorators then nothing will change. This highlights that you're not really following OOP. And it highlights you're barely using the features class provides.

Given the above points, it would be best to start over.

Review

• It's normally recommended to not use print. There are exceptions, but in most cases you're better served either by just not having print or using logging.

  There is one exception, which is to interact with the user via the CLI. And in this case Hashing does not need to do that. Where you're testing needs it and so you should only have them below that comment "code below is just for testing".

• Inheriting from object, Hashing(object), is redundant in Python 3.

• Rather than using an if to compare something and then return True and False. You can just return the comparison.

  return re_hash.decode() == old_hash.decode()
  

• It would be best if the code below the comment "code below is just for testing" were in a function. This would mean that the global namespace isn't polluted with things that don't need to be there.

• It's best to use an if __name__ == '__main__': guard. Since you only run the main function if you're testing, this will allow you to import the code without the testing code running.

From the ground up

1. Merging both the hash and the salt into the encoding, "base64(hash + salt)", is a smart way to keep the hash and the salt together. However it would be clearer if you create a class that stores the hash and the salt independently.

   class PasswordHash:
       def __init__(self, hash, salt):
           self.hash = hash
           self.salt = salt
   
   ph = PasswordHash(b'hash', b'salt')
   print(ph.hash)  # b'hash'
   print(ph.salt)  # b'salt'
   

   From here you should be able to see two things:

   • The method get_old_salt is now obsolete.
   • You no longer need to care about the salt length.

2. When you first define a class, after writing the __init__, you should ask yourself if the class will ever be printed. If it could be printed, then you should make at least one of these dunder methods, double underscore method.

   • __repr__ - Instructions on how to rebuild the method.
   • __str__ - A human friendly interpretation of the class.

   In this case __repr__ could make sense, but __str__ doesn't.

   def __repr__(self):
       return f'PasswordHash({self.hash}, {self.salt})'
   

   You should be able to see, __repr__ outputs the exact text we used to make the instance.

   >>> print(PasswordHash(b'hash', b'salt'))
   PasswordHash(b'hash', b'salt')
   

3. From here we need to think about how we should hash the password, since we don't want to store the password in plaintext. And so we can add compute_hash to PasswordHash, with 2 modifications.

   • I prefer the name hash_password, as it explains what it does, and what it does it on.

   • Secondly I would make it a staticmethod. This comes with a couple of reasons why:

     1. It allows us to create PasswordHash from a password easier.
     2. It allows for subclasses of PasswordHash to change the way the class hashes things.

   @staticmethod
   def hash_password(password, salt):
       hash = hashlib.sha256(password.encode() + salt)
       return base64.b64encode(hash.digest())
   

   Currently making a PasswordHash from a password is pretty ugly.

   ph = PasswordHash(PasswordHash.hash_password(password, salt), salt)
   

4. Since the current interface for making a PasswordHash from a password is not the nicest. And is the standard way to use the class, we can make a class method to make it pretty.

   The reason this is a classmethod and not a standard method is because we need it to run before __init__. It is also a classmethod and not a staticmethod as we'll be using the first argument, cls, to instantiate the class.

   @classmethod
   def from_password(cls, password, salt):
       return cls(cls.hash_password(password, salt), salt)
   

   Now making the PasswordHash from a password is really clean.

   ph = PasswordHash.from_password('pass', salt)
   

5. We're almost at the end. The last thing to do is to add the spiritual child of compare_hashes to the class. Which is really simple since we have self.salt and PasswordHash.hash_password.

   def compare(self, password):
       return self.hash == self.hash_password(password, self.salt)
   

Tying this all together you can get:

import base64
import hashlib
import os


def new_salt(length=16):
    return os.urandom(length)


class PasswordHash:
    def __init__(self, hash, salt):
        self.hash = hash
        self.salt = salt

    @classmethod
    def from_password(cls, password, salt):
        return cls(cls.hash_password(password, salt), salt)

    def __repr__(self):
        return f'PasswordHash({self.hash}, {self.salt})'

    @staticmethod
    def hash_password(password, salt):
        hash = hashlib.sha256(password.encode() + salt)
        return base64.b64encode(hash.digest())

    def compare(self, password):
        return self.hash == self.hash_password(password, self.salt)


def main():
    salt = new_salt()
    print('salt:', salt)
    hash = PasswordHash.from_password('pass', salt)
    print('PasswordHash:', hash)
    print('hash:', hash.hash)
    print('salt:', hash.salt)
    print('compare pass:', hash.compare('pass'))
    print('compare bar :', hash.compare('bar'))


if __name__ == '__main__':
    main()

To make the comparison to be more OOP you could rename compare to the dunder method __eq__. And then you could use it via the == operator.

print('compare pass:', hash == 'pass')
print('compare bar :', hash == 'bar')