Skip to content

Commit

Permalink
Use argon2-cffi directly instead of through passlib
Browse files Browse the repository at this point in the history
Unfortunately passlib appears unmaintained with no commits in the past 2
years and literal spam tickets in their issue tracker. It brings in ~20k
lines of code (per tokei), but we only need the argon2 functionality
that is in argon2-cffi already.

Using argon2-cffi directly is roughly the same, with just a few renames
(e.g. rounds → time_cost) and different method signatures (verify()
throws an exception instead of returning false).

We explicitly specify a type=argon2i, in the next commit we'll switch
over to argon2id.

Fixes #6631.
  • Loading branch information
legoktm committed Oct 19, 2022
1 parent 3288be8 commit 77b3e11
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
import os
import uuid

import argon2
import pyotp
import sqlalchemy as sa
from alembic import op
from passlib.hash import argon2

# raise the errors if we're not in production
raise_errors = os.environ.get("SECUREDROP_ENV", "prod") != "prod"
Expand All @@ -33,7 +33,7 @@

def generate_passphrase_hash() -> str:
passphrase = PassphraseGenerator.get_default().generate_passphrase()
return argon2.using(**ARGON2_PARAMS).hash(passphrase)
return argon2.PasswordHasher(**ARGON2_PARAMS).hash(passphrase)


def create_deleted() -> int:
Expand Down
21 changes: 13 additions & 8 deletions securedrop/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from logging import Logger
from typing import Any, Callable, Dict, List, Optional, Union

import argon2
import pyotp
import qrcode

Expand All @@ -20,7 +21,6 @@
from flask import url_for
from flask_babel import gettext, ngettext
from markupsafe import Markup
from passlib.hash import argon2
from passphrases import PassphraseGenerator
from pyotp import HOTP, TOTP
from sqlalchemy import Boolean, Column, DateTime, ForeignKey, Integer, LargeBinary, String
Expand All @@ -35,7 +35,7 @@
if os.environ.get("SECUREDROP_ENV") == "test":
LOGIN_HARDENING = False

ARGON2_PARAMS = dict(memory_cost=2**16, rounds=4, parallelism=2)
ARGON2_PARAMS = {"memory_cost": 2**16, "time_cost": 4, "parallelism": 2, "type": argon2.Type.I}

# Required length for hex-format HOTP secrets as input by users
HOTP_SECRET_LENGTH = 40 # 160 bits == 40 hex digits (== 32 ascii-encoded chars in db)
Expand Down Expand Up @@ -479,10 +479,11 @@ def set_password(self, passphrase: "Optional[str]") -> None:

self.check_password_acceptable(passphrase)

hasher = argon2.PasswordHasher(**ARGON2_PARAMS)
# "migrate" from the legacy case
if not self.passphrase_hash:
self.passphrase_hash = argon2.using(**ARGON2_PARAMS).hash(passphrase)
# passlib creates one merged field that embeds randomly generated
self.passphrase_hash = hasher.hash(passphrase)
# argon2 creates one merged field that embeds randomly generated
# salt in the output like $alg$salt$hash
self.pw_hash = None
self.pw_salt = None
Expand All @@ -491,7 +492,7 @@ def set_password(self, passphrase: "Optional[str]") -> None:
if self.passphrase_hash and self.valid_password(passphrase):
return

self.passphrase_hash = argon2.using(**ARGON2_PARAMS).hash(passphrase)
self.passphrase_hash = hasher.hash(passphrase)

def set_name(self, first_name: Optional[str], last_name: Optional[str]) -> None:
if first_name:
Expand Down Expand Up @@ -550,9 +551,13 @@ def valid_password(self, passphrase: "Optional[str]") -> bool:
# No check on minimum password length here because some passwords
# may have been set prior to setting the mininum password length.

hasher = argon2.PasswordHasher(**ARGON2_PARAMS)
if self.passphrase_hash:
# default case
is_valid = argon2.verify(passphrase, self.passphrase_hash)
try:
is_valid = hasher.verify(self.passphrase_hash, passphrase)
except argon2.exceptions.VerificationError:
is_valid = False
else:
# legacy support
if self.pw_salt is None:
Expand All @@ -569,8 +574,8 @@ def valid_password(self, passphrase: "Optional[str]") -> bool:

# migrate new passwords
if is_valid and not self.passphrase_hash:
self.passphrase_hash = argon2.using(**ARGON2_PARAMS).hash(passphrase)
# passlib creates one merged field that embeds randomly generated
self.passphrase_hash = hasher.hash(passphrase)
# argon2 creates one merged field that embeds randomly generated
# salt in the output like $alg$salt$hash
self.pw_salt = None
self.pw_hash = None
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ Flask>=2.0.3,<2.1.0
Jinja2>=3.0.0
markupsafe>=2.0
mod_wsgi
passlib
pretty-bad-protocol>=3.1.1
psutil>=5.6.6
pyotp>=2.6.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,6 @@ markupsafe==2.0.1 \
mod-wsgi==4.6.7 \
--hash=sha256:b788abaf0b903a64a7bb8dae609f2e4790c87e6f3d716aa6bc97936410fcbfcc
# via -r requirements/python3/securedrop-app-code-requirements.in
passlib==1.7.1 \
--hash=sha256:3d948f64138c25633613f303bcc471126eae67c04d5e3f6b7b8ce6242f8653e0 \
--hash=sha256:43526aea08fa32c6b6dbbbe9963c4c767285b78147b7437597f992812f69d280
# via -r requirements/python3/securedrop-app-code-requirements.in
pretty-bad-protocol==3.1.1 \
--hash=sha256:fb73797eb6344e3680c919e2af367f9a019ee7c189d3ed7f5d843edde911f73b
# via -r requirements/python3/securedrop-app-code-requirements.in
Expand Down
6 changes: 4 additions & 2 deletions securedrop/tests/test_journalist.py
Original file line number Diff line number Diff line change
Expand Up @@ -2617,6 +2617,7 @@ def test_passphrase_migration_on_verification(journalist_app):

# check that the migration happened
assert journalist.passphrase_hash is not None
assert journalist.passphrase_hash.startswith("$argon2")
assert journalist.pw_salt is None
assert journalist.pw_hash is None

Expand All @@ -2639,6 +2640,7 @@ def test_passphrase_migration_on_reset(journalist_app):

# check that the migration happened
assert journalist.passphrase_hash is not None
assert journalist.passphrase_hash.startswith("$argon2")
assert journalist.pw_salt is None
assert journalist.pw_hash is None

Expand Down Expand Up @@ -3054,7 +3056,7 @@ def db_assertion():


def test_login_with_invalid_password_doesnt_call_argon2(mocker, test_journo):
mock_argon2 = mocker.patch("models.argon2.verify")
mock_argon2 = mocker.patch("models.argon2.PasswordHasher")
invalid_pw = "a" * (Journalist.MAX_PASSWORD_LEN + 1)

with pytest.raises(InvalidPasswordLength):
Expand All @@ -3063,7 +3065,7 @@ def test_login_with_invalid_password_doesnt_call_argon2(mocker, test_journo):


def test_valid_login_calls_argon2(mocker, test_journo):
mock_argon2 = mocker.patch("models.argon2.verify")
mock_argon2 = mocker.patch("models.argon2.PasswordHasher")
Journalist.login(
test_journo["username"],
test_journo["password"],
Expand Down

0 comments on commit 77b3e11

Please sign in to comment.