From 9d9451e13c8e7a1835d721be7b8a4a5c6dff2b95 Mon Sep 17 00:00:00 2001 From: Raildo Mascena Date: Mon, 1 Apr 2019 16:48:07 -0300 Subject: [PATCH] Fixing dn_to_id function for cases were id is not in the DN The more common scenario to return the uid as part of the RDN in a DN, However, it's a valid case to not have the uid in the RDN, so we need to search in the LDAP based on the DN and return the uid in the entire object. Also, we do not support multivalued attribute id on DN, so the test case covering this case, it was adjusted for raise NotFound. Closes-Bug: 1782922 Change-Id: I87a3bfa94b5907ce4c6b4eb8e124ec948b390bf2 (cherry picked from commit a1dc21f3d34ae34bc6a5c9acebc0eb752495ae7a) --- keystone/identity/backends/ldap/common.py | 35 +++++++++++- keystone/identity/backends/ldap/core.py | 7 ++- keystone/tests/unit/test_backend_ldap.py | 55 +++++++++++++++++-- .../notes/bug-1782922-db822fda486ac773.yaml | 10 ++++ 4 files changed, 97 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/bug-1782922-db822fda486ac773.yaml 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'.