Merge "Properly trimm bcrypt hashed passwords"

keystone/common/password_hashing.py

@@ -68,7 +68,7 @@ def verify_length_and_trunc_password(password):
     # When using bcrypt, we limit the password length to 54 to ensure all
     # bytes are fully mixed. See:
     # https://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#security-issues
-    BCRYPT_MAX_LENGTH = 54
+    BCRYPT_MAX_LENGTH = 72
     if (CONF.identity.password_hash_algorithm == 'bcrypt' and  # nosec: B105
             CONF.identity.max_password_length > BCRYPT_MAX_LENGTH):
         msg = "Truncating password to algorithm specific maximum length %d characters."
@@ -78,16 +78,17 @@ def verify_length_and_trunc_password(password):
         max_length = CONF.identity.max_password_length
 
     try:
-        if len(password) > max_length:
+        password_utf8 = password.encode('utf-8')
+        if len(password_utf8) > max_length:
             if CONF.strict_password_check:
                 raise exception.PasswordVerificationError(size=max_length)
             else:
                 msg = "Truncating user password to %d characters."
                 LOG.warning(msg, max_length)
-                return password[:max_length]
+                return password_utf8[:max_length]
         else:
-            return password
-    except TypeError:
+            return password_utf8
+    except AttributeError:
         raise exception.ValidationError(attribute='string', target='password')
 
 
@@ -100,7 +101,7 @@ def check_password(password, hashed):
     """
     if password is None or hashed is None:
         return False
-    password_utf8 = verify_length_and_trunc_password(password).encode('utf-8')
+    password_utf8 = verify_length_and_trunc_password(password)
     hasher = _get_hasher_from_ident(hashed)
     return hasher.verify(password_utf8, hashed)
 
@@ -117,7 +118,7 @@ def hash_user_password(user):
 def hash_password(password):
     """Hash a password. Harder."""
     params = {}
-    password_utf8 = verify_length_and_trunc_password(password).encode('utf-8')
+    password_utf8 = verify_length_and_trunc_password(password)
     conf_hasher = CONF.identity.password_hash_algorithm
     hasher = _HASHER_NAME_MAP.get(conf_hasher)
 

keystone/conf/identity.py

@@ -103,7 +103,7 @@ performance. Changing this value does not effect existing passwords. This value
 can also be overridden by certain hashing algorithms maximum allowed length
 which takes precedence over the configured value.
 
-The bcrypt max_password_length is 54.
+The bcrypt max_password_length is 72 bytes.
 """))
 
 list_limit = cfg.IntOpt(

keystone/tests/unit/common/test_utils.py

@@ -77,7 +77,7 @@ class UtilsTestCase(unit.BaseTestCase):
         self.config_fixture.config(strict_password_check=False)
         password = uuid.uuid4().hex
         verified = common_utils.verify_length_and_trunc_password(password)
-        self.assertEqual(password, verified)
+        self.assertEqual(password.encode('utf-8'), verified)
 
     def test_that_a_hash_can_not_be_validated_against_a_hash(self):
         # NOTE(dstanek): Bug 1279849 reported a problem where passwords
@@ -97,7 +97,7 @@ class UtilsTestCase(unit.BaseTestCase):
         max_length = CONF.identity.max_password_length
         invalid_password = 'passw0rd'
         trunc = common_utils.verify_length_and_trunc_password(invalid_password)
-        self.assertEqual(invalid_password[:max_length], trunc)
+        self.assertEqual(invalid_password.encode('utf-8')[:max_length], trunc)
 
     def test_verify_long_password_strict_raises_exception(self):
         self.config_fixture.config(strict_password_check=True)
@@ -139,8 +139,8 @@ class UtilsTestCase(unit.BaseTestCase):
         self.config_fixture.config(group='identity',
                                    password_hash_algorithm='bcrypt')
         self.config_fixture.config(group='identity',
-                                   max_password_length='64')
-        invalid_length_password = '0' * 64
+                                   max_password_length='96')
+        invalid_length_password = '0' * 96
         self.assertRaises(exception.PasswordVerificationError,
                           common_utils.hash_password,
                           invalid_length_password)

releasenotes/notes/bcrypt_truncation_fix-674dc5d7f1e776f2.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Passwords that are hashed using bcrypt are now truncated properly to the
+    maximum allowed length by the algorythm. This solves regression, when
+    passwords longer then 54 symbols are getting invalidated after the
+    Keystone upgrade.