Skip to content
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

binding as an existing user "None" #55

Closed
cct-dai-sato opened this issue Oct 22, 2018 · 7 comments
Closed

binding as an existing user "None" #55

cct-dai-sato opened this issue Oct 22, 2018 · 7 comments

Comments

@cct-dai-sato
Copy link

Hi,

if len(results) < 1:

Wouldn't it be supposed to check not only the length of the results, but also what's inside?
if len(results) < 1:

The following is the output from the daemon:

results with an existent user

[('CN=my.username,OU=AADDC Users,DC=test01ad,DC=me,DC=net',
  {'objectClass': []}),
 (None,
  ['ldaps://DomainDnsZones.test01ad.me.net/DC=DomainDnsZones,DC=test01ad,DC=me,DC=net']),
 (None,
  ['ldaps://ForestDnsZones.test01ad.me.net/DC=ForestDnsZones,DC=test01ad,DC=me,DC=net']),
 (None,
  ['ldaps://test01ad.me.net/CN=Configuration,DC=test01ad,DC=me,DC=net'])]

results with a non-existent user

[(None,
  ['ldaps://ForestDnsZones.test01ad.me.net/DC=ForestDnsZones,DC=test01ad,DC=me,DC=net']),
 (None,
  ['ldaps://DomainDnsZones.test01ad.me.net/DC=DomainDnsZones,DC=test01ad,DC=me,DC=net']),
 (None,
  ['ldaps://test01ad.me.net/CN=Configuration,DC=test01ad,DC=me,DC=net'])]

Even the latter case, the script falls through and returns 200(Auth OK) which is wrong as the user in question does not exist(in my case in MS Active Directory).

@vl-homutov
Copy link
Contributor

The len() check is just a quick test for results to give better error to user if nothing was found.
The code relies on successful bind operation with a found user with provided credentials.
What filter are you using to find users and why is it returning results in case of wrong username? I.e. I'd expect that proper filter will return zero results with non-existent user.

@cct-dai-sato
Copy link
Author

cct-dai-sato commented Oct 24, 2018

The following is the filter I am using

proxy_set_header X-Ldap-Template "(&(sAMAccountName=%(username)s)(memberOf=CN=groupx,OU=AADDC Users,DC=test01ad,DC=me,DC=net)(objectClass=user))"

I thought that this would suffice to filter out the non-existent users as they would match nither of the attributes specified above.

Is my setting wrong or missing something?

@vl-homutov
Copy link
Contributor

Looks like you are hitting this issue:
https://stackoverflow.com/questions/9441594/python-ldap-and-active-directory-issue
I.e. such an answer is a referral.
You may want to disable them, but setting 'disable_referrals' options.
Since I don't have access to advanced AD installation, I can only suggest how
it works in your case. Can you please check results with 'disable_referrals' option?
If this is the case, I think we need to force stricter checks on results and give some
warning/error.
So what happens next is that passing 'None' to bind_s leads to anonymous bind (which is enabled by default, at least on AD 2003). You may want to check in server logs if this is what actually happening.

@vl-homutov
Copy link
Contributor

vl-homutov commented Oct 24, 2018

results with a non-existent user
[(None,
['ldaps://ForestDnsZones.test01ad.me.net/DC=ForestDnsZones,DC=test01ad,DC=me,DC=net']),
(None,
['ldaps://DomainDnsZones.test01ad.me.net/DC=DomainDnsZones,DC=test01ad,DC=me,DC=net']),
(None,
['ldaps://test01ad.me.net/CN=Configuration,DC=test01ad,DC=me,DC=net'])]

..and this is strange, because I see the error in such case:

[(None, ['ldap://127.0.0.1:8085/ou=more,ou=Users,dc=test,dc=local??sub'])]
User entry for 'userx' is: (None, ['ldap://127.0.0.1:8085/ou=more,ou=Users,dc=test,dc=local??sub']) dn is 'None'
localhost.localdomain - userx [24/Oct/2018 18:20:15] Error while binding as an existing user "None": argument 1 must be string, not None, server="ldap://127.0.0.1:8083", login="userx"

Which python/python_ldap versions are used?

@cct-dai-sato
Copy link
Author

cct-dai-sato commented Oct 25, 2018

You may want to disable them, but setting 'disable_referrals' options.

I have already set it like the following, but as per my understanding, the referrals are disabled by default in the module.

proxy_set_header X-Ldap-DisableReferrals "true";

I do not have the granular level of control over AD as it is the managed service on MS Azure.

Which python/python_ldap versions are used?

python: 2.7.14
python-ldap: 3.1.0

Based on the post on StackOverflow you are referring to, the "None" should be handled by the client side, shouldn't it?

@vl-homutov
Copy link
Contributor

Thank you for the information,
It looks like python-ldap indeed changed the behaviour somewhere between 2.4 and 3.1.0
to allow None to be passed into bind as valid value (meaning anonymous, while previously it resulted in type error check).
I've added some additional checks and more logging. Can you please test the patch below?
Regarding the last question: yes, it should be handled on the client side.

diff --git a/nginx-ldap-auth-daemon.py b/nginx-ldap-auth-daemon.py
index 46daf3b..bdfafff 100755
--- a/nginx-ldap-auth-daemon.py
+++ b/nginx-ldap-auth-daemon.py
@@ -228,13 +228,27 @@ class LDAPAuthHandler(AuthHandler):
                                           searchfilter, ['objectclass'], 1)
 
             ctx['action'] = 'verifying search query results'
-            if len(results) < 1:
+
+            nres = len(results)
+
+            if nres < 1:
                 self.auth_failed(ctx, 'no objects found')
                 return
 
-            ctx['action'] = 'binding as an existing user'
-            ldap_dn = results[0][0]
-            ctx['action'] += ' "%s"' % ldap_dn
+            if nres > 1:
+                self.log_message("note: filter match multiple objects: %d, using first" % nres)
+
+            user_entry = results[0]
+            ldap_dn = user_entry[0]
+
+            if ldap_dn == None:
+                self.auth_failed(ctx, 'matched object has no dn')
+                return
+
+            self.log_message('attempting to bind using dn "%s"' % (ldap_dn))
+
+            ctx['action'] = 'binding as an existing user "%s"' % ldap_dn
+
             ldap_obj.bind_s(ldap_dn, ctx['pass'], ldap.AUTH_SIMPLE)
 
             self.log_message('Auth OK for user "%s"' % (ctx['user']))

@cct-dai-sato
Copy link
Author

Thank you for the patch.

I have tested it on my machine and confirmed that the daemon works fine now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants