Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add pepper to password hashing #907

Merged
merged 5 commits into from
Jul 5, 2016
Merged

Add pepper to password hashing #907

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8bdaf5f
Add pepper to password hashing
KentShikama Jul 4, 2016
507b8bb
Add comment to prompt changing of pepper
KentShikama Jul 5, 2016
1ee2584
Fix pep8
KentShikama Jul 5, 2016
14362bf
Fix password config
KentShikama Jul 5, 2016
252ee2d
Remove default password pepper string
KentShikama Jul 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions synapse/config/password.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,15 @@ class PasswordConfig(Config):
def read_config(self, config):
password_config = config.get("password_config", {})
self.password_enabled = password_config.get("enabled", True)
self.pepper = password_config.get("pepper", "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name this password_pepper or something?


def default_config(self, config_dir_path, server_name, **kwargs):
return """
# Enable password for login.
password_config:
enabled: true
# Uncomment for extra security for your passwords.
# Change to a secret random string.
# DO NOT CHANGE THIS AFTER INITIAL SETUP!
#pepper: "HR32t0xZcQnzn3O0ZkEVuetdFvH1W6TeEPw6JjH0Cl+qflVOseGyFJlJR7ACLnywjN9"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this need to be a secret to each instance? I'd prefer if we instead had it like:

# Uncomment for extra security for your passwords.
# Change to a secret random string.
# DO NOT CHANGE THIS AFTER INITIAL SETUP!
#pepper: "<SOME_SECRET_RANDOM_STRING>"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it does indeed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may do more harm than good to actually include an example string here that is not an obvious CHANGE_ME placeholder, as I can see people simply uncommenting and forgetting to change.

"""
6 changes: 4 additions & 2 deletions synapse/handlers/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,8 @@ def hash(self, password):
Returns:
Hashed password (str).
"""
return bcrypt.hashpw(password, bcrypt.gensalt(self.bcrypt_rounds))
return bcrypt.hashpw(password + self.hs.config.password_config.pepper,
bcrypt.gensalt(self.bcrypt_rounds))

def validate_hash(self, password, stored_hash):
"""Validates that self.hash(password) == stored_hash.
Expand All @@ -763,6 +764,7 @@ def validate_hash(self, password, stored_hash):
Whether self.hash(password) == stored_hash (bool).
"""
if stored_hash:
return bcrypt.hashpw(password, stored_hash.encode('utf-8')) == stored_hash
return bcrypt.hashpw(password + self.hs.config.password_config.pepper,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.hs.config.password_config.pepper should be self.hs.config.pepper

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops…I wonder how this passed on my local machine. Perhaps I forgot to restart the server once I removed the hardcoded pepper I was testing with.

stored_hash.encode('utf-8')) == stored_hash
else:
return False