From 0a0607d2f7065c76bae3ea3f5e10480f7633097f Mon Sep 17 00:00:00 2001 From: Michael Ossareh Date: Wed, 25 Jul 2012 18:55:48 -0700 Subject: [PATCH 1/2] Support searching for users to find their LDAP entry Summary: - the current LDAP auth flow expects a DN to look like cn=ossareh,ou=Users,dc=example,dc=com - however many LDAP setups have their dn look something like cn=Mike Ossareh,ou=Users,dc=example,dc=com Test Plan: Test if logins work with a LDAP setup which has cn=Full Name instead of cn=username. To test you should ensure you set the properties needed to trigger the search before login as detailed in conf/default.conf.php Reviewers: epriestley CC: mbeck, aran, Korvin Differential Revision: https://secure.phabricator.com/D3072 --- conf/default.conf.php | 8 +++++++ .../auth/ldap/PhabricatorLDAPProvider.php | 21 +++++++++++++------ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index 45b6eaa42e..3f6c4078d7 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -642,6 +642,14 @@ return array( // The attribute to be regarded as 'username'. Has to be unique 'ldap.search_attribute' => '', + // Perform a search to find a user + // Many LDAP installations do not have the username in the dn, if this is + // true for you set this to true and configure the username_attribute below + 'ldap.search-first' => false, + + // The attribute to search for if you have to search for a user + 'ldap.username_attribute' => '', + // The attribute(s) to be regarded as 'real name'. // If more then one attribute is supplied the values of the attributes in // the array will be joined diff --git a/src/applications/auth/ldap/PhabricatorLDAPProvider.php b/src/applications/auth/ldap/PhabricatorLDAPProvider.php index b1e703016e..fa1fcda724 100644 --- a/src/applications/auth/ldap/PhabricatorLDAPProvider.php +++ b/src/applications/auth/ldap/PhabricatorLDAPProvider.php @@ -46,6 +46,10 @@ final class PhabricatorLDAPProvider { return PhabricatorEnv::getEnvConfig('ldap.search_attribute'); } + public function getUsernameAttribute() { + return PhabricatorEnv::getEnvConfig('ldap.username_attribute'); + } + public function getLDAPVersion() { return PhabricatorEnv::getEnvConfig('ldap.version'); } @@ -117,6 +121,13 @@ final class PhabricatorLDAPProvider { throw new Exception('Username can not be empty'); } + if (PhabricatorEnv::getEnvConfig('ldap.search-first')) { + $user = $this->getUser($this->getUsernameAttribute(), $username); + $username = $user[($this->getSearchAttribute())][0]; + } + + $conn = $this->getConnection(); + $activeDirectoryDomain = PhabricatorEnv::getEnvConfig('ldap.activedirectory_domain'); @@ -130,8 +141,6 @@ final class PhabricatorLDAPProvider { $this->getBaseDN()); } - $conn = $this->getConnection(); - // NOTE: It is very important we suppress any messages that occur here, // because it logs passwords if it reaches an error log of any sort. DarkConsoleErrorLogPluginAPI::enableDiscardMode(); @@ -143,16 +152,16 @@ final class PhabricatorLDAPProvider { "LDAP Error #".ldap_errno($conn).": ".ldap_error($conn)); } - $this->userData = $this->getUser($username); + $this->userData = $this->getUser($this->getSearchAttribute(), $username); return $this->userData; } - private function getUser($username) { + private function getUser($attribute, $username) { $conn = $this->getConnection(); $query = ldap_sprintf( '%Q=%S', - $this->getSearchAttribute(), + $attribute, $username); $result = ldap_search($conn, $this->getBaseDN(), $query); @@ -170,7 +179,7 @@ final class PhabricatorLDAPProvider { if ($entries['count'] > 1) { throw new Exception('Found more then one user with this ' . - $this->getSearchAttribute()); + $attribute); } if ($entries['count'] == 0) { From a9af5d611d14c826d0cfc33ba1516b5d057096de Mon Sep 17 00:00:00 2001 From: Michael Ossareh Date: Thu, 26 Jul 2012 14:32:51 -0700 Subject: [PATCH 2/2] Prevent the ability to scrape for valid usernames - return the same error message when either bind or the username search fails to find a user - config variables should use hypen and not underscore --- conf/default.conf.php | 2 +- src/__phutil_library_map__.php | 2 ++ .../auth/ldap/PhabricatorLDAPProvider.php | 30 +++++++++++++++---- .../PhabricatorLDAPUnknownUserException.php | 20 +++++++++++++ 4 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 src/applications/auth/ldap/PhabricatorLDAPUnknownUserException.php diff --git a/conf/default.conf.php b/conf/default.conf.php index 3f6c4078d7..093301a991 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -648,7 +648,7 @@ return array( 'ldap.search-first' => false, // The attribute to search for if you have to search for a user - 'ldap.username_attribute' => '', + 'ldap.username-attribute' => '', // The attribute(s) to be regarded as 'real name'. // If more then one attribute is supplied the values of the attributes in diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c4bc3e10e4..424781ef56 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -710,6 +710,7 @@ phutil_register_library_map(array( 'PhabricatorLDAPLoginController' => 'applications/auth/controller/PhabricatorLDAPLoginController.php', 'PhabricatorLDAPProvider' => 'applications/auth/ldap/PhabricatorLDAPProvider.php', 'PhabricatorLDAPRegistrationController' => 'applications/auth/controller/PhabricatorLDAPRegistrationController.php', + 'PhabricatorLDAPUnknownUserException' => 'applications/auth/ldap/PhabricatorLDAPUnknownUserException.php', 'PhabricatorLDAPUnlinkController' => 'applications/auth/controller/PhabricatorLDAPUnlinkController.php', 'PhabricatorLintEngine' => 'infrastructure/lint/PhabricatorLintEngine.php', 'PhabricatorLiskDAO' => 'infrastructure/storage/lisk/PhabricatorLiskDAO.php', @@ -1727,6 +1728,7 @@ phutil_register_library_map(array( 'PhabricatorJavelinLinter' => 'ArcanistLinter', 'PhabricatorLDAPLoginController' => 'PhabricatorAuthController', 'PhabricatorLDAPRegistrationController' => 'PhabricatorAuthController', + 'PhabricatorLDAPUnknownUserException' => 'Exception', 'PhabricatorLDAPUnlinkController' => 'PhabricatorAuthController', 'PhabricatorLintEngine' => 'PhutilLintEngine', 'PhabricatorLiskDAO' => 'LiskDAO', diff --git a/src/applications/auth/ldap/PhabricatorLDAPProvider.php b/src/applications/auth/ldap/PhabricatorLDAPProvider.php index fa1fcda724..f203af44a1 100644 --- a/src/applications/auth/ldap/PhabricatorLDAPProvider.php +++ b/src/applications/auth/ldap/PhabricatorLDAPProvider.php @@ -17,6 +17,10 @@ */ final class PhabricatorLDAPProvider { + // http://www.php.net/manual/en/function.ldap-errno.php#20665 states + // that the number could be 31 or 49, in testing it has always been 49 + const LDAP_INVALID_CREDENTIALS = 49; + private $userData; private $connection; @@ -47,7 +51,7 @@ final class PhabricatorLDAPProvider { } public function getUsernameAttribute() { - return PhabricatorEnv::getEnvConfig('ldap.username_attribute'); + return PhabricatorEnv::getEnvConfig('ldap.username-attribute'); } public function getLDAPVersion() { @@ -116,14 +120,28 @@ final class PhabricatorLDAPProvider { return $this->userData; } + private function invalidLDAPUserErrorMessage($errno, $errmsg) { + return "LDAP Error #".$errno.": ".$errmsg; + } + public function auth($username, PhutilOpaqueEnvelope $password) { if (strlen(trim($username)) == 0) { throw new Exception('Username can not be empty'); } if (PhabricatorEnv::getEnvConfig('ldap.search-first')) { - $user = $this->getUser($this->getUsernameAttribute(), $username); - $username = $user[($this->getSearchAttribute())][0]; + // To protect against people phishing for accounts we catch the + // exception and present the default exception that would be presented + // in the case of a failed bind. + try { + $user = $this->getUser($this->getUsernameAttribute(), $username); + $username = $user[$this->getSearchAttribute()][0]; + } catch (PhabricatorLDAPUnknownUserException $e) { + throw new Exception( + $this->invalidLDAPUserErrorMessage( + self::LDAP_INVALID_CREDENTIALS, + ldap_err2str(self::LDAP_INVALID_CREDENTIALS))); + } } $conn = $this->getConnection(); @@ -149,7 +167,9 @@ final class PhabricatorLDAPProvider { if (!$result) { throw new Exception( - "LDAP Error #".ldap_errno($conn).": ".ldap_error($conn)); + $this->invalidLDAPUserErrorMessage( + ldap_errno($conn), + ldap_error($conn))); } $this->userData = $this->getUser($this->getSearchAttribute(), $username); @@ -183,7 +203,7 @@ final class PhabricatorLDAPProvider { } if ($entries['count'] == 0) { - throw new Exception('Could not find user'); + throw new PhabricatorLDAPUnknownUserException('Could not find user'); } return $entries[0]; diff --git a/src/applications/auth/ldap/PhabricatorLDAPUnknownUserException.php b/src/applications/auth/ldap/PhabricatorLDAPUnknownUserException.php new file mode 100644 index 0000000000..6994025938 --- /dev/null +++ b/src/applications/auth/ldap/PhabricatorLDAPUnknownUserException.php @@ -0,0 +1,20 @@ +