From 8ad765e0230ceeb5ca7c36ec3ed6d25c57b22c9d Mon Sep 17 00:00:00 2001 From: Morgan Fainberg Date: Mon, 27 Feb 2017 13:06:07 -0800 Subject: [PATCH] Support new hashing algorithms for securely storing password hashes Support bcrypt, pbkdf2_sha512, or scrypt in password hashing for passwords managed within keystone. sha512_crypt is insufficient to hash passwords in a secure way for storage in the DB. Keystone defaults now to using bcrypt but can handle scrypt and pbkdf2_sha512 with a number of tuning options if desired. Closes-bug: #1543048 Closes-bug: #1668503 Change-Id: Id05026720839d94de26d0e44631deb34bcc0e610 --- keystone/common/password_hashing.py | 130 ++++++++++++++++++ ...password_column_for_expanded_hash_sizes.py | 15 ++ ...password_column_for_expanded_hash_sizes.py | 15 ++ ...password_column_for_expanded_hash_sizes.py | 25 ++++ keystone/common/utils.py | 54 +------- keystone/conf/default.py | 7 + keystone/conf/identity.py | 82 +++++++++++ keystone/identity/backends/sql.py | 19 +-- keystone/identity/backends/sql_model.py | 56 ++++++-- keystone/tests/unit/core.py | 8 ++ .../tests/unit/identity/test_backend_sql.py | 70 +++++++++- keystone/tests/unit/test_backend_sql.py | 1 + ..._1543048_and_1668503-7ead4e15faaab778.yaml | 43 ++++++ requirements.txt | 2 + 14 files changed, 456 insertions(+), 71 deletions(-) create mode 100644 keystone/common/password_hashing.py create mode 100644 keystone/common/sql/contract_repo/versions/023_contract_add_second_password_column_for_expanded_hash_sizes.py create mode 100644 keystone/common/sql/data_migration_repo/versions/023_migrate_add_second_password_column_for_expanded_hash_sizes.py create mode 100644 keystone/common/sql/expand_repo/versions/023_expand_add_second_password_column_for_expanded_hash_sizes.py create mode 100644 releasenotes/notes/bug_1543048_and_1668503-7ead4e15faaab778.yaml diff --git a/keystone/common/password_hashing.py b/keystone/common/password_hashing.py new file mode 100644 index 0000000000..8dcc9c173b --- /dev/null +++ b/keystone/common/password_hashing.py @@ -0,0 +1,130 @@ +# Copyright 2017 Red Hat +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import itertools + +from oslo_log import log +import passlib.hash + +import keystone.conf +from keystone import exception +from keystone.i18n import _ + + +CONF = keystone.conf.CONF +LOG = log.getLogger(__name__) + +SUPPORTED_HASHERS = frozenset([passlib.hash.bcrypt, + passlib.hash.scrypt, + passlib.hash.pbkdf2_sha512, + passlib.hash.sha512_crypt]) + +_HASHER_NAME_MAP = {hasher.name: hasher for hasher in SUPPORTED_HASHERS} + + +# NOTE(notmorgan): Build the list of prefixes. This comprehension builds +# a dictionary where the keys are the prefix (all hashedpasswords are +# '$$$') so we can do a fast-lookup on the hasher to +# use. If has hasher has multiple ident options it is encoded in the +# .ident_values attribute whereas hashers that have a single option +# (sha512_crypt) only has the .ident attribute. +_HASHER_IDENT_MAP = { + prefix: module for module, prefix in itertools.chain( + *[zip([mod] * len(getattr(mod, 'ident_values', (mod.ident,))), + getattr(mod, 'ident_values', (mod.ident,))) + for mod in SUPPORTED_HASHERS])} + + +def _get_hasher_from_ident(hashed): + try: + return _HASHER_IDENT_MAP[hashed[0:hashed.index('$', 1) + 1]] + except KeyError: + raise ValueError( + _('Unsupported password hashing algorithm ident: %s') % + hashed[0:hashed.index('$', 1) + 1]) + + +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 + try: + if len(password) > 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] + else: + return password + except TypeError: + raise exception.ValidationError(attribute='string', target='password') + + +def check_password(password, hashed): + """Check that a plaintext password matches hashed. + + hashpw returns the salt value concatenated with the actual hash value. + It extracts the actual salt if this value is then passed as the salt. + + """ + if password is None or hashed is None: + return False + password_utf8 = verify_length_and_trunc_password(password).encode('utf-8') + hasher = _get_hasher_from_ident(hashed) + return hasher.verify(password_utf8, hashed) + + +def hash_user_password(user): + """Hash a user dict's password without modifying the passed-in dict.""" + password = user.get('password') + if password is None: + return user + + return dict(user, password=hash_password(password)) + + +def hash_password_compat(password): + password_utf8 = verify_length_and_trunc_password(password).encode('utf-8') + return passlib.hash.sha512_crypt.using(rounds=CONF.crypt_strength).hash( + password_utf8) + + +def hash_password(password): + """Hash a password. Harder.""" + params = {} + password_utf8 = verify_length_and_trunc_password(password).encode('utf-8') + conf_hasher = CONF.identity.password_hash_algorithm + hasher = _HASHER_NAME_MAP.get(conf_hasher) + + if hasher is None: + raise RuntimeError( + _('Password Hash Algorithm %s not found') % + CONF.identity.password_hash_algorithm) + + if CONF.identity.password_hash_rounds: + params['rounds'] = CONF.identity.password_hash_rounds + if hasher is passlib.hash.scrypt: + if CONF.identity.scrypt_block_size: + params['block_size'] = CONF.identity.scrypt_block_size + if CONF.identity.scrypt_parallelism: + params['parallelism'] = CONF.identity.scrypt_parallelism + if CONF.identity.salt_bytesize: + params['salt_size'] = CONF.identity.salt_bytesize + if hasher is passlib.hash.pbkdf2_sha512: + if CONF.identity.salt_bytesize: + params['salt_size'] = CONF.identity.salt_bytesize + + return hasher.using(**params).hash(password_utf8) diff --git a/keystone/common/sql/contract_repo/versions/023_contract_add_second_password_column_for_expanded_hash_sizes.py b/keystone/common/sql/contract_repo/versions/023_contract_add_second_password_column_for_expanded_hash_sizes.py new file mode 100644 index 0000000000..8aa15c1ef2 --- /dev/null +++ b/keystone/common/sql/contract_repo/versions/023_contract_add_second_password_column_for_expanded_hash_sizes.py @@ -0,0 +1,15 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +def upgrade(migrate_engine): + pass diff --git a/keystone/common/sql/data_migration_repo/versions/023_migrate_add_second_password_column_for_expanded_hash_sizes.py b/keystone/common/sql/data_migration_repo/versions/023_migrate_add_second_password_column_for_expanded_hash_sizes.py new file mode 100644 index 0000000000..8aa15c1ef2 --- /dev/null +++ b/keystone/common/sql/data_migration_repo/versions/023_migrate_add_second_password_column_for_expanded_hash_sizes.py @@ -0,0 +1,15 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +def upgrade(migrate_engine): + pass diff --git a/keystone/common/sql/expand_repo/versions/023_expand_add_second_password_column_for_expanded_hash_sizes.py b/keystone/common/sql/expand_repo/versions/023_expand_add_second_password_column_for_expanded_hash_sizes.py new file mode 100644 index 0000000000..ebd2b8bbfc --- /dev/null +++ b/keystone/common/sql/expand_repo/versions/023_expand_add_second_password_column_for_expanded_hash_sizes.py @@ -0,0 +1,25 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import sqlalchemy as sql + + +def upgrade(migrate_engine): + meta = sql.MetaData() + meta.bind = migrate_engine + + # NOTE(notmorgan): To support the full range of scrypt and pbkfd password + # hash lengths, this should be closer to varchar(1500) instead of + # varchar(255). + password_hash = sql.Column('password_hash', sql.String(255), nullable=True) + password_table = sql.Table('password', meta, autoload=True) + password_table.create_column(password_hash) diff --git a/keystone/common/utils.py b/keystone/common/utils.py index b113bd094e..794fd60ec4 100644 --- a/keystone/common/utils.py +++ b/keystone/common/utils.py @@ -30,10 +30,10 @@ from oslo_serialization import jsonutils from oslo_utils import reflection from oslo_utils import strutils from oslo_utils import timeutils -import passlib.hash import six from six import moves +from keystone.common import password_hashing import keystone.conf from keystone import exception from keystone.i18n import _ @@ -53,6 +53,12 @@ WHITELISTED_PROPERTIES = [ # used for ID transformation. RESOURCE_ID_NAMESPACE = uuid.UUID('4332ecab-770b-4288-a680-b9aca3b1b153') +# Compatibilty for password hashing functions. +verify_length_and_trunc_password = password_hashing.verify_length_and_trunc_password # noqa +hash_password = password_hashing.hash_password +hash_user_password = password_hashing.hash_user_password +check_password = password_hashing.check_password + def resource_uuid(value): """Convert input to valid UUID hex digits.""" @@ -94,23 +100,6 @@ class SmarterEncoder(jsonutils.json.JSONEncoder): return super(SmarterEncoder, self).default(obj) -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 - try: - if len(password) > 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] - else: - return password - except TypeError: - raise exception.ValidationError(attribute='string', target='password') - - def hash_access_key(access): hash_ = hashlib.sha256() if not isinstance(access, six.binary_type): @@ -119,35 +108,6 @@ def hash_access_key(access): return hash_.hexdigest() -def hash_user_password(user): - """Hash a user dict's password without modifying the passed-in dict.""" - password = user.get('password') - if password is None: - return user - - return dict(user, password=hash_password(password)) - - -def hash_password(password): - """Hash a password. Hard.""" - password_utf8 = verify_length_and_trunc_password(password).encode('utf-8') - return passlib.hash.sha512_crypt.hash( - password_utf8, rounds=CONF.crypt_strength) - - -def check_password(password, hashed): - """Check that a plaintext password matches hashed. - - hashpw returns the salt value concatenated with the actual hash value. - It extracts the actual salt if this value is then passed as the salt. - - """ - if password is None or hashed is None: - return False - password_utf8 = verify_length_and_trunc_password(password).encode('utf-8') - return passlib.hash.sha512_crypt.verify(password_utf8, hashed) - - def attr_as_boolean(val_attr): """Return the boolean value, decoded from a string. diff --git a/keystone/conf/default.py b/keystone/conf/default.py index a0cd74a570..9773ad766b 100644 --- a/keystone/conf/default.py +++ b/keystone/conf/default.py @@ -121,6 +121,13 @@ crypt_strength = cfg.IntOpt( default=10000, min=1000, max=100000, + deprecated_since=versionutils.deprecated.PIKE, + deprecated_reason=utils.fmt(""" +sha512_crypt is insufficient for password hashes, use of bcrypt, pbkfd2_sha512 +and scrypt are now supported. Options are located in the [identity] config +block. This option is still used for rolling upgrade compatibility password +hashing. +"""), help=utils.fmt(""" The value passed as the keyword "rounds" to passlib's encrypt method. This option represents a trade off between security and performance. Higher values diff --git a/keystone/conf/identity.py b/keystone/conf/identity.py index 2abf09d8cb..089b92fb1b 100644 --- a/keystone/conf/identity.py +++ b/keystone/conf/identity.py @@ -11,6 +11,7 @@ # under the License. from oslo_config import cfg +from oslo_log import versionutils import passlib.utils from keystone.conf import utils @@ -108,6 +109,81 @@ list_limit = cfg.IntOpt( Maximum number of entities that will be returned in an identity collection. """)) +password_hash_algorithm = cfg.StrOpt( + 'password_hash_algorithm', + choices=['bcrypt', 'scrypt', 'pbkdf2_sha512'], + default='bcrypt', + help=utils.fmt(""" +The password hashing algorithm to use for passwords stored within keystone. +""")) + +password_hash_rounds = cfg.IntOpt( + 'password_hash_rounds', + help=utils.fmt(""" +This option represents a trade off between security and performance. Higher +values lead to slower performance, but higher security. Changing this option +will only affect newly created passwords as existing password hashes already +have a fixed number of rounds applied, so it is safe to tune this option in a +running cluster. + +The default for bcrypt is 12, must be between 4 and 31, inclusive. + +The default for scrypt is 16, must be within `range(1,32)`. + +The default for pbkdf_sha512 is 60000, must be within `range(1,1<<32)` + +WARNING: If using scrypt, increasing this value increases BOTH time AND +memory requirements to hash a password. +""")) + +salt_bytesize = cfg.IntOpt( + 'salt_bytesize', + min=0, + max=96, + help=utils.fmt(""" +Number of bytes to use in scrypt and pbkfd2_sha512 hashing salt. + +Default for scrypt is 16 bytes. +Default for pbkfd2_sha512 is 16 bytes. + +Limited to a maximum of 96 bytes due to the size of the column used to store +password hashes. +""")) + +scrypt_block_size = cfg.IntOpt( + 'scrypt_block_size', + help=utils.fmt(""" +Optional block size to pass to scrypt hash function (the `r` parameter). +Useful for tuning scrypt to optimal performance for your CPU architecture. +This option is only used when the `password_hash_algorithm` option is set +to `scrypt`. Defaults to 8. +""")) + +scrypt_paralellism = cfg.IntOpt( + 'scrypt_parallelism', + help=utils.fmt(""" +Optional parallelism to pass to scrypt hash function (the `p` parameter). +This option is only used when the `password_hash_algorithm` option is set +to `scrypt`. Defaults to 1. +""")) + +# TODO(notmorgan): remove this option in Q release. +rolling_upgrade_password_hash_compat = cfg.BoolOpt( + 'rolling_upgrade_password_hash_compat', + default=False, + deprecated_since=versionutils.deprecated.PIKE, + deprecated_reason='Only used for rolling-upgrade between Ocata and Pike', + help=utils.fmt(""" +This option tells keystone to continue to hash passwords with the sha512_crypt +algorithm for supporting rolling upgrades. sha512_crypt is typically more +insecure than bcrypt, pbkdf2, and scrypt. This option should be set to +`False` except in the case of performing a rolling upgrade where some +Keystone servers may not know how to verify non-sha512_crypt based password +hashes. + +This option will be removed in the Queens release and is only to support +rolling upgrades from Ocata release to Pike release. +""")) GROUP_NAME = __name__.split('.')[-1] ALL_OPTS = [ @@ -120,6 +196,12 @@ ALL_OPTS = [ cache_time, max_password_length, list_limit, + password_hash_algorithm, + password_hash_rounds, + scrypt_block_size, + scrypt_paralellism, + salt_bytesize, + rolling_upgrade_password_hash_compat, ] diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py index ad5b016b23..ae23ebad76 100644 --- a/keystone/identity/backends/sql.py +++ b/keystone/identity/backends/sql.py @@ -18,9 +18,9 @@ from oslo_db import api as oslo_db_api import sqlalchemy from keystone.common import driver_hints +from keystone.common import password_hashing from keystone.common import resource_options from keystone.common import sql -from keystone.common import utils import keystone.conf from keystone import exception from keystone.i18n import _ @@ -52,7 +52,7 @@ class Identity(base.IdentityDriverBase): https://blueprints.launchpad.net/keystone/+spec/sql-identiy-pam """ - return utils.check_password(password, user_ref.password) + return password_hashing.check_password(password, user_ref.password) # Identity interface def authenticate(self, user_id, password): @@ -125,7 +125,6 @@ class Identity(base.IdentityDriverBase): @sql.handle_conflicts(conflict_type='user') def create_user(self, user_id, user): - user = utils.hash_user_password(user) with sql.session_for_write() as session: user_ref = model.User.from_dict(user) if self._change_password_required(user_ref): @@ -206,7 +205,6 @@ class Identity(base.IdentityDriverBase): with sql.session_for_write() as session: user_ref = self._get_user(session, user_id) old_user_dict = user_ref.to_dict() - user = utils.hash_user_password(user) for k in user: old_user_dict[k] = user[k] new_user = model.User.from_dict(old_user_dict) @@ -223,8 +221,11 @@ class Identity(base.IdentityDriverBase): resource_options.resource_options_ref_to_mapper( user_ref, model.UserOption) - if 'password' in user and self._change_password_required(user_ref): - user_ref.password_ref.expires_at = datetime.datetime.utcnow() + if 'password' in user: + user_ref.password = user['password'] + if self._change_password_required(user_ref): + expires_now = datetime.datetime.utcnow() + user_ref.password_ref.expires_at = expires_now user_ref.extra = new_user.extra return base.filter_user( @@ -238,7 +239,9 @@ class Identity(base.IdentityDriverBase): # Validate the new password against the remaining passwords. if unique_cnt > 1: for password_ref in user_ref.local_user.passwords: - if utils.check_password(password, password_ref.password): + if password_hashing.check_password( + password, + password_ref.password_hash or password_ref.password): raise exception.PasswordHistoryValidationError( unique_count=unique_cnt) @@ -248,7 +251,7 @@ class Identity(base.IdentityDriverBase): if user_ref.password_ref and user_ref.password_ref.self_service: self._validate_minimum_password_age(user_ref) self._validate_password_history(new_password, user_ref) - user_ref.password = utils.hash_password(new_password) + user_ref.password = new_password user_ref.password_ref.self_service = True def _validate_minimum_password_age(self, user_ref): diff --git a/keystone/identity/backends/sql_model.py b/keystone/identity/backends/sql_model.py index 6d998a34af..0079d2a08f 100644 --- a/keystone/identity/backends/sql_model.py +++ b/keystone/identity/backends/sql_model.py @@ -19,6 +19,7 @@ from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy import orm from sqlalchemy.orm import collections +from keystone.common import password_hashing from keystone.common import resource_options from keystone.common import sql import keystone.conf @@ -32,7 +33,7 @@ class User(sql.ModelBase, sql.ModelDictMixinWithExtras): __tablename__ = 'user' attributes = ['id', 'name', 'domain_id', 'password', 'enabled', 'default_project_id', 'password_expires_at'] - readonly_attributes = ['id', 'password_expires_at'] + readonly_attributes = ['id', 'password_expires_at', 'password'] resource_options_registry = iro.USER_OPTIONS_REGISTRY id = sql.Column(sql.String(64), primary_key=True) domain_id = sql.Column(sql.String(64), nullable=False) @@ -104,7 +105,10 @@ class User(sql.ModelBase, sql.ModelDictMixinWithExtras): def password(self): """Return the current password.""" if self.password_ref: - return self.password_ref.password + if self.password_ref.password_hash is not None: + return self.password_ref.password_hash + else: + return self.password_ref.password return None @property @@ -124,7 +128,7 @@ class User(sql.ModelBase, sql.ModelDictMixinWithExtras): @property def password_is_expired(self): """Return whether password is expired or not.""" - if self.password_expires_at: + if self.password_expires_at and not self._password_expiry_exempt(): return datetime.datetime.utcnow() >= self.password_expires_at return False @@ -142,23 +146,44 @@ class User(sql.ModelBase, sql.ModelDictMixinWithExtras): if not ref.expires_at or ref.expires_at > now: ref.expires_at = now new_password_ref = Password() - new_password_ref.password = value + + hashed_passwd = None + hashed_compat = None + if value is not None: + # NOTE(notmorgan): hash the passwords, never directly bind the + # "value" in the unhashed form to hashed_passwd or hashed_compat + # to ensure the unhashed password cannot end up in the db. If an + # unhashed password ends up in the DB, it cannot be used for auth, + # it is however incorrect and could leak user credentials (due to + # users doing insecure things such as sharing passwords across + # different systems) to unauthorized parties. + hashed_passwd = password_hashing.hash_password(value) + + # TODO(notmorgan): Remove this compat code in Q release. + if CONF.identity.rolling_upgrade_password_hash_compat: + hashed_compat = password_hashing.hash_password_compat(value) + + new_password_ref.password_hash = hashed_passwd + new_password_ref.password = hashed_compat new_password_ref.created_at = now new_password_ref.expires_at = self._get_password_expires_at(now) self.local_user.passwords.append(new_password_ref) - def _get_password_expires_at(self, created_at): - expires_days = CONF.security_compliance.password_expires_days + def _password_expiry_exempt(self): # Get the IGNORE_PASSWORD_EXPIRY_OPT value from the user's # option_mapper. - ignore_pw_expiry = getattr( + return getattr( self.get_resource_option(iro.IGNORE_PASSWORD_EXPIRY_OPT.option_id), 'option_value', False) - if not ignore_pw_expiry and expires_days: - expired_date = (created_at + - datetime.timedelta(days=expires_days)) - return expired_date.replace(microsecond=0) + + def _get_password_expires_at(self, created_at): + expires_days = CONF.security_compliance.password_expires_days + if not self._password_expiry_exempt(): + if expires_days: + expired_date = (created_at + + datetime.timedelta(days=expires_days)) + return expired_date.replace(microsecond=0) return None @password.expression @@ -267,12 +292,17 @@ class LocalUser(sql.ModelBase, sql.ModelDictMixin): class Password(sql.ModelBase, sql.ModelDictMixin): __tablename__ = 'password' - attributes = ['id', 'local_user_id', 'password', 'created_at', - 'expires_at'] + attributes = ['id', 'local_user_id', 'password', 'password_hash', + 'created_at', 'expires_at'] id = sql.Column(sql.Integer, primary_key=True) local_user_id = sql.Column(sql.Integer, sql.ForeignKey('local_user.id', ondelete='CASCADE')) + # TODO(notmorgan): in the Q release the "password" field can be dropped as + # long as data migration exists to move the hashes over to the + # password_hash column if no value is in the password_hash column. password = sql.Column(sql.String(128), nullable=True) + password_hash = sql.Column(sql.String(255), nullable=True) + # created_at default set here to safe guard in case it gets missed created_at = sql.Column(sql.DateTime, nullable=False, default=datetime.datetime.utcnow) diff --git a/keystone/tests/unit/core.py b/keystone/tests/unit/core.py index 7e68626225..582357af28 100644 --- a/keystone/tests/unit/core.py +++ b/keystone/tests/unit/core.py @@ -620,6 +620,14 @@ class TestCase(BaseTestCase): 'keystone.notifications=INFO', 'keystone.identity.backends.ldap.common=INFO', ]) + # NOTE(notmorgan): Set password rounds low here to ensure speedy + # tests. This is explicitly set because the tests here are not testing + # the integrity of the password hashing, just that the correct form + # of hashing has been used. Note that 4 is the lowest for bcrypt + # allowed in the `[identity] password_hash_rounds` setting + self.config_fixture.config(group='identity', password_hash_rounds=4) + self.config_fixture.config(crypt_strength=1000) + self.useFixture( ksfixtures.KeyRepository( self.config_fixture, diff --git a/keystone/tests/unit/identity/test_backend_sql.py b/keystone/tests/unit/identity/test_backend_sql.py index da0a9d593f..ccb4057ec1 100644 --- a/keystone/tests/unit/identity/test_backend_sql.py +++ b/keystone/tests/unit/identity/test_backend_sql.py @@ -14,11 +14,12 @@ import datetime import uuid import freezegun +import passlib.hash from keystone.common import controller +from keystone.common import password_hashing from keystone.common import resource_options from keystone.common import sql -from keystone.common import utils import keystone.conf from keystone import exception from keystone.identity.backends import base @@ -30,6 +31,71 @@ from keystone.tests.unit import test_backend_sql CONF = keystone.conf.CONF +class UserPasswordHashingTestsNoCompat(test_backend_sql.SqlTests): + def config_overrides(self): + super(UserPasswordHashingTestsNoCompat, self).config_overrides() + self.config_fixture.config(group='identity', + password_hash_algorithm='scrypt') + + def test_password_hashing_compat_not_set_used(self): + with sql.session_for_read() as session: + user_ref = self.identity_api._get_user(session, + self.user_foo['id']) + self.assertIsNone(user_ref.password_ref.password) + self.assertIsNotNone(user_ref.password_ref.password_hash) + self.assertEqual(user_ref.password, + user_ref.password_ref.password_hash) + self.assertTrue(password_hashing.check_password( + self.user_foo['password'], user_ref.password)) + + def test_configured_algorithm_used(self): + with sql.session_for_read() as session: + user_ref = self.identity_api._get_user(session, + self.user_foo['id']) + self.assertEqual( + passlib.hash.scrypt, + password_hashing._get_hasher_from_ident(user_ref.password)) + + +class UserPasswordHashingTestsWithCompat(test_backend_sql.SqlTests): + def config_overrides(self): + super(UserPasswordHashingTestsWithCompat, self).config_overrides() + self.config_fixture.config( + group='identity', + rolling_upgrade_password_hash_compat=True) + + def test_compat_password_hashing(self): + with sql.session_for_read() as session: + user_ref = self.identity_api._get_user(session, + self.user_foo['id']) + self.assertIsNotNone(user_ref.password_ref.password) + self.assertIsNotNone(user_ref.password_ref.password_hash) + self.assertEqual(user_ref.password, + user_ref.password_ref.password_hash) + self.assertNotEqual(user_ref.password, + user_ref.password_ref.password) + self.assertTrue(password_hashing.check_password( + self.user_foo['password'], user_ref.password)) + self.assertTrue(password_hashing.check_password( + self.user_foo['password'], user_ref.password_ref.password)) + + def test_user_with_compat_password_hash_only(self): + with sql.session_for_write() as session: + user_ref = self.identity_api._get_user(session, + self.user_foo['id']) + user_ref.password_ref.password_hash = None + + with sql.session_for_read() as session: + user_ref = self.identity_api._get_user(session, + self.user_foo['id']) + + self.assertIsNone(user_ref.password_ref.password_hash) + self.assertIsNotNone(user_ref.password) + self.assertEqual(user_ref.password, user_ref.password_ref.password) + self.assertTrue(password_hashing.check_password( + self.user_foo['password'], user_ref.password)) + + class UserResourceOptionTests(test_backend_sql.SqlTests): def setUp(self): super(UserResourceOptionTests, self).setUp() @@ -188,7 +254,6 @@ class UserResourceOptionTests(test_backend_sql.SqlTests): def _create_user(self, user_dict): user_dict['id'] = uuid.uuid4().hex - user_dict = utils.hash_user_password(user_dict) with sql.session_for_write() as session: user_ref = model.User.from_dict(user_dict) session.add(user_ref) @@ -316,7 +381,6 @@ class DisableInactiveUserTests(test_backend_sql.SqlTests): def _create_user(self, user_dict, last_active_at): user_dict['id'] = uuid.uuid4().hex - user_dict = utils.hash_user_password(user_dict) with sql.session_for_write() as session: user_ref = model.User.from_dict(user_dict) user_ref.last_active_at = last_active_at diff --git a/keystone/tests/unit/test_backend_sql.py b/keystone/tests/unit/test_backend_sql.py index 93e804f924..f6c15dbf33 100644 --- a/keystone/tests/unit/test_backend_sql.py +++ b/keystone/tests/unit/test_backend_sql.py @@ -154,6 +154,7 @@ class SqlModels(SqlTests): cols = (('id', sql.Integer, None), ('local_user_id', sql.Integer, None), ('password', sql.String, 128), + ('password_hash', sql.String, 255), ('created_at', sql.DateTime, None), ('expires_at', sql.DateTime, None), ('self_service', sql.Boolean, False)) diff --git a/releasenotes/notes/bug_1543048_and_1668503-7ead4e15faaab778.yaml b/releasenotes/notes/bug_1543048_and_1668503-7ead4e15faaab778.yaml new file mode 100644 index 0000000000..6595d7ce63 --- /dev/null +++ b/releasenotes/notes/bug_1543048_and_1668503-7ead4e15faaab778.yaml @@ -0,0 +1,43 @@ +--- +features: + - | + * [`bug 1543048 `_] + [`bug 1668503 `_] + Keystone now supports multiple forms of password hashing. Notably bcrypt, + scrypt, and pbkdf2_sha512. The options are now located in the + `[identity]` section of the configuration file. To set the algorithm + use `[identity] password_hash_algorithm`. To set the number of rounds + (time-complexity, and memory-use in the case of scrypt) use + `[identity] password_hash_rounds`. `scrypt` and `pbkdf2_sha512` have + further tuning options available. Keystone now defaults to using + `bcrypt` as the hashing algorithm. All passwords will continue to + function with the old sha512_crypt hash, but new password hashes + will be bcrypt. +upgrade: + - | + * If performing rolling upgrades, set + `[identity] rolling_upgrade_password_hash_compat` to `True`. This will + instruct keystone to continue to hash passwords in a manner that older + (pre Pike release) keystones can still verify passwords. Once all + upgrades are complete, ensure this option is set back to `False`. +deprecations: + - | + * `[DEFAULT] crypt_strength` is deprecated in favor of + `[identity] password_hash_rounds`. Note that `[DEFAULT] crypt_strength` + is still used when `[identity] rolling_upgrade_password_hash_compat` is + set to `True`. +security: + - | + * The use of `sha512_crypt` is considered inadequate for password hashing + in an application like Keystone. The use of bcrypt or scrypt is + recommended to ensure protection against password cracking utilities if + the hashes are exposed. This is due to Time-Complexity requirements for + computing the hashes in light of modern hardware (CPU, GPU, ASIC, FPGA, + etc). Keystone has moved to bcrypt as a default and no longer hashes + new passwords (and password changes) with sha512_crypt. It is + recommended passwords be changed after upgrade to Pike. The risk of + password hash exposure is limited, but for the best possible + protection against cracking the hash it is recommended passwords be + changed after upgrade. The password change will then result in a more + secure hash (bcrypt by default) being used to store the password in the + DB. diff --git a/requirements.txt b/requirements.txt index 03a232a291..81f3bb8865 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,6 +20,8 @@ stevedore>=1.20.0 # Apache-2.0 passlib>=1.7.0 # BSD python-keystoneclient>=3.8.0 # Apache-2.0 keystonemiddleware>=4.12.0 # Apache-2.0 +bcrypt>=3.1.3 # Apache-2.0 +scrypt>=0.8.0 # BSD oslo.cache>=1.5.0 # Apache-2.0 oslo.concurrency>=3.8.0 # Apache-2.0 oslo.config>=3.22.0 # Apache-2.0