Properly trimm bcrypt hashed passwords

bcrypt  hashing algorythm has a limitation on length of passwords it
can hash on 72 bytes. In [1] a password trimm to 54 symbols has been
implemented, which resulted in password being invalidated after the
keystone upgrade, since passwords are trimmed differently by bcrypt
itself, as well as len(str()) is not always equal to
len(str().encode()) as trimming should be done based on bytes and not
string itself.

With the change we return a byte object from
`verify_length_and_trunc_password`, so it does not need to
be encoded afterwards, since we need to strip based on bytes
rather then on length of the string.

[1] https://review.opendev.org/c/openstack/keystone/+/828595

Closes-Bug: #2028809
Related-Bug: #1901891
Change-Id: Iea95a3c2df041a0046647b3d3dadead1a6d054d1

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.