/synapse

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add pepper to password hashing #907

Merged
merged 5 commits into from Jul 5, 2016

Conversation

Reviewers
No reviews
Assignees

@erikjohnston erikjohnston

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@KentShikama @matrixbot @erikjohnston
@KentShikama
Contributor

KentShikama commented Jul 4, 2016

Random pepper generated by

dd if=/dev/urandom bs=1k count=1 2>/dev/null | base64
HR32t0xZcQnzn3O0ZkEVuetdFvH1W6TeEPw6JjH0Cl+qflVOseGyFJlJR7ACLnywjN997kuy9kbN
bBCUiAWT/630Dd8qsBHu6+nZGnsQ4FH0Eo5Psh+NFoDUSIwYQRUWW+3jSSZMPYb7qvBl+ww3j9f+
5l+BlhhiV7QOUTJwCWTvp0G+Rb2C0SVATswaxNnf79bC9Dme8/CKhlfx0KVuYnEOhkOEEMn99xy3
/DxE3O6njWH1HkiE20iLUERPhtnBeQXm10ZzWenIUC4mEXXBXAffRgRgqiSm3e6sv4ngPBdrAekt

I took the first x characters so the line length was 89 to fit within the < 90 char limit.
@KentShikama
Add pepper to password hashing 
Signed-off-by: Kent Shikama <kent@kentshikama.com>
This commit was signed with a verified signature.
@KentShikama KentShikama Kent Shikama
GPG key ID: D275958A0FA61427 Learn about signing commits

Some checks were not successful

4 failing and 1 successful checks
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
8bdaf5f
@matrixbot

matrixbot commented Jul 4, 2016

Can one of the admins verify this patch?
@matrixbot

matrixbot commented Jul 4, 2016

Can one of the admins verify this patch?
@matrixbot

matrixbot commented Jul 4, 2016

Can one of the admins verify this patch?
@matrixbot

matrixbot commented Jul 4, 2016

Can one of the admins verify this patch?
@matrixbot

matrixbot commented Jul 4, 2016

Can one of the admins verify this patch?

@erikjohnston erikjohnston and 1 other commented on an outdated diff Jul 5, 2016

View full outdated diff
synapse/config/password.py
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.
+ # DO NOT CHANGE THIS AFTER INITIAL SETUP!
+ #pepper: "HR32t0xZcQnzn3O0ZkEVuetdFvH1W6TeEPw6JjH0Cl+qflVOseGyFJlJR7ACLnywjN9"
@erikjohnston

erikjohnston Jul 5, 2016

Owner

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>"
@KentShikama

KentShikama Jul 5, 2016

Contributor

Yeah it does indeed

@erikjohnston erikjohnston self-assigned this Jul 5, 2016

@erikjohnston
Owner

erikjohnston commented Jul 5, 2016

@matrixbot ok to test

KentShikama added some commits Jul 5, 2016

@KentShikama
Add comment to prompt changing of pepper
This commit was signed with a verified signature.
@KentShikama KentShikama Kent Shikama
GPG key ID: D275958A0FA61427 Learn about signing commits

Some checks were not successful

4 failing and 1 successful checks
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
507b8bb
@KentShikama
Fix pep8
This commit was signed with a verified signature.
@KentShikama KentShikama Kent Shikama
GPG key ID: D275958A0FA61427 Learn about signing commits

Some checks were not successful

3 failing and 2 successful checks
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
1ee2584

@erikjohnston erikjohnston and 1 other commented on an outdated diff Jul 5, 2016

View full outdated diff
synapse/handlers/auth.py
@@ -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,
@erikjohnston

erikjohnston Jul 5, 2016

Owner

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

@KentShikama

KentShikama Jul 5, 2016

Contributor

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.

@erikjohnston erikjohnston commented on an outdated diff Jul 5, 2016

View full outdated diff
synapse/config/password.py
@@ -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", "")
@erikjohnston

erikjohnston Jul 5, 2016

Owner

Can we name this password_pepper or something?

@erikjohnston erikjohnston commented on an outdated diff Jul 5, 2016

View full outdated diff
synapse/config/password.py
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"
@erikjohnston

erikjohnston Jul 5, 2016

Owner

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.

KentShikama added some commits Jul 5, 2016

@KentShikama
Fix password config
This commit was signed with a verified signature.
@KentShikama KentShikama Kent Shikama
GPG key ID: D275958A0FA61427 Learn about signing commits
14362bf
@KentShikama
Remove default password pepper string
This commit was signed with a verified signature.
@KentShikama KentShikama Kent Shikama
GPG key ID: D275958A0FA61427 Learn about signing commits

Some checks were not successful

1 failing and 4 successful checks
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
252ee2d
@erikjohnston
Owner

erikjohnston commented Jul 5, 2016

(I think the dendron test failure is nothing to do with this PR)
@erikjohnston
Owner

erikjohnston commented Jul 5, 2016

Thanks for this! :)

@erikjohnston erikjohnston merged commit e34cb5e into matrix-org:develop Jul 5, 2016
4 of 5 checks passed

4 of 5 checks passed

Sytest Dendron (Merged PR) Build finished.
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Merged PR) Build finished.
Details

@KentShikama KentShikama deleted the KentShikama:pepper branch Jul 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment