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

User CN name lookup with specific query #32

Merged
merged 9 commits into from
Sep 27, 2017

Conversation

mateuszboryn
Copy link
Contributor

User CN name lookup with specific query and (non)anonymous ActiveDirectory search account.

@3cham
Copy link

3cham commented Mar 3, 2017

+1 for this PR

@@ -161,7 +249,7 @@ def getConnection(userdn, username, password):
username=username,
userdn=userdn
))
conn = ldap3.Connection(server, user=userdn, password=password)
conn = ldap3.Connection(server, user=escape_filter_chars(userdn), password=password)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escaping filter chars from userdn can make the valid userdn invalid, e.g. escaping "(" or ")" which are valid characters for this parameter.

Copy link
Contributor Author

@mateuszboryn mateuszboryn Mar 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I had problem with ldap3.Connection() for non-escaped '(' and ')'. At least for Active Directory - I don't know how it would work for other implementations.
In my case ldap3.Connection().bind() failed with error message saying claiming that authentication failed when user parameter had '(' or ')' in it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fascinating. For us it was the inverse - we had to remove the escaping because the escaped parentheses caused problems. We're using AD, too.

According to https://social.technet.microsoft.com/wiki/contents/articles/5312.active-directory-characters-to-escape.aspx '(' and ')' do not belong to the characters to be escaped.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For us it requires to be without the "escape_filter_chars()" function. We have usernames that have a dash in them, like foo-bar. Does not work with the escape wrapper, but does work without it.

@jhereth
Copy link

jhereth commented Mar 9, 2017

Other than my comment regarding "escape_filter_char": +1

Thanks @mateuszboryn , this was very helpful for our project.

@beenje
Copy link

beenje commented Mar 24, 2017

Thanks for this pull request! It allowed me to make this plugin work with our Active Directory server.
I had to use the following config:

c.JupyterHub.authenticator_class = 'ldapauthenticator.LDAPAuthenticator'
c.LDAPAuthenticator.server_address = 'myserver'
c.LDAPAuthenticator.bind_dn_template = '{username}'
c.LDAPAuthenticator.lookup_dn = True
c.LDAPAuthenticator.lookup_dn_search_filter = '({login_attr}={login})'
c.LDAPAuthenticator.lookup_dn_search_user = 'ldap_search_user'
c.LDAPAuthenticator.lookup_dn_search_password = 'secret'
c.LDAPAuthenticator.user_search_base = 'OU=Users,DC=company,DC=com'
c.LDAPAuthenticator.user_attribute = 'sAMAccountName'
c.LDAPAuthenticator.lookup_dn_user_dn_attribute = 'cn'

It would be nice to have this merged.
Anything I can do to help this happen?

@laurikoobas
Copy link

Worked for us as well, please merge this (without the escape_filter_chars() part).

@mateuszboryn
Copy link
Contributor Author

I'm going to add new parameter that controls escape_filter_chars(). I think this will quick solution.

@mateuszboryn
Copy link
Contributor Author

Escaping userdn fixed. Now there is an option that enables escaping. This new option defaults to False.

@DrewRay
Copy link

DrewRay commented Apr 19, 2017

will this be added to the main repo? looks like a solid idea/improvement.

@beenje
Copy link

beenje commented Apr 20, 2017

@mateuszboryn, I thought I could just use c.LDAPAuthenticator.bind_dn_template = '{username}' but that doesn't work for all users. We still have to provide a list of bind_dn_template based on the CN retrieved. This is not convenient with our setup.
I did some tests with ldap3 and realised that we could get the user dn from the search result: conn.response[0]['dn']
I could just use that directly instead of retrieving the cn and then building the dn based on cn and bind_dn_template. I don't know much about LDAP. So this is maybe specific to AD?
Any reason you didn't go that way?

@pratik705
Copy link

pratik705 commented May 18, 2017

I tried using updated "ldapauthenticator.py" file(https:////raw.githubusercontent.com/mateuszboryn/ldapauthenticator/ebbdfdbeaa199b08ebd00ac31c6904969080411d/ldapauthenticator/ldapauthenticator.py). But, I am unable to connect AD.

Configuration details:
$ /usr/local/bin/jupyterhub --version
0.7.2

c.JupyterHub.authenticator_class = 'ldapauthenticator.LDAPAuthenticator'
c.LDAPAuthenticator.server_address = 'IP of AD'
c.LDAPAuthenticator.bind_dn_template = '{username}'
c.LDAPAuthenticator.lookup_dn = True
c.LDAPAuthenticator.use_ssl = False
c.LDAPAuthenticator.lookup_dn_search_filter = '({login_attr}={login})'
c.LDAPAuthenticator.lookup_dn_search_user = 'username to connect AD'
c.LDAPAuthenticator.lookup_dn_search_password = 'password to connect AD'
c.LDAPAuthenticator.user_search_base = 'CN=Users,DC=abc,DC=xyz'
c.LDAPAuthenticator.user_attribute = 'sAMAccountName'
c.LDAPAuthenticator.lookup_dn_user_dn_attribute = 'cn

It fails with :

[W 2017-05-18 10:45:34.892 JupyterHub ldapauthenticator:231] Can't connect to LDAP
[D 2017-05-18 10:45:34.892 JupyterHub login:95] Failed login for test_user

From the same server, I am able to fetch list of users with ldapsearch command.

Is there anything wrong in the configuration ?

@pratik705
Copy link

pratik705 commented May 18, 2017

ok. Its working with below configuration:

c.JupyterHub.authenticator_class = 'ldapauthenticator.LDAPAuthenticator'
c.LDAPAuthenticator.server_address = 'xyz'
c.LDAPAuthenticator.bind_dn_template = 'CN={username},CN=abc,DC=abc,DC=abc'
c.LDAPAuthenticator.lookup_dn = True
c.LDAPAuthenticator.use_ssl = False
c.LDAPAuthenticator.lookup_dn_search_filter = '({login_attr}={login})'
c.LDAPAuthenticator.lookup_dn_search_user = 'abc'
c.LDAPAuthenticator.lookup_dn_search_password = 'abc'
c.LDAPAuthenticator.user_search_base = 'DC=abc,DC=abc'
c.LDAPAuthenticator.user_attribute = 'sAMAccountName'
c.LDAPAuthenticator.lookup_dn_user_dn_attribute = 'cn'

When the changes will be merged in master branch?


But, I need to manually create user on local system. If its not there then it fails with :

KeyError: 'getpwnam(): name not found: test-user'

any suggestion ?


@efaurie
Copy link

efaurie commented May 19, 2017

@mateuszboryn thanks for this PR it's exactly what I needed. I can verify that I got this working as expected.

@pbandark I used a combination of the changes proposed in this PR and those in PR#36. Combining them I was able to get both LDAP and the local user creation working.

@pratik705
Copy link

@efaurie thanks for reply. Yes. It started working for me after combining PR#36

@minrk minrk merged commit 8799094 into jupyterhub:master Sep 27, 2017
@minrk
Copy link
Member

minrk commented Sep 27, 2017

Thanks! Sorry for the delays on this repo.

@ixxie
Copy link

ixxie commented Sep 28, 2017

@beenje - did you get it to work by using something like realmd to automatically create user accounts on the Linux system?

I am kind of at a loss as to why one would even use this plugin otherwise, since PR #36 wasn't accepted and it is recommended to use something like realmd to link up SSSD with the LDAP directory. It would be helpful to explain this clearly in the documentation, especially once this PR is released; explicit instructions for setting up Active Directory would also be handy for many I believe.

Edit: I just saw there is some explanation of AD on the README, but it was just added today (sorry I missed it!)

@beenje
Copy link

beenje commented Sep 28, 2017

@ixxie I currently use my own fork: beenje@0c0583e

Now that this was merged, I might try to submit a pull request.

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

Successfully merging this pull request may close these issues.

10 participants