diff options
| -rw-r--r-- | .zuul.yaml | 17 | ||||
| -rw-r--r-- | devstack/files/oidc/apache_oidc.conf | 47 | ||||
| -rw-r--r-- | devstack/lib/oidc.sh | 160 | ||||
| -rw-r--r-- | devstack/plugin.sh | 29 | ||||
| -rw-r--r-- | devstack/tools/oidc/__init__.py | 0 | ||||
| -rw-r--r-- | devstack/tools/oidc/docker-compose.yaml | 33 | ||||
| -rw-r--r-- | devstack/tools/oidc/setup_keycloak_client.py | 61 | ||||
| -rw-r--r-- | keystone/common/password_hashing.py | 22 | ||||
| -rw-r--r-- | keystone/conf/identity.py | 6 | ||||
| -rw-r--r-- | keystone/identity/backends/ldap/common.py | 21 | ||||
| -rw-r--r-- | keystone/tests/unit/common/test_utils.py | 11 | ||||
| -rw-r--r-- | keystone/tests/unit/fakeldap.py | 9 | ||||
| -rw-r--r-- | keystone/tests/unit/test_backend_ldap_pool.py | 118 | ||||
| -rw-r--r-- | releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml | 9 | ||||
| -rw-r--r-- | tox.ini | 3 |
15 files changed, 532 insertions, 14 deletions
diff --git a/.zuul.yaml b/.zuul.yaml index 37b1a8c8a..4a3ccf244 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -202,6 +202,21 @@ name: keystone-dsvm-py35-functional-federation parent: keystone-dsvm-py35-functional-federation-ubuntu-xenial +# Experimental +- job: + name: keystone-dsvm-functional-oidc-federation + parent: keystone-dsvm-functional + vars: + devstack_localrc: + TEMPEST_PLUGINS: '/opt/stack/keystone-tempest-plugin' + USE_PYTHON3: True + OS_CACERT: '/opt/stack/data/ca_bundle.pem' + devstack_services: + tls-proxy: true + keystone-oidc-federation: true + devstack_plugins: + keystone: https://opendev.org/openstack/keystone + - project: templates: - openstack-cover-jobs @@ -279,3 +294,5 @@ irrelevant-files: *irrelevant-files - keystone-dsvm-py35-functional-federation-ubuntu-xenial: irrelevant-files: *irrelevant-files + - keystone-dsvm-functional-oidc-federation: + irrelevant-files: *irrelevant-files diff --git a/devstack/files/oidc/apache_oidc.conf b/devstack/files/oidc/apache_oidc.conf new file mode 100644 index 000000000..eab84fd07 --- /dev/null +++ b/devstack/files/oidc/apache_oidc.conf @@ -0,0 +1,47 @@ +# DO NOT USE THIS IN PRODUCTION ENVIRONMENTS! +OIDCSSLValidateServer Off +OIDCOAuthSSLValidateServer Off +OIDCCookieSameSite On + +OIDCClaimPrefix "OIDC-" +OIDCResponseType "id_token" +OIDCScope "openid email profile" +OIDCProviderMetadataURL "%OIDC_METADATA_URL%" +OIDCClientID "%OIDC_CLIENT_ID%" +OIDCClientSecret "%OIDC_CLIENT_SECRET%" +OIDCPKCEMethod "S256" +OIDCCryptoPassphrase "openstack" + +OIDCRedirectURI "https://%HOST_IP%/identity/v3/auth/OS-FEDERATION/identity_providers/%IDP_ID%/protocols/openid/websso" +OIDCRedirectURI "https://%HOST_IP%/identity/v3/auth/OS-FEDERATION/websso/openid" + +<LocationMatch "/v3/auth/OS-FEDERATION/websso/openid"> + AuthType "openid-connect" + Require valid-user + LogLevel debug +</LocationMatch> + +<LocationMatch "/v3/auth/OS-FEDERATION/identity_providers/%IDP_ID%/protocols/openid/websso"> + AuthType "openid-connect" + Require valid-user + LogLevel debug +</LocationMatch> + +<LocationMatch "/v3/auth/OS-FEDERATION/identity_providers/%IDP_ID%/protocols/openid/auth"> + AuthType "openid-connect" + Require valid-user + LogLevel debug +</LocationMatch> + +<Location ~ "/v3/OS-FEDERATION/identity_providers/%IDP_ID%/protocols/openid/auth"> + AuthType oauth20 + Require valid-user +</Location> + +OIDCOAuthClientID "%OIDC_CLIENT_ID%" +OIDCOAuthClientSecret "%OIDC_CLIENT_SECRET%" +OIDCOAuthIntrospectionEndpoint "%OIDC_INTROSPECTION_URL%" + +# Horizon favors the referrer to the Keystone URL that is set. +# https://github.com/openstack/horizon/blob/5e4ca1a9fdec04db08552e9e93fe372b8b8b45ae/openstack_auth/views.py#L192 +Header always set Referrer-Policy "no-referrer" diff --git a/devstack/lib/oidc.sh b/devstack/lib/oidc.sh new file mode 100644 index 000000000..ab8731d98 --- /dev/null +++ b/devstack/lib/oidc.sh @@ -0,0 +1,160 @@ +# 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. + +DOMAIN_NAME=${DOMAIN_NAME:-federated_domain} +PROJECT_NAME=${PROJECT_NAME:-federated_project} +GROUP_NAME=${GROUP_NAME:-federated_users} + +OIDC_CLIENT_ID=${CLIENT_ID:-devstack} +OIDC_CLIENT_SECRET=${OIDC_CLIENT_SECRET:-nomoresecret} + +OIDC_ISSUER=${OIDC_ISSUER:-"https://$HOST_IP:8443"} +OIDC_ISSUER_BASE="${OIDC_ISSUER}/realms/master" + +OIDC_METADATA_URL=${OIDC_METADATA_URL:-"https://$HOST_IP:8443/realms/master/.well-known/openid-configuration"} +OIDC_INTROSPECTION_URL=${OIDC_INTROSPECTION_URL:-"https://$HOST_IP:8443/realms/master/protocol/openid-connect/token/introspect"} + +IDP_ID=${IDP_ID:-sso} +IDP_USERNAME=${IDP_USERNAME:-admin} +IDP_PASSWORD=${IDP_PASSWORD:-nomoresecret} + +MAPPING_REMOTE_TYPE=${MAPPING_REMOTE_TYPE:-OIDC-preferred_username} +MAPPING_USER_NAME=${MAPPING_USER_NAME:-"{0}"} +PROTOCOL_ID=${PROTOCOL_ID:-openid} + +REDIRECT_URI="https://$HOST_IP/identity/v3/auth/OS-FEDERATION/identity_providers/$IDP_ID/protocols/openid/websso" + +OIDC_PLUGIN="$DEST/keystone/devstack" + +function install_federation { + if is_ubuntu; then + install_package libapache2-mod-auth-openidc + sudo a2enmod headers + install_package docker.io + install_package docker-compose + elif is_fedora; then + install_package mod_auth_openidc + install_package podman + install_package podman-docker + install_package docker-compose + sudo systemctl start podman.socket + else + echo "Skipping installation. Only supported on Ubuntu and RHEL based." + fi +} + +function configure_federation { + # Specify the header that contains information about the identity provider + iniset $KEYSTONE_CONF openid remote_id_attribute "HTTP_OIDC_ISS" + iniset $KEYSTONE_CONF auth methods "password,token,openid,application_credential" + iniset $KEYSTONE_CONF federation trusted_dashboard "https://$HOST_IP/auth/websso/" + + cp $DEST/keystone/etc/sso_callback_template.html /etc/keystone/ + + if [[ "$WSGI_MODE" == "uwsgi" ]]; then + restart_service "devstack@keystone" + fi + + if [[ "$OIDC_ISSUER_BASE" == "https://$HOST_IP:8443/realms/master" ]]; then + # Assuming we want to setup a local keycloak here. + sed -i "s#DEVSTACK_DEST#${DATA_DIR}#" ${OIDC_PLUGIN}/tools/oidc/docker-compose.yaml + sudo docker-compose --file ${OIDC_PLUGIN}/tools/oidc/docker-compose.yaml up -d + + # wait for the server to be up + attempt_counter=0 + max_attempts=100 + until $(curl --output /dev/null --silent --fail $OIDC_METADATA_URL); do + if [ ${attempt_counter} -eq ${max_attempts} ];then + echo "Keycloak server failed to come up in time" + exit 1 + fi + + attempt_counter=$(($attempt_counter+1)) + sleep 5 + done + + KEYCLOAK_URL="https://$HOST_IP:8443" \ + KEYCLOAK_USERNAME="admin" \ + KEYCLOAK_PASSWORD="nomoresecret" \ + HOST_IP="$HOST_IP" \ + python3 $OIDC_PLUGIN/tools/oidc/setup_keycloak_client.py + fi + + local keystone_apache_conf=$(apache_site_config_for keystone-wsgi-public) + cat $OIDC_PLUGIN/files/oidc/apache_oidc.conf | sudo tee -a $keystone_apache_conf + sudo sed -i -e " + s|%OIDC_CLIENT_ID%|$OIDC_CLIENT_ID|g; + s|%OIDC_CLIENT_SECRET%|$OIDC_CLIENT_SECRET|g; + s|%OIDC_METADATA_URL%|$OIDC_METADATA_URL|g; + s|%OIDC_INTROSPECTION_URL%|$OIDC_INTROSPECTION_URL|g; + s|%HOST_IP%|$HOST_IP|g; + s|%IDP_ID%|$IDP_ID|g; + " $keystone_apache_conf + + restart_apache_server +} + +function register_federation { + local federated_domain=$(get_or_create_domain $DOMAIN_NAME) + local federated_project=$(get_or_create_project $PROJECT_NAME $DOMAIN_NAME) + local federated_users=$(get_or_create_group $GROUP_NAME $DOMAIN_NAME) + local member_role=$(get_or_create_role Member) + + openstack role add --group $federated_users --domain $federated_domain $member_role + openstack role add --group $federated_users --project $federated_project $member_role + + openstack identity provider create \ + --remote-id $OIDC_ISSUER_BASE \ + --domain $DOMAIN_NAME $IDP_ID +} + +function configure_tests_settings { + # Here we set any settings that might be need by the fed_scenario set of tests + iniset $TEMPEST_CONFIG identity-feature-enabled federation True + + # we probably need an oidc version of this flag based on local oidc + iniset $TEMPEST_CONFIG identity-feature-enabled external_idp True + + # Identity provider settings + iniset $TEMPEST_CONFIG fed_scenario idp_id $IDP_ID + iniset $TEMPEST_CONFIG fed_scenario idp_remote_ids $OIDC_ISSUER_BASE + iniset $TEMPEST_CONFIG fed_scenario idp_username $IDP_USERNAME + iniset $TEMPEST_CONFIG fed_scenario idp_password $IDP_PASSWORD + iniset $TEMPEST_CONFIG fed_scenario idp_oidc_url $OIDC_ISSUER + iniset $TEMPEST_CONFIG fed_scenario idp_client_id $OIDC_CLIENT_ID + iniset $TEMPEST_CONFIG fed_scenario idp_client_secret $OIDC_CLIENT_SECRET + + # Mapping rules settings + iniset $TEMPEST_CONFIG fed_scenario mapping_remote_type $MAPPING_REMOTE_TYPE + iniset $TEMPEST_CONFIG fed_scenario mapping_user_name $MAPPING_USER_NAME + iniset $TEMPEST_CONFIG fed_scenario mapping_group_name $GROUP_NAME + iniset $TEMPEST_CONFIG fed_scenario mapping_group_domain_name $DOMAIN_NAME + iniset $TEMPEST_CONFIG fed_scenario enable_k2k_groups_mapping False + + # Protocol settings + iniset $TEMPEST_CONFIG fed_scenario protocol_id $PROTOCOL_ID +} + +function uninstall_federation { + # Ensure Keycloak is stopped and the containers are cleaned up + sudo docker-compose --file ${OIDC_PLUGIN}/tools/oidc/docker-compose.yaml down + if is_ubuntu; then + sudo docker rmi $(sudo docker images -a -q) + uninstall_package docker-compose + elif is_fedora; then + sudo podman rmi $(sudo podman images -a -q) + uninstall_package podman + else + echo "Skipping uninstallation of OIDC federation for non ubuntu nor fedora nor suse host" + fi +} + diff --git a/devstack/plugin.sh b/devstack/plugin.sh index 8f7a38535..eca1d1ac0 100644 --- a/devstack/plugin.sh +++ b/devstack/plugin.sh @@ -14,7 +14,13 @@ # under the License. KEYSTONE_PLUGIN=$DEST/keystone/devstack -source $KEYSTONE_PLUGIN/lib/federation.sh + +if is_service_enabled keystone-saml2-federation; then + source $KEYSTONE_PLUGIN/lib/federation.sh +elif is_service_enabled keystone-oidc-federation; then + source $KEYSTONE_PLUGIN/lib/oidc.sh +fi + source $KEYSTONE_PLUGIN/lib/scope.sh # For more information on Devstack plugins, including a more detailed @@ -25,6 +31,10 @@ if [[ "$1" == "stack" && "$2" == "install" ]]; then # This phase is executed after the projects have been installed echo "Keystone plugin - Install phase" if is_service_enabled keystone-saml2-federation; then + echo "installing saml2 federation" + install_federation + elif is_service_enabled keystone-oidc-federation; then + echo "installing oidc federation" install_federation fi @@ -33,6 +43,10 @@ elif [[ "$1" == "stack" && "$2" == "post-config" ]]; then # before they are started echo "Keystone plugin - Post-config phase" if is_service_enabled keystone-saml2-federation; then + echo "configuring saml2 federation" + configure_federation + elif is_service_enabled keystone-oidc-federation; then + echo "configuring oidc federation" configure_federation fi @@ -40,12 +54,21 @@ elif [[ "$1" == "stack" && "$2" == "extra" ]]; then # This phase is executed after the projects have been started echo "Keystone plugin - Extra phase" if is_service_enabled keystone-saml2-federation; then + echo "registering saml2 federation" + register_federation + elif is_service_enabled keystone-oidc-federation; then + echo "registering oidc federation" register_federation fi + elif [[ "$1" == "stack" && "$2" == "test-config" ]]; then # This phase is executed after Tempest was configured echo "Keystone plugin - Test-config phase" if is_service_enabled keystone-saml2-federation; then + echo "config tests settings for saml" + configure_tests_settings + elif is_service_enabled keystone-oidc-federation; then + echo "config tests settings for oidc" configure_tests_settings fi if [[ "$(trueorfalse False KEYSTONE_ENFORCE_SCOPE)" == "True" ]] ; then @@ -66,6 +89,10 @@ if [[ "$1" == "clean" ]]; then # Called by clean.sh after the "unstack" phase # Undo what was performed during the "install" phase if is_service_enabled keystone-saml2-federation; then + echo "uninstalling saml" + uninstall_federation + elif is_service_enabled keystone-oidc-federation; then + echo "uninstalling oidc" uninstall_federation fi fi diff --git a/devstack/tools/oidc/__init__.py b/devstack/tools/oidc/__init__.py new file mode 100644 index 000000000..e69de29bb --- /dev/null +++ b/devstack/tools/oidc/__init__.py diff --git a/devstack/tools/oidc/docker-compose.yaml b/devstack/tools/oidc/docker-compose.yaml new file mode 100644 index 000000000..6e4a428c9 --- /dev/null +++ b/devstack/tools/oidc/docker-compose.yaml @@ -0,0 +1,33 @@ +version: "3" + +services: + keycloak: + image: quay.io/keycloak/keycloak:latest + command: start-dev --log-level debug --log=console,file --https-certificate-file=/etc/certs/devstack-cert.pem --https-certificate-key-file=/etc/certs/devstack-cert.pem + container_name: oidc_keycloak_1 + environment: + KEYCLOAK_ADMIN: admin + KEYCLOAK_ADMIN_PASSWORD: nomoresecret + KEYCLOAK_USER: admin + KEYCLOAK_PASSWORD: nomoresecret + KEYCLOAK_LOG_LEVEL: DEBUG + DB_VENDOR: mariadb + DB_DATABASE: keycloak + DB_USER: keycloak + DB_PASSWORD: "nomoresecret" + DB_ADDR: "keycloak-database" + DB_PORT: "3306" + JAVA_OPTS: "-server -Xms128m -Xmx1024m -XX:MetaspaceSize=128M -XX:MaxMetaspaceSize=512m -Djava.net.preferIPv4Stack=true -Djboss.modules.system.pkgs=org.jboss.byteman -Djava.awt.headless=true" + ports: + - "8088:8080" # host:container + - "8443:8443" + volumes: + - DEVSTACK_DEST:/etc/certs:rw + + keycloak-database: + image: quay.io/metal3-io/mariadb:latest + environment: + MYSQL_ROOT_PASSWORD: nomoresecret + MYSQL_DATABASE: keycloak + MYSQL_USER: keycloak + MYSQL_PASSWORD: nomoresecret diff --git a/devstack/tools/oidc/setup_keycloak_client.py b/devstack/tools/oidc/setup_keycloak_client.py new file mode 100644 index 000000000..15fa37b41 --- /dev/null +++ b/devstack/tools/oidc/setup_keycloak_client.py @@ -0,0 +1,61 @@ +import os +import requests + +KEYCLOAK_USERNAME = os.environ.get('KEYCLOAK_USERNAME') +KEYCLOAK_PASSWORD = os.environ.get('KEYCLOAK_PASSWORD') +KEYCLOAK_URL = os.environ.get('KEYCLOAK_URL') +HOST_IP = os.environ.get('HOST_IP', 'localhost') + +class KeycloakClient(object): + def __init__(self): + self.session = requests.session() + + @staticmethod + def construct_url(realm, path): + return f'{KEYCLOAK_URL}/admin/realms/{realm}/{path}' + + @staticmethod + def token_endpoint(realm): + return f'{KEYCLOAK_URL}/realms/{realm}/protocol/openid-connect/token' + + def _admin_auth(self, realm): + params = { + 'grant_type': 'password', + 'client_id': 'admin-cli', + 'username': KEYCLOAK_USERNAME, + 'password': KEYCLOAK_PASSWORD, + 'scope': 'openid', + } + r = requests.post(self.token_endpoint(realm), data=params).json() + headers = { + 'Authorization': ("Bearer %s" % r['access_token']), + 'Content-Type': 'application/json' + } + self.session.headers.update(headers) + return r + + def create_client(self, realm, client_id, client_secret, redirect_uris): + self._admin_auth(realm) + data = { + 'clientId': client_id, + 'secret': client_secret, + 'redirectUris': redirect_uris, + 'implicitFlowEnabled': True, + 'directAccessGrantsEnabled': True, + } + return self.session.post(self.construct_url(realm, 'clients'), json=data) + + +def main(): + c = KeycloakClient() + + redirect_uris = [ + f'http://{HOST_IP}/identity/v3/auth/OS-FEDERATION/identity_providers/sso/protocols/openid/websso', + f'http://{HOST_IP}/identity/v3/auth/OS-FEDERATION/websso/openid' + ] + + c.create_client('master', 'devstack', 'nomoresecret', redirect_uris) + + +if __name__ == "__main__": + main() 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/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py index 1033a4efd..d9c07fd87 100644 --- a/keystone/identity/backends/ldap/common.py +++ b/keystone/identity/backends/ldap/common.py @@ -860,11 +860,22 @@ class PooledLDAPHandler(LDAPHandler): cleaned up when message.clean() is called. """ - results = message.connection.result3(message.id, all, timeout) - - # Now that we have the results from the LDAP server for the message, we - # don't need the the context manager used to create the connection. - message.clean() + # message.connection.result3 might throw an exception + # so the code must ensure that message.clean() is invoked + # regardless of the result3's result. Otherwise, the + # connection will be marked as active forever, which + # ultimately renders the pool unusable, causing a DoS. + try: + results = message.connection.result3(message.id, all, timeout) + except Exception: + # We don't want to ignore thrown + # exceptions, raise them + raise + finally: + # Now that we have the results from the LDAP server for + # the message, we don't need the the context manager used + # to create the connection. + message.clean() return results diff --git a/keystone/tests/unit/common/test_utils.py b/keystone/tests/unit/common/test_utils.py index 0391c8e61..673175aea 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/keystone/tests/unit/fakeldap.py b/keystone/tests/unit/fakeldap.py index f374322d1..5119305a7 100644 --- a/keystone/tests/unit/fakeldap.py +++ b/keystone/tests/unit/fakeldap.py @@ -296,6 +296,9 @@ class FakeLdap(common.LDAPHandler): raise ldap.SERVER_DOWN whos = ['cn=Admin', CONF.ldap.user] if (who in whos and cred in ['password', CONF.ldap.password]): + self.connected = True + self.who = who + self.cred = cred return attrs = self.db.get(self.key(who)) @@ -316,6 +319,9 @@ class FakeLdap(common.LDAPHandler): def unbind_s(self): """Provide for compatibility but this method is ignored.""" + self.connected = False + self.who = None + self.cred = None if server_fail: raise ldap.SERVER_DOWN @@ -534,7 +540,7 @@ class FakeLdap(common.LDAPHandler): raise exception.NotImplemented() # only passing a single server control is supported by this fake ldap - if len(serverctrls) > 1: + if serverctrls and len(serverctrls) > 1: raise exception.NotImplemented() # search_ext is async and returns an identifier used for @@ -589,6 +595,7 @@ class FakeLdapPool(FakeLdap): def __init__(self, uri, retry_max=None, retry_delay=None, conn=None): super(FakeLdapPool, self).__init__(conn=conn) self.url = uri + self._uri = uri self.connected = None self.conn = self self._connection_time = 5 # any number greater than 0 diff --git a/keystone/tests/unit/test_backend_ldap_pool.py b/keystone/tests/unit/test_backend_ldap_pool.py index 9b5e92748..1c4b19804 100644 --- a/keystone/tests/unit/test_backend_ldap_pool.py +++ b/keystone/tests/unit/test_backend_ldap_pool.py @@ -163,12 +163,23 @@ class LdapPoolCommonTestMixin(object): # Then open 3 connections again and make sure size does not grow # over 3 - with _get_conn() as _: # conn1 + with _get_conn() as c1: # conn1 + self.assertEqual(3, len(ldappool_cm)) + c1.connected = False + with _get_conn() as c2: # conn2 + self.assertEqual(3, len(ldappool_cm)) + c2.connected = False + with _get_conn() as c3: # conn3 + c3.connected = False + c3.unbind_ext_s() + self.assertEqual(3, len(ldappool_cm)) + + with _get_conn() as c1: # conn1 self.assertEqual(1, len(ldappool_cm)) - with _get_conn() as _: # conn2 + with _get_conn() as c2: # conn2 self.assertEqual(2, len(ldappool_cm)) - with _get_conn() as _: # conn3 - _.unbind_ext_s() + with _get_conn() as c3: # conn3 + c3.unbind_ext_s() self.assertEqual(3, len(ldappool_cm)) def test_password_change_with_pool(self): @@ -209,6 +220,105 @@ class LdapPoolCommonTestMixin(object): user_id=self.user_sna['id'], password=old_password) + @mock.patch.object(fakeldap.FakeLdap, 'search_ext') + def test_search_ext_ensure_pool_connection_released(self, mock_search_ext): + """Test search_ext exception resiliency. + + Call search_ext function in isolation. Doing so will cause + search_ext to borrow a connection from the pool and associate + it with an AsynchronousMessage object. Borrowed connection ought + to be released if anything goes wrong during LDAP API call. This + test case intentionally throws an exception to ensure everything + goes as expected when LDAP connection raises an exception. + """ + class CustomDummyException(Exception): + pass + + # Throw an exception intentionally when LDAP + # connection search_ext function is called + mock_search_ext.side_effect = CustomDummyException() + self.config_fixture.config(group='ldap', pool_size=1) + pool = self.conn_pools[CONF.ldap.url] + user_api = ldap.UserApi(CONF) + + # setUp primes the pool so pool + # must have one connection + self.assertEqual(1, len(pool)) + for i in range(1, 10): + handler = user_api.get_connection() + # Just to ensure that we're using pooled connections + self.assertIsInstance(handler.conn, common_ldap.PooledLDAPHandler) + # LDAP API will throw CustomDummyException. In this scenario + # we expect LDAP connection to be made available back to the + # pool. + self.assertRaises( + CustomDummyException, + lambda: handler.search_ext( + 'dc=example,dc=test', + 'dummy', + 'objectclass=*', + ['mail', 'userPassword'] + ) + ) + # Pooled connection must not be evicted from the pool + self.assertEqual(1, len(pool)) + # Ensure that the connection is inactive afterwards + with pool._pool_lock: + for slot, conn in enumerate(pool._pool): + self.assertFalse(conn.active) + + self.assertEqual(mock_search_ext.call_count, i) + + @mock.patch.object(fakeldap.FakeLdap, 'result3') + def test_result3_ensure_pool_connection_released(self, mock_result3): + """Test search_ext-->result3 exception resiliency. + + Call search_ext function, grab an AsynchronousMessage object and + call result3 with it. During the result3 call, LDAP API will throw + an exception.The expectation is that the associated LDAP pool + connection for AsynchronousMessage must be released back to the + LDAP connection pool. + """ + class CustomDummyException(Exception): + pass + + # Throw an exception intentionally when LDAP + # connection result3 function is called + mock_result3.side_effect = CustomDummyException() + self.config_fixture.config(group='ldap', pool_size=1) + pool = self.conn_pools[CONF.ldap.url] + user_api = ldap.UserApi(CONF) + + # setUp primes the pool so pool + # must have one connection + self.assertEqual(1, len(pool)) + for i in range(1, 10): + handler = user_api.get_connection() + # Just to ensure that we're using pooled connections + self.assertIsInstance(handler.conn, common_ldap.PooledLDAPHandler) + msg = handler.search_ext( + 'dc=example,dc=test', + 'dummy', + 'objectclass=*', + ['mail', 'userPassword'] + ) + # Connection is in use, must be already marked active + self.assertTrue(msg.connection.active) + # Pooled connection must not be evicted from the pool + self.assertEqual(1, len(pool)) + # LDAP API will throw CustomDummyException. In this + # scenario we expect LDAP connection to be made + # available back to the pool. + self.assertRaises( + CustomDummyException, + lambda: handler.result3(msg) + ) + # Connection must be set inactive + self.assertFalse(msg.connection.active) + # Pooled connection must not be evicted from the pool + self.assertEqual(1, len(pool)) + self.assertEqual(mock_result3.call_count, i) + class LDAPIdentity(LdapPoolCommonTestMixin, test_backend_ldap.LDAPIdentity, 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. @@ -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 = |