-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LDAP login not working (LDAP login test not working as well) #9903
Comments
I'm sorry I'm doing this by phone. But almost always I've seen this, it's been "append domain to username" checkbox? |
Still isn't working, with or without the checkbox checked. |
Does the debug log show anything regarding testing the ldap login? |
Can you provide screenshots of your LDAP config? It appears that usernames are not being picked up from your LDAP server |
I've encountered the same issue, and it's a bit of a quirky implementation of LDAP auth. The way it works is by using the "LDAP Authentication query" value and "Base Bind DN* value, sandwiched with your user ID. eg. LDAP Authentication query: uid= becomes:
Which means it won't do a subtree search the user must match the exact DN. I trie setting my "Base Bind DN" to "dc=domain,dc=tld in the hope it would find my user, but it doesn;t work that way. I checked the OpenLDAP and saw it was passing exacly the DN to look for and not searching, ie. DN=uid=[My User ID].dc=domain,dc=tld - which doesn't exist :( |
I've just been hit by the same issue. We usually don't use the uid as the unique username attribute, but instead use the mail attribute, rendering the "LDAP Authentication query" to look for entries like
But of course, no such DN exists in the DIT, but only IMHO, the "authentication query" should be a real query/filter, as its name says. |
+1 from me. i was going crazy over this - expecting some weird mistake on my side. i think its reasonable to expect a subtree query. if anyone is worried about backwards compatibility, then one option would be to make it switchable (like apache directory studio: and default to "one level" - this would also avoid frustration on the user's side (like i experienced) when it is not working as expected. |
wouldnt call it a bug though ... just a quirk. allowing selecting "subtree" would deffo be an improvement. :) |
LDAP is absolutely a tough pain point for us to support and it does tend to call for a lot of esoteric knowledge of the protocol and how your directory has been configured. We've had customers who have wanted something similar - a query function that might not be based on Also, I'm always interested in hearing about other implementations that people think are good models for us to follow (ideally ones where I can at least see the source code). We don't really need to re-invent the wheel here; this has been done before. I just want us to do it as well as others do it. This would be something we would address in the Great re-LDAP'pening sometime post v6. |
@uberbrady i've been looking at your code - and although LDAP doesnt seem too esoteric to me anymore, PHP certainly is a weak point. i'd still be happy to help explaining LDAP things if you want me to. the basic pattern for something like this is this:
note that
|
I have patched this for myself in ldapLogin function to search for the user by username, fetch the cn from it, and let the rest of the app take it's course, but this is a hack and should not be done this way. And change LdapAd.php in app/Service on line 130 so that in else block search is done. public function ldapLogin(string $username, string $password): User
{
if ($this->ldapSettings['ad_append_domain']) { //if you're using 'userprincipalname', don't check the ad_append_domain checkbox
$login_username = $username . '@' . $this->ldapSettings['ad_domain']; // I feel like could can be solved with the 'suffix' feature? Then this would be easier.
} else {
$ldapUser = $this->ldap->search()->findBy('uid', $username);
if ($ldapUser) {
$login_username = $ldapUser->getFirstAttribute('cn');
} else {
$login_username = $username;
}
} |
Thanks @Vaxter that literally solved our issue.... |
It is, and this fix is not really a fix but a hack. |
Yup, it is. I'd be happy to take a PR for this fix if it were cleaned up a little so that it could be less of a 'hack' - we certainly don't want to break LDAP for everyone else. |
And, to make matters more confusing, that file doesn't even exist in Snipe-IT v6 - the LDAP system is different. But it sounds like what we want here is to search for the username using the "LDAP Username Attribute" - find that person, then try to log in using that username. We do already have the one as the "LDAP Authentication query" - so I feel like our current settings could support this. I think the big change would be doing an arbitrary-depth search under the username attribute first, then trying to log in as that username. If we can do that in a way that won't blow everyone else up. |
I actually wonder if there should be another checkbox to do this search function, rather than the existing function? Have it be default-off? |
@uberbrady the "LDAP" way of doing this would be to set the scope for the user query - the scope being part of the normal configuration of any ldap query, the normal scopes are
a bit more in-depth: https://ldapwiki.com/wiki/LDAP%20Search%20Scopes so UI-wise, you could simply ask for "ONELEVEL" or "SUBTREE" also, you can query any attribute of the entries, such as this "or" query:
or, for active directory:
this will find all matching entries. uniqueness of attributes very much depends on the LDAP implementation and additional configuration - for AD, both "SAMAccountName" and "UserPrincipalName" have to be unique, for Apache DS, "uid" has to be unique, etc. once you have the user entry, you can authenticate with it's full DN (e.g. "uid=foo,ou=bar,o=banana" and the password, if this succeeds, the authentication succeeds. |
Yes, I like the idea of the Where I feel a little...uh, "oogier" on this is the possibility of the search retrieving more than one user - for instance in your first example query, if email weren't required to be unique and I tried to log in as "[email protected]" and there were a few users with that email address - well, we'd have to reject the login and, for security purposes, we'd have to not display anything about it other than the standard 'unknown username or bad password' type of message. That certainly makes me a little uneasy - adding another way for our users to mess up an LDAP config, which I definitely don't want. I also feel awkward about the I'm going to dig through the v6 LDAP code to see if this first thing is something that we can sneak in there safely. The question I guess I'm going to try and answer is: "How is Thanks to all the users who have been updating this issue! And a super-huge shout-out to @rmalchow for talking me through some implementation ideas for this. |
OK, I looked through the code - and we already do an I think there will be three 'search modes'
I think the only difference between 'ONELEVEL' and 'SUBTREE' is that one uses One thing here that I'm not quite sure about is how this might affect AD setups. Tricky. And another thing I don't like is the additional complication - isn't there some way of guaranteeing that an |
@uberbrady regarding printf injection: these were just examples - in most ldap support libs, you have builders that take key / value pairs and take care of the escaping of evil things for you. not sure about PHP, but i guess it's there. as for non-uniqueness: there is absolutely nothing you can do about it, since - as i said - uniqueness depends on all kinds of things beyond your control. the one thing people will always be thankful for though is you printing a proper error message in the log (not to the user) when this happens. "Expected to find a unique entry, but found (X) - check your search filter" in my experience, people using LDAP are aware of this and know what to do. for the scopes - i would think "Classic" and "SUBTREE" should be sufficient - i've worked on LDAP servers a lot, and the situation that you want "ONE LEVEL" and nothing below just doesn't happen. LDAP tree just aren't structured that way. on the other hand, performance wise, this doesn't make a difference until you are way past the number of users any sane person would put into an LDAP tree. so i think you can spare yourself the trouble of spreading it three ways. |
I must say I am very happy and pleased that the devs now look into the issue and try to implement a new function! Thanks! I am indeed very afraid that if we upgrade to v6 the login might again not work, so I'm happy you're thinking about a more permanent solution :) |
There's a Work-in-Progress (WIP) PR up now that does what we're talking about. I'd love to get some eyes on it to make sure I'm on the right track. |
@uberbrady Just gave it a quick glimpse, but it looks ok to me. |
@uberbrady I have pulled the most recent code and debugged. By looking at the code, the entire if block from 122 to 142 in Ldap.php can be deleted. Also, since findAndBindUserLdap already connects and binds, there is no need to do the same in ldaptestlogin function of SettingsController. |
ok. this looks promising. a couple of remarks though: a.) you should check the length of $result - there is no practical way to know if the "username" attribute is actually unique in the LDAP implementation you're working with - so you have to check that you get exactly one. you do that, but in a roundabout way. b.) i dont think you need to treat AD different from any other LDAP - you can handle it exactly the same way. yes, there are some well-known fields (userprincipalname and samaccountname) but this is probably better left as a suggestion to the user in the UI - other fields can be used and work exactly like any other LDAP server c.) to be safe, you should perform a bind AND a read operation on the user entry. some LDAP servers simply accept the bind (even if the password is wrong), and then fail on the next read operation. more like:
|
For those struggling to get LDAP working on OpenLDAP and need it now, here is modified version from @uberbrady MR which is working for me. Without settings etc. diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php
index 4c4714762..646ec0215 100644
--- a/app/Models/Ldap.php
+++ b/app/Models/Ldap.php
@@ -97,6 +97,24 @@ class Ldap extends Model
$baseDn = $settings->ldap_basedn;
$userDn = $ldap_username_field.'='.$username.','.$settings->ldap_basedn;
+ // FIND the user first, then attempt to log in as them using the appropriate attribute
+ $filterQuery = $settings->ldap_auth_filter_query.$username;
+ $filter = Setting::getSettings()->ldap_filter; //FIXME - this *does* respect the ldap filter, but I believe that AdLdap2 did *not*.
+ $filterQuery = "({$filter}({$filterQuery}))"; // FIXME - this makes an incorrect LDAP filter if there is no filter query
+
+ if (! $results = ldap_search($connection, $baseDn, $filterQuery)) {
+ throw new Exception('Could not search LDAP: '); // FIXME - I'm worried these early returns are somehow bad, in that they might disclose whether or not users exist?
+ }
+
+ if (! $entry = ldap_first_entry($connection, $results)) {
+ throw new Exception('Could not retrieve LDAP entry'); // TODO - should this throw an exception or just return false?
+ return false;
+ }
+
+ if (! $userDn = ldap_get_dn($connection, $entry)) {
+ return false;
+ }
+
if ($settings->is_ad == '1') {
// Check if they are using the userprincipalname for the username field.
// If they are, we can skip building the UPN to authenticate against AD Use at your own risk. |
I'm starting to wonder if this actually could be a universal fix - it should work for AD as well as regular LDAP, right? Would you be interested in submitting a PR with just that change @kepi ? Then you can get some nice open-source cred :) |
I never used AD so can't really say. From what I understand if we just add condition to not run this code when append domain is set, it might work without breaking anything else. But I didn't study the code in deep. But I don't have servers to test this against. Do you think that there would be some volunteers to test it? IMHO both @rmalchow and @Vaxter seems to know more about this. I put this together from your code and their suggestions as hot-fix so my colleagues can work :) From what @rmalchow wrote, I think b) and c) might be ok with this code. With all this in mind - if you want me to open PR with this code and you can help find someone to test it, I have no problem with it. I can't promise I'll find time for fixes, but I will try. It would be great to have stable LDAP login as we are moving to use snipe more and I don't want it broken every time I upgrade. |
Sure, why don't you whip up that PR and I'll test it against our own test AD server (it has a thousand users on it so we can test pagination). Thank you! |
unfortunately, while i do have access to ADs, i cannot use them to test this. but - the logic looks right. and when configured correctly, this should work for both AD and non-AD LDAP servers, without any special treatment. |
I can test on OpenLDAP, np. |
Expected Behavior (or desired behavior if a feature request)
I have created an LDAP server and my users have been uploaded onto snipe it successfully via LDAP.
The users are supposed to be able to log in to Snipe it Application.
Actual Behavior
But ldap users cannot log in and the ldap login test fails. (Please find attached)
Please confirm you have done the following before posting your bug report:
Provide answers to these questions:
Is this a fresh install or an upgrade? fresh install
Version of Snipe-IT you're running v5.1.5 build 6055 (gd9d5b4d73)
Version of PHP you're running 7.4.3
Version of MySQL/MariaDB you're running
What OS and web server you're running Snipe-IT on Ubuntu, Apache
What method you used to install Snipe-IT (install.sh, manual installation, docker, etc) manual installation using git
WITH DEBUG TURNED ON, if you're getting an error in your browser, include that error
What specific Snipe-IT page you're on, and what specific element you're interacting with to trigger the error : LDAP page for testing and log on page
If a stacktrace is provided in the error, include that too.
Any errors that appear in your browser's error console.
Attachment :
The text was updated successfully, but these errors were encountered: