Skip to content

Commit 485d343

Browse files
snipesnipe
authored
Merge pull request #18058 from uberbrady/ldap_fetch_dn_before_login_FIXED
FIXED - perform Ldap fetch for dn (Distinguished Name) before logging-in (fixed)
2 parents 07f0e8a + 598612d commit 485d343

File tree

2 files changed

+119
-17
lines changed

2 files changed

+119
-17
lines changed

‎app/Models/Ldap.php‎

Lines changed: 85 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,78 @@ public static function connectToLdap()
104104
return $connection;
105105
}
106106

107+
/**
108+
* Finds user via Admin search *first*, and _then_ try to bind as that user, returning the user attributes on success,
109+
* or false on failure. This enables login when the DN is harder to programmatically 'guess' due to having users in
110+
* various different OU's or other LDAP entities.
111+
*/
112+
public static function findAndBindMultiOU(string $baseDn, string $filterQuery, string $password, int $slow_failure = 3): array|false
113+
{
114+
/**
115+
* If you *don't* set the slow_failure variable, do note that we might permit timing attacks in here - if
116+
* your find results come back 'slow' when a user *does* exist, but fast if they *don't* exist, then you
117+
* can use this to enumerate users.
118+
*
119+
* Even if that's *not* true, we still might have an issue: if we don't find the user, then we don't even _try_
120+
* to bind as them. Again, that could permit a timing attack.
121+
*
122+
* Instead of checking every little thing, we just wrap everything in a try/catch in order to unify the
123+
* 'slow_failure' treatment. All failures are re-raised as exceptions so that all failures exit from the
124+
* same place.
125+
*/
126+
$connection = null;
127+
$admin_conn = null;
128+
try {
129+
/**
130+
* First we get an 'admin' connection, which will need search permissions. That was already a requirement
131+
* here, so that's not a big lift. But it _is_ possible to configure LDAP to only login, and *not* to be
132+
* able to import lists of users. In that case, this function *will not work* - and you should use the
133+
* legacy 'findAndBindUserLdap' method, below. Otherwise, it looks like this would attempt an anonymous
134+
* bind - which you might want, but you probably don't.
135+
*
136+
**/
137+
$admin_conn = self::connectToLdap();
138+
self::bindAdminToLdap($admin_conn);
139+
$results = ldap_search($admin_conn, $baseDn, $filterQuery);
140+
$entry_count = ldap_count_entries($admin_conn, $results);
141+
if ($entry_count != 1) {
142+
throw new \Exception('Wrong number of entries found: ' . $entry_count);
143+
}
144+
$entry = ldap_first_entry($admin_conn, $results);
145+
$user = ldap_get_attributes($admin_conn, $entry);
146+
$userDn = ldap_get_dn($admin_conn, $entry);
147+
if (!$userDn) {
148+
throw new \Exception("No user DN found");
149+
}
150+
\Log::debug("FOUND DN IS: $userDn");
151+
// The temptation now is to do ldap_unbind on the $admin_conn, but that gets handled in the 'finally' below.
152+
// I don't know if that means a separate 'connection' is maintained to the LDAP server or not, and would
153+
// definitely prefer to not do that if we can avoid it. But I don't know enough about the LDAP protocol to
154+
// be certain that that happens.
155+
156+
//now we try to log in (bind) as that found user
157+
$connection = self::connectToLdap();
158+
$bind_results = ldap_bind($connection, $userDn, $password);
159+
if (!$bind_results) {
160+
throw new \Exception("Unable to bind as user");
161+
}
162+
return array_change_key_case($user);
163+
} catch (\Exception $e) {
164+
\Log::debug("Exception on fast find-and-bind: " . $e->getMessage());
165+
if ($slow_failure) {
166+
sleep($slow_failure);
167+
}
168+
return false; //TODO - make this null instead for a slightly nicer type signature
169+
} finally {
170+
if ($admin_conn) {
171+
ldap_unbind($admin_conn);
172+
}
173+
if ($connection) {
174+
ldap_unbind($connection);
175+
}
176+
}
177+
}
178+
107179

108180
/**
109181
* Binds/authenticates the user to LDAP, and returns their attributes.
@@ -147,18 +219,29 @@ public static function findAndBindUserLdap($username, $password)
147219

148220
Log::debug('Filter query: '.$filterQuery);
149221

222+
// only try this if we have an Admin username set; otherwise use the 'legacy' method
223+
if ($settings->ldap_uname) {
224+
// in the fallowing call, we pick a slow-failure of 0 because we might need to fall through to 'legacy'
225+
$fast_bind = self::findAndBindMultiOU($baseDn, $filterQuery, $password, 0);
226+
if ($fast_bind) {
227+
\Log::debug("Fast bind worked");
228+
return $fast_bind;
229+
}
230+
\Log::debug("Fast bind failed; falling through to legacy bind");
231+
}
232+
150233
if (! $ldapbind = @ldap_bind($connection, $userDn, $password)) {
151234
Log::debug("Status of binding user: $userDn to directory: (directly!) ".($ldapbind ? "success" : "FAILURE"));
152235
if (! $ldapbind = self::bindAdminToLdap($connection)) {
153236
/*
154237
* TODO PLEASE:
155238
*
156239
* this isn't very clear, so it's important to note: the $ldapbind value is never correctly returned - we never 'return true' from self::bindAdminToLdap() (the function
157-
* just "falls off the end" without ever explictly returning 'true')
240+
* just "falls off the end" without ever explicitly returning 'true')
158241
*
159242
* but it *does* have an interesting side-effect of checking for the LDAP password being incorrectly encrypted with the wrong APP_KEY, so I'm leaving it in for now.
160243
*
161-
* If it *did* correctly return 'true' on a succesful bind, it would _probably_ allow users to log in with an incorrect password. Which would be horrible!
244+
* If it *did* correctly return 'true' on a successful bind, it would _probably_ allow users to log in with an incorrect password. Which would be horrible!
162245
*
163246
* Let's definitely fix this at the next refactor!!!!
164247
*

‎tests/Unit/LdapTest.php‎

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -81,18 +81,22 @@ public function testFindAndBind()
8181
$this->settings->enableLdap();
8282

8383
$ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
84-
$ldap_connect->expects($this->once())->willReturn('hello');
84+
$ldap_connect->expects($this->exactly(3))->willReturn('hello');
8585

8686
$ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
87-
$ldap_set_option->expects($this->exactly(4));
87+
$ldap_set_option->expects($this->exactly(12));
8888

89-
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->once())->willReturn(true);
89+
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(true);
9090

91-
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->once())->willReturn(true);
91+
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->exactly(1))->willReturn(true);
9292

93-
$this->getFunctionMock("App\\Models", "ldap_first_entry")->expects($this->once())->willReturn(true);
93+
$this->getFunctionMock("App\\Models", "ldap_first_entry")->expects($this->exactly(1))->willReturn(true);
94+
$this->getFunctionMock("App\\Models", "ldap_unbind")->expects($this->exactly(2));
95+
$this->getFunctionMock("App\\Models", "ldap_count_entries")->expects($this->once())->willReturn(1);
96+
$this->getFunctionMock("App\\Models", "ldap_get_dn")->expects($this->once())->willReturn('dn=FirstName Surname,ou=Org,dc=example,dc=com');
9497

95-
$this->getFunctionMock("App\\Models", "ldap_get_attributes")->expects($this->once())->willReturn(
98+
99+
$this->getFunctionMock("App\\Models", "ldap_get_attributes")->expects($this->exactly(1))->willReturn(
96100
[
97101
"count" => 1,
98102
0 => [
@@ -111,13 +115,25 @@ public function testFindAndBindBadPassword()
111115
$this->settings->enableLdap();
112116

113117
$ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
114-
$ldap_connect->expects($this->once())->willReturn('hello');
118+
$ldap_connect->expects($this->exactly(3))->willReturn('hello');
115119

116120
$ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
117-
$ldap_set_option->expects($this->exactly(4));
118-
119-
// note - we return FALSE first, to simulate a bad-bind, then TRUE the second time to simulate a successful admin bind
120-
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(false, true);
121+
$ldap_set_option->expects($this->exactly(12));
122+
123+
//
124+
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(4))->willReturn(
125+
true, /* initial admin connection for 'fast path' */
126+
false, /* the actual login for the user */
127+
false, /* the direct login for the user binding-as-themselves in the legacy path */
128+
true /* the admin login afterwards (which is weird and doesn't make sense) */
129+
);
130+
$this->getFunctionMock("App\\Models", "ldap_unbind")->expects($this->exactly(2));
131+
$this->getFunctionMock("App\\Models", "ldap_error")->expects($this->never())->willReturn("exception");
132+
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->exactly(1))->willReturn(false); //uhm?
133+
$this->getFunctionMock("App\\Models", "ldap_count_entries")->expects($this->exactly(1))->willReturn(1);
134+
$this->getFunctionMock("App\\Models", "ldap_first_entry")->expects($this->exactly(1))->willReturn(true);
135+
$this->getFunctionMock("App\\Models", "ldap_get_attributes")->expects($this->exactly(1))->willReturn(1);
136+
$this->getFunctionMock("App\\Models", "ldap_get_dn")->expects($this->exactly(1))->willReturn('dn=FirstName Surname,ou=Org,dc=example,dc=com');
121137

122138
// $this->getFunctionMock("App\\Models","ldap_error")->expects($this->once())->willReturn("exception");
123139

@@ -132,14 +148,17 @@ public function testFindAndBindCannotFindSelf()
132148
$this->settings->enableLdap();
133149

134150
$ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
135-
$ldap_connect->expects($this->once())->willReturn('hello');
151+
$ldap_connect->expects($this->exactly(2))->willReturn('hello');
136152

137153
$ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
138-
$ldap_set_option->expects($this->exactly(4));
154+
$ldap_set_option->expects($this->exactly(8));
139155

140-
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->once())->willReturn(true);
156+
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(true); //I think this is OK
157+
158+
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->exactly(2))->willReturn(false); //uhm?
159+
$this->getFunctionMock("App\\Models", "ldap_unbind")->expects($this->once());
160+
$this->getFunctionMock("App\\Models", "ldap_count_entries")->expects($this->once())->willReturn(0);
141161

142-
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->once())->willReturn(false);
143162

144163
$this->expectExceptionMessage("Could not search LDAP:");
145164
$results = Ldap::findAndBindUserLdap("username","password");

0 commit comments

Comments
 (0)