summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--keystone/identity/backends/ldap/common.py21
-rw-r--r--keystone/tests/unit/fakeldap.py9
-rw-r--r--keystone/tests/unit/test_backend_ldap_pool.py118
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,