From 909cc9fa8380a03dfdb808db7fb863400fa36054 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 e2f85a7ee7..d2447f47b4 100644 --- a/keystone/identity/backends/ldap/common.py +++ b/keystone/identity/backends/ldap/common.py @@ -1293,9 +1293,38 @@ class BaseLdap(object): else: return self._id_to_dn_string(object_id) - @staticmethod - def _dn_to_id(dn): - return ldap.dn.str2dn(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 b6b87185f6..2afa730e98 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 a13c718bc2..aa7a50747f 100644 --- a/keystone/tests/unit/test_backend_ldap.py +++ b/keystone/tests/unit/test_backend_ldap.py @@ -1819,7 +1819,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' @@ -1836,10 +1862,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'.