[PooledLDAPHandler] Ensure result3() invokes message.clean()stable/zed

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 <mustafa.gilor@canonical.com>
(cherry picked from commit ff632a81fb09e6d9f3298e494d53eb6df50269cf)

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,