From efab02ae4790121fef919bb9c011d3f3c8eb5342 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 11 May 2015 21:23:18 -0400 Subject: [PATCH] LDAP improvements: - Better logging - Better error messages - Add unit tests - Clean up the setup tool for LDAP --- data/users.py | 41 +++++---- requirements-nover.txt | 1 + requirements.txt | 1 + .../directives/config/config-setup-tool.html | 75 +++++++++++++---- test/test_ldap.py | 83 +++++++++++++++++++ 5 files changed, 173 insertions(+), 28 deletions(-) create mode 100644 test/test_ldap.py diff --git a/data/users.py b/data/users.py index fd3263a0a..0f228786f 100644 --- a/data/users.py +++ b/data/users.py @@ -11,12 +11,23 @@ from util.validation import generate_valid_usernames from data import model logger = logging.getLogger(__name__) +if os.environ.get('LDAP_DEBUG') == '1': + logger.setLevel(logging.DEBUG) + ch = logging.StreamHandler() + ch.setLevel(logging.DEBUG) + + logger.addHandler(ch) class DatabaseUsers(object): def verify_user(self, username_or_email, password): """ Simply delegate to the model implementation. """ - return model.verify_user(username_or_email, password) + result = model.verify_user(username_or_email, password) + if not result: + return (None, 'Invalid Username or Password') + + return (result, None) + def user_exists(self, username): return model.get_user(username) is not None @@ -30,9 +41,10 @@ class LDAPConnection(object): self._conn = None def __enter__(self): - trace_level = 2 if os.environ.get('LDAP_DEBUG') else 0 + trace_level = 2 if os.environ.get('LDAP_DEBUG') == '1' else 0 self._conn = ldap.initialize(self._ldap_uri, trace_level=trace_level) self._conn.simple_bind_s(self._user_dn, self._user_pw) + return self._conn def __exit__(self, exc_type, value, tb): @@ -55,7 +67,7 @@ class LDAPUsers(object): query = u'(|({0}={2})({1}={2}))'.format(self._uid_attr, self._email_attr, username_or_email) - logger.debug('Conducting user search: %s => %s', user_search_dn, query) + logger.debug('Conducting user search: %s under %s', query, user_search_dn) try: user = conn.search_s(user_search_dn, ldap.SCOPE_SUBTREE, query.encode('utf-8')) except ldap.LDAPError: @@ -75,12 +87,12 @@ class LDAPUsers(object): # Make sure that even if the server supports anonymous binds, we don't allow it if not password: - return None + return (None, 'Anonymous binding not allowed') found_user = self._ldap_user_search(username_or_email) if found_user is None: - return None + return (None, 'Username not found') found_dn, found_response = found_user @@ -91,9 +103,15 @@ class LDAPUsers(object): pass except ldap.INVALID_CREDENTIALS: logger.exception('Invalid LDAP credentials') - return None + return (None, 'Invalid password') # Now check if we have a federated login for this user + if not found_response.get(self._uid_attr): + return (None, 'Missing uid field "%s" in user record' % self._uid_attr) + + if not found_response.get(self._email_attr): + return (None, 'Missing mail field "%s" in user record' % self._email_attr) + username = found_response[self._uid_attr][0].decode('utf-8') email = found_response[self._email_attr][0] db_user = model.verify_federated_login('ldap', username) @@ -107,7 +125,7 @@ class LDAPUsers(object): if not valid_username: logger.error('Unable to pick a username for user: %s', username) - return None + return (None, 'Unable to pick a username. Please report this to your administrator.') db_user = model.create_federated_user(valid_username, email, 'ldap', username, set_password_notification=False) @@ -116,7 +134,7 @@ class LDAPUsers(object): db_user.email = email db_user.save() - return db_user + return (db_user, None) def user_exists(self, username): found_user = self._ldap_user_search(username) @@ -225,12 +243,7 @@ class UserAuthentication(object): else: password = decrypted - result = self.state.verify_user(username_or_email, password) - if result: - return (result, '') - else: - return (result, 'Invalid password.') - + return self.state.verify_user(username_or_email, password) def __getattr__(self, name): return getattr(self.state, name, None) diff --git a/requirements-nover.txt b/requirements-nover.txt index f0ac2cdf9..d76e671b5 100644 --- a/requirements-nover.txt +++ b/requirements-nover.txt @@ -51,3 +51,4 @@ cachetools mock psutil stringscore +mockldap diff --git a/requirements.txt b/requirements.txt index e3d00eeb0..08eddb442 100644 --- a/requirements.txt +++ b/requirements.txt @@ -37,6 +37,7 @@ jsonschema==2.4.0 marisa-trie==0.7 mixpanel-py==3.2.1 mock==1.0.1 +mockldap==0.2.4 paramiko==1.15.2 peewee==2.4.7 psutil==2.2.1 diff --git a/static/directives/config/config-setup-tool.html b/static/directives/config/config-setup-tool.html index 35511916f..7c2fa2e19 100644 --- a/static/directives/config/config-setup-tool.html +++ b/static/directives/config/config-setup-tool.html @@ -310,12 +310,12 @@

-
+
It is highly recommended to require encrypted client passwords. LDAP passwords used in the Docker client will be stored in plaintext! Enable this requirement now.
-
+
Note: The "Require Encrypted Client Passwords" feature is currently enabled which will prevent LDAP passwords from being saved as plaintext by the Docker client.
@@ -343,29 +343,76 @@
- - Administrator DN: - - Base DN: - + + +
+ A list of Distinguished Name pieces which forms the base path for + looking up all LDAP records. +
+
+ Example: [dc=my,dc=domain,dc=com] +
+ - Administrator Password: - + User Relative DN: + + +
+ A list of Distinguished Name pieces which forms the base path for + looking up all user LDAP records, relative to the Base DN defined above. +
+
+ Example: [ou=employees] +
+ - E-mail Attribute: - + Administrator DN: + +
+ The Distinguished Name for the Administrator account. This account must be able to login and view the records for all user accounts. +
+
+ Example: uid=admin,ou=employees,dc=my,dc=domain,dc=com +
+ + + + Administrator DN Password: + +
+ Note: This will be stored in + plaintext inside the config.yaml, so setting up a dedicated account or using + a password hash is highly recommended. +
+ +
+ The password for the Administrator DN. +
+ UID Attribute: - + + +
+ The name of the property field in your LDAP user records that stores your + users' username. Typically "uid". +
+ - User RDN: - + Mail Attribute: + + +
+ The name of the property field in your LDAP user records that stores your + users' e-mail address(es). Typically "mail". +
+
diff --git a/test/test_ldap.py b/test/test_ldap.py new file mode 100644 index 000000000..323a87a4e --- /dev/null +++ b/test/test_ldap.py @@ -0,0 +1,83 @@ +import unittest + +from app import app +from initdb import setup_database_for_testing, finished_database_for_testing +from data import model +from data.users import LDAPUsers + +from mockldap import MockLdap + +class TestLDAP(unittest.TestCase): + def setUp(self): + setup_database_for_testing(self) + self.app = app.test_client() + self.ctx = app.test_request_context() + self.ctx.__enter__() + + self.mockldap = MockLdap({ + 'dc=quay,dc=io': {'dc': ['quay', 'io']}, + 'ou=employees,dc=quay,dc=io': { + 'dc': ['quay', 'io'], + 'ou': 'employees' + }, + 'uid=testy,ou=employees,dc=quay,dc=io': { + 'dc': ['quay', 'io'], + 'ou': 'employees', + 'uid': 'testy', + 'userPassword': ['password'] + }, + 'uid=someuser,ou=employees,dc=quay,dc=io': { + 'dc': ['quay', 'io'], + 'ou': 'employees', + 'uid': ['someuser'], + 'userPassword': ['somepass'], + 'mail': ['foo@bar.com'] + }, + 'uid=nomail,ou=employees,dc=quay,dc=io': { + 'dc': ['quay', 'io'], + 'ou': 'employees', + 'uid': ['nomail'], + 'userPassword': ['somepass'] + } + }) + + self.mockldap.start() + + def tearDown(self): + self.mockldap.stop() + finished_database_for_testing(self) + self.ctx.__exit__(True, None, None) + + def test_login(self): + base_dn = ['dc=quay', 'dc=io'] + admin_dn = 'uid=testy,ou=employees,dc=quay,dc=io' + admin_passwd = 'password' + user_rdn = ['ou=employees'] + uid_attr = 'uid' + email_attr = 'mail' + + ldap = LDAPUsers('ldap://localhost', base_dn, admin_dn, admin_passwd, user_rdn, + uid_attr, email_attr) + + (response, _) = ldap.verify_user('someuser', 'somepass') + self.assertEquals(response.username, 'someuser') + + def test_missing_mail(self): + base_dn = ['dc=quay', 'dc=io'] + admin_dn = 'uid=testy,ou=employees,dc=quay,dc=io' + admin_passwd = 'password' + user_rdn = ['ou=employees'] + uid_attr = 'uid' + email_attr = 'mail' + + ldap = LDAPUsers('ldap://localhost', base_dn, admin_dn, admin_passwd, user_rdn, + uid_attr, email_attr) + + (response, err_msg) = ldap.verify_user('nomail', 'somepass') + self.assertIsNone(response) + self.assertEquals('Missing mail field "mail" in user record', err_msg) + + +if __name__ == '__main__': + unittest.main() +