Better LDAP authentication

Issue #108 resolved
Simen Sandberg created an issue

The LDAP authentication is a bit inflexible. A better way would be to search for a user with a normal LDAP filter, and then try to bind as that user's DN.

The attached version of lib/ should do this. Note that it includes some new parameters that should be moved to the LDAP configuration page.

Also note that the default port for ldaps is 636, not 689.

Comments (11)

  1. Marcin Kuzminski repo owner

    Great ! Thanks for the extensions, I'll review the code and merge into main repository, Just a quick question

            # TODO: Add parentheses if not present:
            self.USER_SEARCH_FILTER = '(objectCategory=User)'

    This should be also a part of configuration, and users can enter it without the parentheses, which should be added than to this string ?

  2. Simen Sandberg reporter

    Yes, I think that is a good idea. The custom filter will be used in line 95. The LDAP library will return an error if you have none or double parenteses. It would be useful to add some if they're missing.

    This parameter is optional, but it's useful for e.g. MS Active Directory. (Computer accounts also has a SAM Account Name, stored in the same attributes as AD stores normal usernames.)

    Description of the parameters:

    USER_BASE_DN - Container where the user objects are stored. Often something like "OU=People,DC=example,DC=com"

    USER_ATTRIBUTE - The attribute where the user's login name is stored. For RFC 4519-compliant servers, this is "uid". For Active Directory it is usually "sAMAccountName".

    USER_SEARCH_FILTER - An optional extra LDAP filter to further limit which objects should be considered valid users. Filters should be in the syntax described in RFC 2254.

    RhodeCode looks great, by the way. I'm looking forward to test it in our network, now that LDAP is working. :)

  3. Marcin Kuzminski repo owner

    Ok, i See You separated the base_dn and user attribute, from what it was in the original implementation.

    Was that any particulate reason to do that ?

    In current implementation same behavior You can obtain by specifying such dn string tempalate




    Is the single string You have combined the USER_ATTRIBUTE (as `uid=` part) and USER_BASE_DN as part after the `uid=%(user)s`

  4. Simen Sandberg reporter

    You assume the username attribute is part of the DN. More often than not, that's an invalid assumption. For instance, Active Directory uses the CN attribute (common name) in the DN, not the sAMAccountName attribute.

    Plus, the "template" approach won't work if the directory stores the users in different organizational units under the base DN. To allow that, you need to do a subtree search.

    My user's DN is "CN=Simen E. Sandberg,OU=MSS,OU=Technicians,OU=Users,OU=mnemonic,DC=ad3,DC=mnemonic,DC=no", while the SAM Account Name is "simen". My test user has the DN "CN=simen.tst,OU=Test,OU=Role Users,OU=Users,OU=mnemonic,DC=ad3,DC=mnemonic,DC=no" and the SAM Account Name "simen.tst".

  5. Marcin Kuzminski repo owner

    That's all clear to me now. I'll patch the admin ldap interface and ldap_auth to handle new way authentication.

    Thanks You for participation.


  6. Former user Account Deleted

    I didn't know about the work being done here (and the existing patch). I did some similar work - which does what is described above as well as a few more things:

    • Adds an LDAP filter for locating the LDAP object
    • Adds a search scope policy when using the Base DN
    • Adds option required certificate policy when using LDAPS
    • Adds attribute mapping for username, firstname, lastname, email
    • Initializes rhodecode user using LDAP info (no longer uses "@ldap")
    • Remembers the user object (DN) in the user table
    • Updates admin interfaces
    • Authenticates against actual user objects in LDAP
    • Possibly other things.

    Really, this should be extended to a list of LDAP configurations, but this is a good start.

  7. Marcin Kuzminski repo owner
    • changed status to open

    Nice, thanks for great participation, I merged those codes into beta branch. And get back to them later to write migrations/tests when I finish users groups. Thanks again for great work.

  8. Marcin Kuzminski repo owner
    • changed milestone to 1.2

    Ldap extensions are integrated to current beta, so i think this one can go to 1.2

  9. Thayne Harbaugh
    • changed status to new

    Maybe the only thing remaining is to be able to specify multiple LDAP servers. It might be better to close this ticket, since all the concerns mentioned here are addressed, and open a new ticket fro multiple servers.

  10. Marcin Kuzminski repo owner

    I'm closing this one since we're getting close to 1.2 release, multiple server can be a request if anyone needs it.

  11. Log in to comment