openstack/keystone
Code Issues Proposed changes

[PooledLDAPHandler] Ensure result3() invokes message.clean()

Browse Source 
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>
This commit is contained in:
Mustafa Kemal Gilor 2022-12-05 17:33:47 +03:00
parent 7d4047cb69
commit ff632a81fb
No known key found for this signature in database
GPG Key ID: FAF50F1C5306E2AC
3 changed files with 138 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
keystone/identity/backends/ldap/common.py
View File

@ -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

9
keystone/tests/unit/fakeldap.py
View File

@ -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

118
keystone/tests/unit/test_backend_ldap_pool.py
View File

@ -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,