Add Active Directory LDAP Auth support

[dlewen]

The current LDAP Auth does not work with AD, and having the current one doing both I think would have been a bit messy, so created a new one for AD.

You use your samAccountName as username, it does UPN style LDAP bind with username@domain, where domain is from the config file.

It checks for group memberships, you have to be member of either the rogroup or rwgroup to be authenticated. If you are a member of both groups you will get rw access.

Fixes #607

[tobbez]

The value in self.username should be escaped (by calling ldap.filter.escape_filter_chars on it) before being included in the filter string passed to search_s.

[plajjan]

This is a great point and applies to the already existing LDAP module as well...

[tobbez]

I'm not sure whether the username needs to be escaped when performing the bind using UPN, but if it does, it should be escaped using ldap.dn.escape_dn_chars.

Aside from that and the line note, this looks good to me.

[plajjan]

Ok, so we had a lengthy discussion on IRC around this PR. To sum up, I'd like to not see an extra module for AD but that we integrate the functionality needed into the current LDAP module so that it also can be used for AD authentication.

I think much of it is based on the search filter and by having something along the lines of a dn_filter variable we could use it for both scenarios;

AD:
dn_filter = sAMAccountName=%{username},ou=Users,dc=test,dc=com
LDAP:
dn_filter = uid=%{username},ou=Users,dc=test,dc=com

• escaping et al.

The rwgroup / rogroup is new functionality (applause! :) and I would love to see the same support for "normal" LDAP, which is again a reason I don't want to have two modules. Also for the group support we should be able to handle a configuration where groups are not used, ie all users are granted RW or if we only define a rwgroup then all users get RO except for the users that are in the rwgroup who obviously gets RW access.

It also has me thinking about more granular privileges but perhaps it's not really time for that quite yet...

[tobbez]

It would be possible to merge this into the LDAP module, however it's not possible to directly bind as the authenticating user in that case.

When binding against AD⁰, you can either use the UPN (as in this PR), or you can use the CN (usually the user's real name). This means, that to authenticate the user using their sAMAccountName, you need to:

1. Bind as another user
2. Search for the CN of the authenticating user (the filter used can be configured by the end user)
3. Unbind and try to bind using the authenticating user's CN.

This extra user would need to be configured in AD and NIPAP, and have read access to the relevant trees.

⁰: https://msdn.microsoft.com/en-us/library/cc223499.aspx

[dlewen]

Escaping added
Merged the AD code into the LDAP module, almost as we discussed, please look at it and let me know what you think.
Groups in groups are untested, not sure how that works, if it doesn't I'll look into implementing having groups as a list in the config.

[plajjan]

According to https://github.com/SpriteLink/NIPAP/pull/697/files#diff-1ac79fd5b11080d64d2820829861e54eR143 one should get RW by default, so if ro_group is defined and search fails we should give read/write, correct?

[dlewen]

Maybe, depends. If one is using the ro_group its probably for some security reason, if the search fail I would prefer giving normal users read only over giving untrusted users read/write. I guess this will most likely happen if there is a LDAP miss configuration. If your normal users don't get write access they will let you know and you can fix the problem, your untrusted users will probably not tell you that they got write access by accident, so you will never know.

[tobbez]

I'd agree with giving read only rather than read/write access here.

However, this except clause will only be reached if search_s fails, which should never happen, except when it's passed an invalid filter string (_ldap_search is misconfigured), or when there's a problem communicating with the LDAP server (e.g. connection has dropped since binding). The former is likely to be much more common.

Maybe it would be better to raise an exception instead?

[plajjan]

I'm sorry, I read this on a mobile device and didn't get the whole context when I initially commented.

If we only end up in here because of something like an invalid filter string, we should throw an exception instead and please try to refrain from using just except - try to match on the specific exception instead.

[dlewen]

Sure, do you happen to know what ldap exceptions I can get from a search_s? Or should I use LDAPError? We can also get KeyError if the memberOf attribute is missing at if self._ldap_ro_group in res[0][1]['memberOf']:. At least from my tests the search_s succeeds when the attribute is missing.

[dlewen]

We will also end up in the except if the username does not exist at all (but then the bind should fail) or if the basedn is wrong, or if the username is not found with the configured basedn.

[plajjan]

Dunno if you are interested in the python3 compatibility effort going on over in #611. I don't mind keeping the two issues separate and convert the LDAP implementation to ldap3 once this PR is merged, but if you are interested in writing for py3 I would happily merge a PR that fixes both AD and py3 compatibility in one go.

Regardless, before merging anything I want a squashed history - no need to see the rejected code in our history.

[dlewen]

I prefer keeping it separate, it will be to much work for me right now to test this code with py3.