From 19d5b3b621dbdfe96b290ac2f7af63008d01aa80 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Fri, 25 Jun 2021 09:14:23 -0400 Subject: [PATCH] Return a copy of cached entries, only with requested attributes Some plugins, notably dns, modifies a returned entry in order to compare it to the user-provided info (e.g. dnsrecord-del). This modification was done on the cached entry directly rather than a copy which caused unexpected results, mostly EmptyResult because the cached entry was changed directly so the next get_entry returned the same modified entry. In addition, on a hit in the LDAP cache the entire cached entry was returned regardless of what attributes were requested. The automember condition add/remove calls only request the inclusive/exclusive rule attributes and loop over the returned values to look for duplicates. This was failing because the queried entry contains attributes that the candidate entry does not contain. The automember code is: old_entry = ldap.get_entry(dn, [attr]) for regex in old_entry.keys(): if not isinstance(entry_attrs[regex], (list, tuple)): old_entry, returned from the cache, contained objectclass, cn, description, etc. which don't exist in the candidate entry so entry_attrs[regex] threw a KeyError. To return a copy of the entry and requested attributes on a search HIT. Also be more careful when storing the attributes in the cache entry. The returned attributes may not match the requested. So store the attributes we actually have. This issue was exposed by Ansible which maintains a larger and longer-lived cache because commands are executed in the server context one after another, giving the cache a chance to build up. Adjust the expected test results as well. In test_get_testuser() the first request asks for all attributes (default) so ensure that is successful since a user_add gets all attributes in the post_callback. Next request a subset of the attributes which is also a hit and confirm that only those requested were returned. https://pagure.io/freeipa/issue/8897 Signed-off-by: Rob Crittenden Reviewed-By: Stanislav Levin Reviewed-By: Alexander Bokovoy --- ipapython/ipaldap.py | 39 ++++++++++++++++++---- ipatests/test_ipapython/test_ldap_cache.py | 16 ++++++--- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py index dc14a1811..c06cdffb5 100644 --- a/ipapython/ipaldap.py +++ b/ipapython/ipaldap.py @@ -1802,6 +1802,22 @@ class LDAPCache(LDAPClient): if self._enable_cache: logger.debug(msg, *args, **kwargs) + def copy_entry(self, dn, entry, attrs=[]): + new_entry = LDAPEntry(self, DN(dn)) + + # Return either the whole entry or only those attrs requested + if not attrs: + new_entry.raw.update(deepcopy(dict(entry.raw))) + else: + for attr, original_values in entry.raw.items(): + if attr.lower() not in attrs: + continue + new_entry.raw[attr.lower()] = deepcopy(original_values) + new_entry._orig_raw = deepcopy(dict(entry.raw)) + new_entry.reset_modlist() + + return new_entry + def add_cache_entry(self, dn, attrs_list=None, get_all=False, entry=None, exception=None): # idnsname - caching prevents delete when mod value to None @@ -1828,12 +1844,14 @@ class LDAPCache(LDAPClient): if not BANNED_ATTRS.intersection(attrs_list): self.cache[dn] = CacheEntry( entry=entry.copy(), - attrs_list=deepcopy(attrs_list), + attrs_list=attrs_list.copy(), all=get_all, ) else: return + self.emit("ADD: %s: %s all=%s", dn, attrs_list, get_all) + self.cache.move_to_end(dn) if len(self.cache) > self.max_entries: @@ -1944,7 +1962,7 @@ class LDAPCache(LDAPClient): if ( entry and (attrs_list in (['*'], [''])) - and (entry.attrs_list in (['*'], [''])) + and entry.all ): get_all = True @@ -1953,18 +1971,19 @@ class LDAPCache(LDAPClient): hits = self._cache_hits + 1 # pylint: disable=no-member object.__setattr__(self, '_cache_hits', hits) self.cache_status('HIT') - return entry.entry + return self.copy_entry(dn, entry.entry) # Be sure we have all the requested attributes before returning # a cached entry. if entry and attrs_list: req_attrs = set(attr.lower() for attr in set(attrs_list)) cache_attrs = set(attr.lower() for attr in entry.attrs_list) - if (req_attrs.issubset(cache_attrs)): + if req_attrs.issubset(cache_attrs): hits = self._cache_hits + 1 # pylint: disable=no-member object.__setattr__(self, '_cache_hits', hits) self.cache_status('HIT') - return entry.entry + + return self.copy_entry(dn, entry.entry, req_attrs) try: entry = super(LDAPCache, self).get_entry( @@ -1982,8 +2001,14 @@ class LDAPCache(LDAPClient): # re-raise anything we aren't caching raise else: - self.add_cache_entry(dn, attrs_list=attrs_list, get_all=get_all, - entry=entry) + if attrs_list in (['*'], ['']): + get_all = True + self.add_cache_entry( + dn, + attrs_list=set(k.lower() for k in entry._names.keys()), + get_all=get_all, + entry=self.copy_entry(dn, entry), + ) misses = self._cache_misses + 1 # pylint: disable=no-member object.__setattr__(self, '_cache_misses', misses) self.cache_status('MISS') diff --git a/ipatests/test_ipapython/test_ldap_cache.py b/ipatests/test_ipapython/test_ldap_cache.py index fbe21fc3c..c960db027 100644 --- a/ipatests/test_ipapython/test_ldap_cache.py +++ b/ipatests/test_ipapython/test_ldap_cache.py @@ -90,9 +90,17 @@ class TestLDAPCache: def test_get_testuser_again(self): assert self.userdn in self.cache.cache + + # get the user again with with no attributes requested (so all) self.cache.get_entry(self.userdn) hits_and_misses(self.cache, 2, 2) + # Now get the user with a subset of cached attributes + entry = self.cache.get_entry(self.userdn, ('givenname', 'sn', 'cn')) + # Make sure we only got three attributes, as requested + assert len(entry.items()) == 3 + hits_and_misses(self.cache, 3, 2) + def test_update_testuser(self): entry = self.cache.cache[self.userdn].entry try: @@ -100,7 +108,7 @@ class TestLDAPCache: except errors.EmptyModlist: pass assert self.userdn not in self.cache.cache - hits_and_misses(self.cache, 2, 2) + hits_and_misses(self.cache, 3, 2) def test_modify_testuser(self): self.cache.get_entry(self.userdn) @@ -110,7 +118,7 @@ class TestLDAPCache: except errors.EmptyModlist: pass assert self.userdn not in self.cache.cache - hits_and_misses(self.cache, 2, 3) + hits_and_misses(self.cache, 3, 3) def test_delete_entry(self): # We don't care if this is successful or not, just that the @@ -120,7 +128,7 @@ class TestLDAPCache: except Exception: pass assert self.userdn not in self.cache.cache - hits_and_misses(self.cache, 2, 3) + hits_and_misses(self.cache, 3, 3) def test_add_entry(self): # We don't care if this is successful or not, just that the @@ -130,7 +138,7 @@ class TestLDAPCache: except Exception: pass assert self.userdn not in self.cache.cache - hits_and_misses(self.cache, 2, 3) + hits_and_misses(self.cache, 3, 3) def test_clear_cache(self): self.cache.clear_cache()