From 7c30c9e000b58055b61d8cf58e52493f2b5aba8a Mon Sep 17 00:00:00 2001 From: Mustafa Kemal Gilor Date: Mon, 5 Dec 2022 17:33:47 +0300 Subject: [PooledLDAPHandler] Ensure result3() invokes message.clean() result3 does not invoke message.clean() when an exception is thrown by `message.connection.result3()` call, causing pool connection associated with the message to be marked active forever. This causes a denial-of-service on ldappool. The fix ensures message.clean() is invoked by wrapping the offending call in try-except-finally and putting the message.clean() in finally block. Closes-Bug: #1998789 Change-Id: I59ebf0fa77391d49b2349e918fc55f96318c42a6 Signed-off-by: Mustafa Kemal Gilor (cherry picked from commit ff632a81fb09e6d9f3298e494d53eb6df50269cf) --- keystone/identity/backends/ldap/common.py | 21 +++-- keystone/tests/unit/fakeldap.py | 9 +- keystone/tests/unit/test_backend_ldap_pool.py | 118 +++++++++++++++++++++++++- 3 files changed, 138 insertions(+), 10 deletions(-) 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/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, -- cgit v1.2.1