From f8ee249bf08cefd8468aa15c589dab48bd5c4cd8 Mon Sep 17 00:00:00 2001 From: Colleen Murphy Date: Tue, 6 Dec 2016 15:40:02 +0100 Subject: [PATCH] Add anonymous bind to get_connection method If no username and password is specified in the keystone ldap configuration, it may still be possible to bind to an LDAP server anonymously if the LDAP server is configured to allow it. Currently, upon creating a connection object, keystone only attempts to bind to the LDAP server if a username and password has been provided to it. This would rarely be an issue because pyldap attempts a reconnect upon executing any ldap command, if necessary, and hence the anonymous bind just happens later. It is a problem now because logic was added[1] to check if the server errored during that initial connection, and for it to work correctly the initial connection needs to happen in a predictable place. This patch adds an anonymous bind to the get_connection method so that no matter the credential configuration the initial connection is consistent. This required adding mocks to many of the LDAP backend tests since every LDAP interaction now attempts a simple_bind_s() regardless of whether credentials are configured in keystone. [1] https://review.openstack.org/#/c/390948 Closes-bug: #1649138 Change-Id: I193c9537c107092e48f7ea1d25ff9c17f872c15b --- keystone/identity/backends/ldap/common.py | 2 ++ keystone/tests/unit/fakeldap.py | 24 +++++++++---------- .../identity/backends/test_ldap_common.py | 12 ++++++---- keystone/tests/unit/test_backend_ldap.py | 5 ++-- .../notes/bug-1649138-c53974f6bb0eab14.yaml | 9 +++++++ 5 files changed, 33 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/bug-1649138-c53974f6bb0eab14.yaml diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py index 559bf149d7..b176e3f8ab 100644 --- a/keystone/identity/backends/ldap/common.py +++ b/keystone/identity/backends/ldap/common.py @@ -1298,6 +1298,8 @@ class BaseLdap(object): # if we don't have any user/pass if user and password: conn.simple_bind_s(user, password) + else: + conn.simple_bind_s() return conn except ldap.SERVER_DOWN: diff --git a/keystone/tests/unit/fakeldap.py b/keystone/tests/unit/fakeldap.py index 1c95242923..1e67a32264 100644 --- a/keystone/tests/unit/fakeldap.py +++ b/keystone/tests/unit/fakeldap.py @@ -305,19 +305,19 @@ class FakeLdap(common.LDAPHandler): common.utf8_decode(cred) in ['password', CONF.ldap.password]): return - try: - attrs = self.db[self.key(who)] - except KeyError: - LOG.debug('bind fail: who=%s not found', common.utf8_decode(who)) - raise ldap.NO_SUCH_OBJECT - - db_password = None - try: - db_password = attrs['userPassword'][0] - except (KeyError, IndexError): - LOG.debug('bind fail: password for who=%s not found', + attrs = self.db.get(self.key(who)) + if not attrs: + LOG.debug('who=%s not found, binding anonymously', common.utf8_decode(who)) - raise ldap.INAPPROPRIATE_AUTH + + db_password = '' + if attrs: + try: + db_password = attrs['userPassword'][0] + except (KeyError, IndexError): + LOG.debug('bind fail: password for who=%s not found', + common.utf8_decode(who)) + raise ldap.INAPPROPRIATE_AUTH if cred != common.utf8_encode(db_password): LOG.debug('bind fail: password for who=%s does not match', diff --git a/keystone/tests/unit/identity/backends/test_ldap_common.py b/keystone/tests/unit/identity/backends/test_ldap_common.py index d1586bc392..7751fdc220 100644 --- a/keystone/tests/unit/identity/backends/test_ldap_common.py +++ b/keystone/tests/unit/identity/backends/test_ldap_common.py @@ -283,14 +283,16 @@ class LDAPDeleteTreeTest(unit.TestCase): class MultiURLTests(unit.TestCase): """Test for setting multiple LDAP URLs.""" - def test_multiple_urls_with_comma_no_conn_pool(self): + @mock.patch.object(common_ldap.KeystoneLDAPHandler, 'simple_bind_s') + def test_multiple_urls_with_comma_no_conn_pool(self, mock_ldap_bind): urls = 'ldap://localhost,ldap://backup.localhost' self.config_fixture.config(group='ldap', url=urls, use_pool=False) base_ldap = common_ldap.BaseLdap(CONF) ldap_connection = base_ldap.get_connection() self.assertEqual(urls, ldap_connection.conn.conn._uri) - def test_multiple_urls_with_comma_with_conn_pool(self): + @mock.patch.object(common_ldap.KeystoneLDAPHandler, 'simple_bind_s') + def test_multiple_urls_with_comma_with_conn_pool(self, mock_ldap_bind): urls = 'ldap://localhost,ldap://backup.localhost' self.config_fixture.config(group='ldap', url=urls, use_pool=True) base_ldap = common_ldap.BaseLdap(CONF) @@ -301,7 +303,8 @@ class MultiURLTests(unit.TestCase): class LDAPConnectionTimeoutTest(unit.TestCase): """Test for Network Connection timeout on LDAP URL connection.""" - def test_connectivity_timeout_no_conn_pool(self): + @mock.patch.object(common_ldap.KeystoneLDAPHandler, 'simple_bind_s') + def test_connectivity_timeout_no_conn_pool(self, mock_ldap_bind): url = 'ldap://localhost' conn_timeout = 1 # 1 second self.config_fixture.config(group='ldap', @@ -325,7 +328,8 @@ class LDAPConnectionTimeoutTest(unit.TestCase): ldap.get_option(ldap.OPT_NETWORK_TIMEOUT)) self.assertEqual(url, ldap_connection.conn.conn._uri) - def test_connectivity_timeout_with_conn_pool(self): + @mock.patch.object(common_ldap.KeystoneLDAPHandler, 'simple_bind_s') + def test_connectivity_timeout_with_conn_pool(self, mock_ldap_bind): url = 'ldap://localhost' conn_timeout = 1 # 1 second self.config_fixture.config(group='ldap', diff --git a/keystone/tests/unit/test_backend_ldap.py b/keystone/tests/unit/test_backend_ldap.py index 53a0604ddf..97d3518704 100644 --- a/keystone/tests/unit/test_backend_ldap.py +++ b/keystone/tests/unit/test_backend_ldap.py @@ -1409,13 +1409,12 @@ class LDAPIdentity(BaseLDAPIdentity, unit.TestCase): @mock.patch.object(common_ldap.KeystoneLDAPHandler, 'simple_bind_s') def test_user_api_get_connection_no_user_password(self, mocked_method): - """Don't bind in case the user and password are blank.""" + """Bind anonymously when the user and password are blank.""" # Ensure the username/password are in-fact blank self.config_fixture.config(group='ldap', user=None, password=None) user_api = identity.backends.ldap.UserApi(CONF) user_api.get_connection(user=None, password=None) - self.assertFalse(mocked_method.called, - msg='`simple_bind_s` method was unexpectedly called') + self.assertTrue(mocked_method.called) @mock.patch.object(common_ldap.KeystoneLDAPHandler, 'connect') def test_chase_referrals_off(self, mocked_fakeldap): diff --git a/releasenotes/notes/bug-1649138-c53974f6bb0eab14.yaml b/releasenotes/notes/bug-1649138-c53974f6bb0eab14.yaml new file mode 100644 index 0000000000..4a89461d30 --- /dev/null +++ b/releasenotes/notes/bug-1649138-c53974f6bb0eab14.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - > + [`bug 1649138 `_] + When using LDAP as an identity backend, the initial bind will now + occur upon creation of a connection object, i.e. early on when + performing LDAP queries, no matter whether the bind is + authenticated or anonymous, so that any connection errors can be + handled correctly and early.