diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py index a7b3e605ff..6b8750baaf 100644 --- a/keystone/identity/backends/ldap/common.py +++ b/keystone/identity/backends/ldap/common.py @@ -1281,9 +1281,38 @@ class BaseLdap(object): else: return self._id_to_dn_string(object_id) - @staticmethod - def _dn_to_id(dn): - return utf8_decode(ldap.dn.str2dn(utf8_encode(dn))[0][0][1]) + def _dn_to_id(self, dn): + # Check if the naming attribute in the DN is the same as keystone's + # configured 'id' attribute'. If so, extract the ID value from the DN + if self.id_attr == utf8_decode( + ldap.dn.str2dn(utf8_encode(dn))[0][0][0].lower()): + return utf8_decode(ldap.dn.str2dn(utf8_encode(dn))[0][0][1]) + else: + # The 'ID' attribute is NOT in the DN, so we need to perform an + # LDAP search to look it up from the user entry itself. + with self.get_connection() as conn: + search_result = conn.search_s(dn, ldap.SCOPE_BASE) + + if search_result: + try: + id_list = search_result[0][1][self.id_attr] + except KeyError: + message = ('ID attribute %(id_attr)s not found in LDAP ' + 'object %(dn)s.') % ({'id_attr': self.id_attr, + 'dn': search_result}) + LOG.warning(message) + raise exception.NotFound(message=message) + if len(id_list) > 1: + message = ('In order to keep backward compatibility, in ' + 'the case of multivalued ids, we are ' + 'returning the first id %(id_attr) in the ' + 'DN.') % ({'id_attr': id_list[0]}) + LOG.warning(message) + return id_list[0] + else: + message = _('DN attribute %(dn)s not found in LDAP') % ( + {'dn': dn}) + raise exception.NotFound(message=message) def _ldap_res_to_model(self, res): # LDAP attribute names may be returned in a different case than diff --git a/keystone/identity/backends/ldap/core.py b/keystone/identity/backends/ldap/core.py index 32cd97602f..212f89e73f 100644 --- a/keystone/identity/backends/ldap/core.py +++ b/keystone/identity/backends/ldap/core.py @@ -311,8 +311,11 @@ class UserApi(common_ldap.EnabledEmuMixIn, common_ldap.BaseLdap): return obj def get_filtered(self, user_id): - user = self.get(user_id) - return self.filter_attributes(user) + try: + user = self.get(user_id) + return self.filter_attributes(user) + except ldap.NO_SUCH_OBJECT: + raise self.NotFound(user_id=user_id) def get_all(self, ldap_filter=None, hints=None): objs = super(UserApi, self).get_all(ldap_filter=ldap_filter, diff --git a/keystone/tests/unit/test_backend_ldap.py b/keystone/tests/unit/test_backend_ldap.py index c41f80f7cc..50011295ed 100644 --- a/keystone/tests/unit/test_backend_ldap.py +++ b/keystone/tests/unit/test_backend_ldap.py @@ -1813,7 +1813,33 @@ class LDAPIdentity(BaseLDAPIdentity): self.assertEqual(self.user_foo['email'], user_ref['id']) @mock.patch.object(common_ldap.BaseLdap, '_ldap_get') - def test_get_id_from_dn_for_multivalued_attribute_id(self, mock_ldap_get): + def test_get_multivalued_attribute_id_from_dn(self, + mock_ldap_get): + driver = PROVIDERS.identity_api._select_identity_driver( + CONF.identity.default_domain_id) + driver.user.id_attr = 'mail' + + # make 'email' multivalued so we can test the error condition + email1 = uuid.uuid4().hex + email2 = uuid.uuid4().hex + # Mock the ldap search results to return user entries with + # user_name_attribute('sn') value has emptyspaces, emptystring + # and attibute itself is not set. + mock_ldap_get.return_value = ( + 'cn=users,dc=example,dc=com', + { + 'mail': [email1, email2], + } + ) + + # This is not a valid scenario, since we do not support multiple value + # attribute id on DN. + self.assertRaises(exception.NotFound, + PROVIDERS.identity_api.get_user, email1) + + @mock.patch.object(common_ldap.BaseLdap, '_ldap_get') + def test_raise_not_found_dn_for_multivalued_attribute_id(self, + mock_ldap_get): driver = PROVIDERS.identity_api._select_identity_driver( CONF.identity.default_domain_id) driver.user.id_attr = 'mail' @@ -1830,10 +1856,29 @@ class LDAPIdentity(BaseLDAPIdentity): } ) - user_ref = PROVIDERS.identity_api.get_user(email1) - # make sure we get the ID from DN (old behavior) if the ID attribute - # has multiple values - self.assertEqual('nobodycares', user_ref['id']) + # This is not a valid scenario, since we do not support multiple value + # attribute id on DN. + self.assertRaises(exception.NotFound, + PROVIDERS.identity_api.get_user, email1) + + @mock.patch.object(common_ldap.BaseLdap, '_ldap_get') + def test_get_id_not_in_dn(self, + mock_ldap_get): + driver = PROVIDERS.identity_api._select_identity_driver( + CONF.identity.default_domain_id) + driver.user.id_attr = 'sAMAccountName' + + user_id = uuid.uuid4().hex + mock_ldap_get.return_value = ( + 'cn=someuser,dc=example,dc=com', + { + 'cn': 'someuser', + 'sn': [uuid.uuid4().hex], + 'sAMAccountName': [user_id], + } + ) + user_ref = PROVIDERS.identity_api.get_user(user_id) + self.assertEqual(user_id, user_ref['id']) @mock.patch.object(common_ldap.BaseLdap, '_ldap_get') def test_id_attribute_not_found(self, mock_ldap_get): diff --git a/releasenotes/notes/bug-1782922-db822fda486ac773.yaml b/releasenotes/notes/bug-1782922-db822fda486ac773.yaml new file mode 100644 index 0000000000..d8f5e26d78 --- /dev/null +++ b/releasenotes/notes/bug-1782922-db822fda486ac773.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + [`bug 1782922 `_] + Fixed the problem where Keystone indiscriminately return the first RDN + as the user ID, regardless whether it matches the configured + 'user_id_attribute' or not. This will break deployments where + 'group_members_are_ids' are set to False and 'user_id_attribute' is not + in the DN. This patch will perform a lookup by DN if the first RND does + not match the configured 'user_id_attribute'.