Merge pull request #6657 from freedomofpress/argon2

Drop passlib, switch to argon2id hashes

securedrop/alembic/versions/2e24fc7536e8_make_journalist_id_non_nullable.py

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

securedrop/models.py

@@ -8,6 +8,7 @@
 from logging import Logger
 from typing import Any, Callable, Dict, List, Optional, Union
 
+import argon2
 import pyotp
 import qrcode
 
@@ -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
@@ -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.ID}
 
 # 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)
@@ -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
@@ -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:
@@ -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:
@@ -567,13 +572,28 @@ def valid_password(self, passphrase: "Optional[str]") -> bool:
                 self._scrypt_hash(passphrase, self.pw_salt), self.pw_hash
             )
 
-        # 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
-            # salt in the output like $alg$salt$hash
+        # If the passphrase isn't valid, bail out now
+        if not is_valid:
+            return False
+        # From here on we can assume the passphrase was valid
+
+        # Perform migration checks
+        needs_update = False
+        if self.passphrase_hash:
+            # Check if the hash needs an update
+            if hasher.check_needs_rehash(self.passphrase_hash):
+                self.passphrase_hash = hasher.hash(passphrase)
+                needs_update = True
+        else:
+            # Migrate to an argon2 hash, which creates one merged field
+            # that embeds randomly generated salt in the output like
+            # $alg$salt$hash
+            self.passphrase_hash = hasher.hash(passphrase)
             self.pw_salt = None
             self.pw_hash = None
+            needs_update = True
+
+        if needs_update:
             db.session.add(self)
             db.session.commit()
 

securedrop/requirements/python3/securedrop-app-code-requirements.in

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

securedrop/requirements/python3/securedrop-app-code-requirements.txt

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

securedrop/tests/test_journalist.py

@@ -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
 
@@ -2639,13 +2640,27 @@ 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
 
     # check that that a verification post-migration works
     assert journalist.valid_password(VALID_PASSWORD)
 
 
+def test_passphrase_argon2i_migration(test_journo):
+    """verify argon2i hashes work and then are migrated to argon2id"""
+    journalist = test_journo["journalist"]
+    # But use our password hash
+    journalist.passphrase_hash = (
+        "$argon2i$v=19$m=65536,t=4,p=2$JfFkLIJ2ogPUDI19XiBzHA$kaKNVckLLQNNBnmllMWqXg"
+    )
+    db.session.add(journalist)
+    db.session.commit()
+    assert journalist.valid_password("correct horse battery staple profanity oil chewy")
+    assert journalist.passphrase_hash.startswith("$argon2id$")
+
+
 def test_journalist_reply_view(journalist_app, test_source, test_journo, app_storage):
     source, _ = utils.db_helper.init_source(app_storage)
     journalist, _ = utils.db_helper.init_journalist()
@@ -3054,24 +3069,14 @@ 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):
         Journalist.login(test_journo["username"], invalid_pw, TOTP(test_journo["otp_secret"]).now())
     assert not mock_argon2.called
 
 
-def test_valid_login_calls_argon2(mocker, test_journo):
-    mock_argon2 = mocker.patch("models.argon2.verify")
-    Journalist.login(
-        test_journo["username"],
-        test_journo["password"],
-        TOTP(test_journo["otp_secret"]).now(),
-    )
-    assert mock_argon2.called
-
-
 def test_render_locales(config, journalist_app, test_journo, test_source):
     """the locales.html template must collect both request.args (l=XX) and
     request.view_args (/<filesystem_id>) to build the URL to