openstack
/
keystone
Code Issues Proposed changes

Force algo specific maximum length & Properly trimm bcrypt hashed passwords 

This is the squash of 2 patches related to bcrypt hashing settings.

1.
Force algo specific maximum length

The bcrypt algorithm that we use for password hashing silently
length limits the size of the password that is hashed giving the
user a false sense of security [0].  This patch adds a check
in the verify_length_and_trunc_password function for the hash in
use and updates the max_length accordingly, this will override
the configured value and log a warning if the password is truncated.

Conflicts:
* tox.ini

[0]: https://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#security-issues

2.
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
original change id: Iea95a3c2df041a0046647b3d3dadead1a6d054d1
(cherry picked from commit 6730c761d1)
(cherry picked from commit 65f1fb6b4a)

Closes-bug: #1901891
Change-Id: I8d0bb2438b23227b5a66b94af6f8e198084fcd8d
(cherry picked from commit 3288af579d)
(cherry picked from commit 1b3536a7a4)
This commit is contained in:
Dave Wilde (d34dh0r53) 2022-02-09 11:28:59 -06:00 committed by elajkat
parent 7c96280555
commit 7852ca24a4
6 changed files with 64 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
keystone/common/password_hashing.py
View File

@ -57,19 +57,38 @@ 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 = 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."
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:
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')
@ -82,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)
@ -99,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)

6
keystone/conf/identity.py
View File

@ -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 72 bytes.
"""))
list_limit = cfg.IntOpt(

15
keystone/tests/unit/common/test_utils.py
View File

@ -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)
@ -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='96')
invalid_length_password = '0' * 96
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:

7
releasenotes/notes/bcrypt_truncation_fix-674dc5d7f1e776f2.yaml Normal file
View File

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

9
releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml Normal file
View File

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

3
tox.ini
View File

@ -119,6 +119,9 @@ enable-extensions = H203,H904
ignore = D100,D101,D102,D103,D104,D203,E402,W503,W504
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 =