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

LDAP Recursive Search (Nested groups) - by tkr #147

Open
wants to merge 2 commits into
base: maintenance-0.20.x
Choose a base branch
from

Conversation

GeoMaciolek
Copy link
Contributor

All credit for this goes to 'tkr.' Noted this was in the original mailing list thread in 2014, see: http://www.freelists.org/post/racktables-users/Nested-LDAP-groups,6 (Fri, 15 Aug 2014 14:41:40 +0200).

This has been tested in my environment with an Active Directory server, but should be tested against other server types as well.

Added a function to perform recursive search through nested groups in AD using LDAP. Most of the code is just reused from the queryLDAPServer function.
Removed unnecessary code from the recursive ldap search function
@infrastation
Copy link
Member

cc: @trkr1410

The original patch was overlooked, most probably because life got in the way. Anyway, this would be a useful feature.

The following general points must be considered carefully when making changes to the LDAP authentication code in RackTables:

  • how the new code handles server failures and unexpected/inconsistent data
  • whether the new code can be gamed to gain extra privileges
  • how the new code performs under load and relates with the existing LDAP cache

I have not yet studied the patch to see how they apply.

@trkr1410
Copy link

Hi,

I am not sure that I completely understand what you want to test out, or
what your concerns are at this point, but I will try to comment on it
anyways.
This little code addition have been in use at my company for about 18
months now, and no problems have come up.
It seems that the search time in Active Directory is quite rapid, but we
only have a small forest, and very few nested groups, so I cant tell how it
will be in larger environments.

When it comes to server failure and unexpected/unconsistent data, the logon
procedure is the same as before, only the search query have been modified.
During the setup of nested LDAP search, I've used the top level name of the
forest as both "Server" and "Domain", "SearchDn" and "BaseDn".
Example:
$LDAP_options = array
(
'server' => 'domain.contoso.com',
'domain' => 'domain.contoso.com',
'search_attr' => 'sAMAccountName',
'search_dn' => 'DC=domain,DC=contoso,DC=com',
'base_dn' => 'DC=domain,DC=contoso,DC=com',

// The following credentials will be used when searching for the user's

DN:
'search_bind_rdn' => 'CN=racktables_service_user',
'search_bind_password' => 'racktables_password',
'displayname_attrs' => 'givenname sn',
'options' => array (LDAP_OPT_PROTOCOL_VERSION => 3, LDAP_OPT_REFERRALS =>
0),
);

Plase dont hesitate on contacting me again for further questions :-)

Mvh
Thomas R. Kristiansen

On Tue, Mar 1, 2016 at 10:29 AM, Denis Ovsienko [email protected]
wrote:

cc: @trkr1410 https://github.com/trkr1410

The original patch was overlooked, most probably because life got in the
way. Anyway, this would be a useful feature.

The following general points must be considered carefully when making
changes to the LDAP authentication code in RackTables:

  • how the new code handles server failures and unexpected/inconsistent
    data
  • whether the new code can be gamed to gain extra privileges
  • how the new code performs under load and relates with the existing
    LDAP cache

I have not yet studied the patch to see how they apply.


Reply to this email directly or view it on GitHub
#147 (comment)
.

@infrastation
Copy link
Member

Thank you for the comment! Maybe anybody could meanwhile look why these changes don't merge into the current code and make fixes?

@bpothier
Copy link
Contributor

I copied over the changes, ignored some whitespace "dupes" and created pull request #156
It says it can merge... likely a whitespace issue?
You can try updating this pull/commit and I can close my pull or use that one.. whichever...

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

Successfully merging this pull request may close these issues.

4 participants