diff -Nru keystone-20.0.1/debian/changelog keystone-20.0.1/debian/changelog --- keystone-20.0.1/debian/changelog 2023-01-27 20:52:41.000000000 +0300 +++ keystone-20.0.1/debian/changelog 2023-10-04 16:51:10.000000000 +0300 @@ -1,3 +1,11 @@ +keystone (2:20.0.1-0ubuntu1~cloud1) focal-xena; urgency=medium + + * d/p/lp1998789-pooledldap-conn-release-on-exception.patch: release + pooled ldap connection back to pool when an exception is thrown + by ldap methods. (LP: #1998789) + + -- Mustafa Kemal GILOR Wed, 04 Oct 2023 16:51:10 +0300 + keystone (2:20.0.1-0ubuntu1~cloud0) focal-xena; urgency=medium * d/gbp.conf: Create stable/xena branch. diff -Nru keystone-20.0.1/debian/patches/lp1998789-pooledldap-conn-release-on-exception.patch keystone-20.0.1/debian/patches/lp1998789-pooledldap-conn-release-on-exception.patch --- keystone-20.0.1/debian/patches/lp1998789-pooledldap-conn-release-on-exception.patch 1970-01-01 02:00:00.000000000 +0200 +++ keystone-20.0.1/debian/patches/lp1998789-pooledldap-conn-release-on-exception.patch 2023-10-04 16:50:38.000000000 +0300 @@ -0,0 +1,220 @@ +Description: [PATCH] [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. + +Author: Mustafa Kemal Gilor +Origin: backport, https://opendev.org/openstack/keystone/commit/ff632a81fb09e6d9f3298e494d53eb6df50269cf +Bug: https://bugs.launchpad.net/keystone/+bug/1998789 +Last-Update: 2023-09-12 +--- +This patch header follows DEP-3: http://dep.debian.net/deps/dep3/ +--- a/keystone/identity/backends/ldap/common.py ++++ b/keystone/identity/backends/ldap/common.py +@@ -860,11 +860,22 @@ + 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 + +--- a/keystone/tests/unit/fakeldap.py ++++ b/keystone/tests/unit/fakeldap.py +@@ -296,6 +296,9 @@ + 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 @@ + + 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 @@ + 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 @@ + 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 +--- a/keystone/tests/unit/test_backend_ldap_pool.py ++++ b/keystone/tests/unit/test_backend_ldap_pool.py +@@ -163,12 +163,23 @@ + + # 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 @@ + 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, diff -Nru keystone-20.0.1/debian/patches/series keystone-20.0.1/debian/patches/series --- keystone-20.0.1/debian/patches/series 2023-01-27 20:52:41.000000000 +0300 +++ keystone-20.0.1/debian/patches/series 2023-10-04 16:50:30.000000000 +0300 @@ -1,3 +1,4 @@ skip-py38-failure.patch add-version-info.patch add-missing-manifest.patch +lp1998789-pooledldap-conn-release-on-exception.patch