search

snipe / snipe-it

A free open source IT asset/license management system
https://snipeitapp.com
GNU Affero General Public License v3.0
11.1k stars 3.18k forks source link

LDAP login not working (LDAP login test not working as well) #9903

Open masternas opened 3 years ago

masternas commented 3 years ago

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:

uberbrady commented 3 years ago

I'm sorry I'm doing this by phone. But almost always I've seen this, it's been "append domain to username" checkbox?

masternas commented 3 years ago

Still isn't working, with or without the checkbox checked.

csayre commented 3 years ago

Does the debug log show anything regarding testing the ldap login?

madd15 commented 3 years ago

Can you provide screenshots of your LDAP config?

It appears that usernames are not being picked up from your LDAP server

paulb-smartit commented 3 years ago

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= Base Bind DN: ou=staff,ou=people,dc=domain,dc=tld

becomes:

uid=[My User ID].ou=staff,ou=people,dc=domain,dc=tld

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 :(

daudo commented 3 years ago

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

mail=foobar@example.com,ou=hq,dc=example,dc=com

But of course, no such DN exists in the DIT, but only uid=foobar,ou=hq,dc=example.com.

IMHO, the "authentication query" should be a real query/filter, as its name says.

rmalchow commented 2 years ago

+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:

image

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.

rmalchow commented 2 years ago

wouldnt call it a bug though ... just a quirk. allowing selecting "subtree" would deffo be an improvement. :)

uberbrady commented 2 years ago

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 uid=blah,etc=blah,whatever=blah - so this is definitely something we'd want to build in when we re-do the LDAP system. It probably would be set up as some kind of 'compatibility mode' versus 'search mode' or something along those lines. (Happy to workshop those mode names, too).

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.

rmalchow commented 2 years ago

@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

Vaxter commented 2 years ago

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. The entire setting and user searching procedure should be completely changed here. If someone needs this hack for OpenLDAP, configure SnipeIT so that: Username Field: uid LDAP Authentication query: cn=

And change LdapAd.php in app/Service on line 130 so that in else block search is done. After this ldapLogin function beginning should look like this:

    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;
            }
        }
taeys commented 2 years ago

Thanks @Vaxter that literally solved our issue.... But isn't that something snipeit should fix in general?

Vaxter commented 2 years ago

It is, and this fix is not really a fix but a hack. This should be handled differently to allow configuration to be able to handle this kind of setup.

uberbrady commented 2 years ago

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.

uberbrady commented 2 years ago

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.

uberbrady commented 2 years ago

I actually wonder if there should be another checkbox to do this search function, rather than the existing function? Have it be default-off?

rmalchow commented 2 years ago

@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 

- "OBJECT" (pretty pointless here, as it only searches the object at the base path - this is normally used for retrieving additional attributes of an object)
- "ONE_LEVEL" - this is similar to the present behaviour - you specify a base path, and only entries directly underneath this base path are searched
- "SUBTREE" - this searches anywhere under the base path.

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:

  (|(uid=%s)(email=%s))

or, for active directory:

  (|(SAMAccountName=%s)(UserPrincipalName=%s))

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.

Ldapwiki: LDAP Search Scopes
uberbrady commented 2 years ago

Yes, I like the idea of the ONE_LEVEL vs. SUBTREE distinction in the UI (with it defaulting to ONE_LEVEL so that existing users' configurations don't change). I don't think that'd be too hard to do.

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 "somebody@somewhere.com" 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 %s expression, but I can't put my finger on why that is. Something about printf-style injections? I dunno, maybe I'm just being a little bit unnecessarily nervous here; but I just really don't want to light up our helpdesk or our GitHub by making a change that I haven't thought through carefully enough. We've absolutely done it before.

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 ONE_LEVEL different than the method we use now? Is it just an extension of what we do now, or is it somehow, subtly, slightly different?"

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.

uberbrady commented 2 years ago

OK, I looked through the code - and we already do an ldap_search() for the user - once they've bound in. That search is run by them. In fact, some setups may not even have an LDAP Bind Username nor password, which we will need to continue to support. So this has some more complications than I had thought.

I think there will be three 'search modes'

I think the only difference between 'ONELEVEL' and 'SUBTREE' is that one uses ldap_search(), and one uses ldap_list(). So maybe instead it's two settings, one for 'classic snipe-it' and one for 'LDAP Scoped search?'

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 ldap_list() method is functionally identical to our previous methods, so we can at least somehow get rid of one of them? I'm not quite sure.

rmalchow commented 2 years ago

@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.

taeys commented 2 years ago

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 :)

uberbrady commented 2 years ago

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.

Vaxter commented 2 years ago

@uberbrady Just gave it a quick glimpse, but it looks ok to me.

Vaxter commented 2 years ago

@uberbrady I have pulled the most recent code and debugged. There is an inconsistency where in Ldap.php on line 124 is expected bindAdminToLdap to return ldapbind, which is not returned (you did leave a comment about it). When this return is added to bindAdminToLdap everything works, but it's not necessary to add return here. I would rather change the code using this method.

By looking at the code, the entire if block from 122 to 142 in Ldap.php can be deleted. It is not necessary to try to bind with user, really. I would rather bind with connection user right after connectToLdap, and then everything should work smoothly without the mentioned block that currently fails.

Also, since findAndBindUserLdap already connects and binds, there is no need to do the same in ldaptestlogin function of SettingsController.

rmalchow commented 2 years ago

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. i think lines 116 to 129 should simply be "check length of result", then bind with dn and password, then read $user for it's actual attributes.

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:

        # example "filterQuery" for AD 
        #  (&(|(userprincipalName="bob")(samaccountname="bob"))(objectClass="inetOrgPerson"))
        #  in AD, there are some custom filters that allow filtering for single bits in 
        #  user account control (see https://social.technet.microsoft.com/wiki/contents/articles/5392.active-directory-ldap-syntax-filters.aspx) - this is super complicated to configure, but if someone wants something special, they can just put it in the filter.

        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(length($result) < 1) {
             // exception no user found
        }
        if(length($result) > 1) {
             // exception user non unique
        }

        $userDn =  /// get dn from only element in $result

        if (! $ldapbind = @ldap_bind($connection, $userDn, $password)) {
            \Log::debug("Failed to log in to: $userDn with password '$password'");
            throw new Exception("Unable to bind as user: $userDn");
        }

        $user = /// use $ldapbind to retrieve the user object

        [ ... whatever ... ]
kepi commented 1 year ago

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.

uberbrady commented 1 year ago

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 :)

kepi commented 1 year ago

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.

uberbrady commented 1 year ago

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!

rmalchow commented 1 year ago

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.

Vaxter commented 1 year ago

I can test on OpenLDAP, np. For me the current solution actually works it only throws an exception which I have previously mentioned that $ldapbind is not returned from method. Everything else basically works. We have a patch to add that return after Docker containers are created and then everything works.