Merge "Force algo specific maximum length"

5 files changed, 48 insertions, 3 deletions

diff --git a/keystone/common/password_hashing.py b/keystone/common/password_hashing.py
index 4e62d9c38..b38d3cba7 100644
--- a/keystone/common/password_hashing.py
+++ b/keystone/common/password_hashing.py
@@ -57,8 +57,26 @@ def _get_hasher_from_ident(hashed):
 
 
 def verify_length_and_trunc_password(password):
-    """Verify and truncate the provided password to the max_password_length."""
-    max_length = CONF.identity.max_password_length
+    """Verify and truncate the provided password to the max_password_length.
+
+    We also need to check that the configured password hashing algorithm does
+    not silently truncate the password.  For example, passlib.hash.bcrypt does
+    this:
+    https://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#security-issues
+
+    """
+    # 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
+    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."
+        LOG.warning(msg, BCRYPT_MAX_LENGTH)
+        max_length = BCRYPT_MAX_LENGTH
+    else:
+        max_length = CONF.identity.max_password_length
+
     try:
         if len(password) > max_length:
             if CONF.strict_password_check:
diff --git a/keystone/conf/identity.py b/keystone/conf/identity.py
index 0dffe58d6..5cce78cf9 100644
--- a/keystone/conf/identity.py
+++ b/keystone/conf/identity.py
@@ -99,7 +99,11 @@ max_password_length = cfg.IntOpt(
     max=passlib.utils.MAX_PASSWORD_SIZE,
     help=utils.fmt("""
 Maximum allowed length for user passwords. Decrease this value to improve
-performance. Changing this value does not effect existing passwords.
+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.
 """))
 
 list_limit = cfg.IntOpt(
diff --git a/keystone/tests/unit/common/test_utils.py b/keystone/tests/unit/common/test_utils.py
index 39962b4f6..8b8280dc9 100644
--- a/keystone/tests/unit/common/test_utils.py
+++ b/keystone/tests/unit/common/test_utils.py
@@ -134,6 +134,17 @@ class UtilsTestCase(unit.BaseTestCase):
                           common_utils.hash_password,
                           invalid_length_password)
 
+    def test_max_algo_length_truncates_password(self):
+        self.config_fixture.config(strict_password_check=True)
+        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
+        self.assertRaises(exception.PasswordVerificationError,
+                          common_utils.hash_password,
+                          invalid_length_password)
+
     def _create_test_user(self, password=OPTIONAL):
         user = {"name": "hthtest"}
         if password is not self.OPTIONAL:
diff --git a/releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml b/releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml
new file mode 100644
index 000000000..003dc47df
--- /dev/null
+++ b/releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml
@@ -0,0 +1,9 @@
+---
+security:
+  - |
+    Passwords will now be automatically truncated if the max_password_length is
+    greater than the allowed length for the selected password hashing
+    algorithm. Currently only bcrypt has fixed allowed lengths defined which is
+    54 characters. A warning will be generated in the log if a password is
+    truncated.  This will not affect existing passwords, however only the first
+    54 characters of existing bcrypt passwords will be validated.
diff --git a/tox.ini b/tox.ini
index 65baa761a..4a168a9e1 100644
--- a/tox.ini
+++ b/tox.ini
@@ -109,6 +109,9 @@ enable-extensions = H203,H904
 ignore = D100,D101,D102,D103,D104,D106,D107,D203,D401,E305,E402,H211,H214,W503,W504,W605
 exclude = .venv,.git,.tox,build,dist,*lib/python*,*egg,tools,vendor,.update-venv,*.ini,*.po,*.pot
 max-complexity = 24
+per-file-ignores =
+# URL lines too long
+    keystone/common/password_hashing.py: E501
 
 [testenv:docs]
 deps =