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

Add slightly better Active Directory support #94

Closed
wants to merge 19 commits into from

Conversation

jfautley
Copy link

@jfautley jfautley commented Jul 12, 2018

This change does the following:

It is anticipated this makes using JupyterHub when authenticating against Microsoft AD somewhat easier.

@jfautley
Copy link
Author

bump

Is this something you're interested in (in which case I'll work on rebasing against master) or is there a better way to handle binding to AD?

@dhirschfeld
Copy link
Collaborator

Unfortunately I've got zero bandwidth to look at this just at the moment. As I'm using Active Directory I do plan on looking at this when time allows but for me at least that's measured in weeks rather than days...

@@ -62,6 +63,9 @@ def _server_port_default(self):
uid={username},ou=people,dc=wikimedia,dc=org,
uid={username},ou=Developers,dc=wikimedia,dc=org
]

Active Directory Example:
DOMAIN\{login}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious why this is needed? I'm on AD and I login with my samid (DOMAIN\samid) which is used to look up my CN which is then substituted into my DN template.

Copy link
Author

Choose a reason for hiding this comment

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

For people with weird and wonderful OU configurations setting templates in bind_dn is either very complex or potentially impossible (especially if you don't directly control Active Directory and the admin team keep making changes!). Being able to use standard AD syntax of DOMAIN\username to bind with makes life much easier and means the JH admin doesn't have to care about the backend AD structure.

"""
)

activedirectory = Bool(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems a bit of a code-smell to have both get_groups_from_user and activedirectory. I have a niggling feeling that there must be a better way to do this... but I'm not an expert so, maybe there isn't?

Having more configuration knobs to turn adds complexity and this is especially true if they're only valid in some situations and not others. Given that I'd want to make sure they were really necessary and the benefit outweighed the added complexity (and hence maintenance burden)

Copy link
Author

Choose a reason for hiding this comment

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

The reason for the different configuration setting is to ensure we can use the AD specific search functionality to look up users in nested groups. AD user objects only have groups the user is a direct member of as a memberOf attribute on their object.

For example, if we have group 'JH Users' which contains 'Lab APAC' and 'Lab EMEA' groups, a user in 'Lab EMEA' won't also show up as being in 'JH Users' when you query the memberOf attribute on the user object, so you need to use the AD specific search syntax.

There is not, as far as I know, an easy way to tell if you're talking to 'Active Directory' LDAP or another implementation hence the configuration option. An alternative might be to test the AD specific search and if it fails, fall back to the "standard" behaviour but to me this seems to smell even more than the 'duplicate' code.

@sigurdurb
Copy link

sigurdurb commented Oct 16, 2018

You know you can authenicate against groups with memberOf by just adding it to the lookup_dn_search_filter
Here is an example:

c.LDAPAuthenticator.lookup_dn_search_filter = '(&({login_attr}={login})(memberOf=CN=SomeCnName,OU=SomeOu,OU=SomeOU,DC=bla,DC=com))'

and also using c.LDAPAuthenticator.user_attribute = 'sAMAccountName' that maps to the {login_attr} in the variable above.

@myoung34
Copy link

we really need this.
Right now the way it works is only by a specific OU string which is terrible.

Group membership would be much better

@jfautley
Copy link
Author

Seems like there's at least one other person who would find this useful so I'll have a go at rebasing the changes to master.

@dhirschfeld
Copy link
Collaborator

Does the memberOf trick mentioned by @sigurdurb not work for you?

@jfautley
Copy link
Author

Does the memberOf trick mentioned by @sigurdurb not work for you?

I don't see how that actually fixes the underlying issue - it would only work for a single group, and still relies on the memberOf attribute which isn't populated for nested groups in AD.

@jfautley
Copy link
Author

I've reworked the logic here a bit, and twiddled some config options which hopefully make it a bit less finicky to configure, and simplify both AD and non-AD configurations and make it clear which is which.

@dhirschfeld
Copy link
Collaborator

I'm pretty swamped (and don't really consider myself a proper maintainer) but I'll try to set aside some time to take a look...

@jfautley
Copy link
Author

Just a gentle reminder here - it would be nice to get this merged (or rejected so I can work on an alternative if this PR isn't suitable).

Active Directory (for better or worse...) is very popular in enterprise environments and this PR provides better integration with the Microsoft-way of managing LDAP.

If you don't want the code-smell of if ad: <code> else: <almost the same code> then I'm also happy to fork the repo and maintain a specific Active Directory authenticator for JupyterHub but I'd (personally) rather keep the code integrated (also I don't really want to maintain a fork of an already solid codebase!).

@jfautley jfautley force-pushed the fix-ad-binding branch 2 times, most recently from ef6c7e0 to b90fab3 Compare July 30, 2019 12:39
Also add Microsoft Active directory specific lookup logic to resolve
nested groups.
if self.activedirectory:
self.log.debug('Active Directory enabled')
user_dn = self.resolve_username(username)
search_filter='(member:1.2.840.113556.1.4.1941:={dn})'.format(dn=escape_filter_chars(user_dn))

Choose a reason for hiding this comment

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

1.2.840.113556.1.4.1941 feels like a magic string, would extracting it as a well known constant be acceptable to you? https://ldapwiki.com/wiki/1.2.840.113556.1.4.1941

@consideRatio consideRatio mentioned this pull request Jun 4, 2023
12 tasks
@consideRatio
Copy link
Member

Despite "slightly better" in the title, this looks like a lot of great work! Thank you @jfautley for the effort into this and everyone involved in review efforts!

I've recently onboarded myself as a maintainer to this project, and I think the right call for this PR is to close it at this point.

I note that:

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.

Members of nested groups aren't allowed if only the parent group is listed under allowed_groups
10 participants