connector: add LDAP connector

[fnordahl]

Authentication is performed by binding to the configured LDAP server using
the user supplied credentials. Successfull bind equals authenticated user.

Optionally the connector can be configured to search before authentication.
The entryDN found will be used to bind to the LDAP server.

This feature must be enabled to get supplementary information from the
directory (ID, Name, Email). This feature can also be used to limit access
to the service.

Example use case: Allow your users to log in with e-mail address instead of
the identification string in your DNs (typically username).

To make re-use of HTTP form handling code from the Local connector possible:

• Implemented IdentityProvider interface
• Moved the re-used functions to login_local.go

Fixes #119

[fnordahl]

Please note that this is a Work In Progress and it is probably not ready for merge at this stage.

However, I would really like some feedback/reviews on the code.

[bobbyrullo]

Hi @fnordahl, thanks for the contribution! I'm pretty inexperienced with LDAP but at a cursory glance this is looking pretty good. I'll give a closer look later on today, and try configuring it up with an LDAP server.

[fnordahl]

Cool @bobbyrullo ! I guess you can use this Docker-image to test: https://github.com/osixia/docker-openldap I have not tested it myself, but it seems good and well documented.

Note that you can create the encrypted string to use in the userPassword attribute with the slappasswd command (in reference to the "Add a new user" step in the README.md of the linked repository).

I am happy to follow up with more complete/improved code, but would like some pointers as of how the current code looks and fits into the project, and some pointers as to how to address the issues outlined in the commit message. (Callbacks, Registration etc.)

Thank you, and I look forward to hear from you!

[bobbyrullo]

hey @fnordahl - I'm having some troubles with the PR - when I register, it seems to recognize my name/password combo, but fails after the "choose an email" part with "There was a problem processing your request." - in the logs I get ERROR: Internal Error during registration: No Identity found in session.

[philips]

Hey @fnordahl Any updates? :)

[fnordahl]

Sure @philips I will be picking this up again shortly. @bobbyrullo Do you have both authAttribute and uidAttribute configured, and do the fields they point to both exist? What does the Identity section of the INFO: Session log-line look like?

[bobbyrullo]

@fnordahl Here's the logs:

INFO: 127.0.0.1 - - [30/Nov/2015:15:08:33 -0500] "POST /auth/ldap/callback/login?prompt=&session_key=9Xv2iXzTUwQ%3D HTTP/1.1" 200 21819
INFO: IDENTITY!:
INFO: Connecting to 192.168.59.103:8989
INFO: binbDN: cn=admin,dc=example,dc=org
INFO: GOT HERE%!(EXTRA string=cn=admin,dc=example,dc=org)
ERROR: IDENTITY: err: <nil>
INFO: Session zht3VRPKnHU= remote identity attached: clientID=XXX identity=oidc.Identity{ID:"", Name:"admin", Email:"", ExpiresAt:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}
INFO: 127.0.0.1 - - [30/Nov/2015:15:08:36 -0500] "POST /auth/ldap/callback/login?prompt=&session_key=9Xv2iXzTUwQ%3D HTTP/1.1" 307 0
INFO: 127.0.0.1 - - [30/Nov/2015:15:08:36 -0500] "POST /auth?client_id=XXX&msg_code=register-maybe&redirect_uri=http%3A%2F%2F127.0.0.1%3A5555%2Fcallback&register=1&show_connectors=ldap&state= HTTP/1.1" 405 38

[bobbyrullo]

Here's my connector...looks like I need to have UID attribute set?

  {
    "id": "ldap",
    "type": "ldap",
    "serverHost": "192.168.59.103",
    "serverPort": 8989,
    "emailAttribute": "mail",
    "authAttribute": "cn",
    "baseDN": "dc=example,dc=org"
  }

[fnordahl]

@bobbyrullo Yes, in the current state of the code you need to have the uidAttribute set.

I have written better initialization of the connector on my TODO-list. Some of these settings could probably also often be initalized with good default values, or even autodetection.

[steveej]

@fnordahl any updates on this one? I'd love to see SSL support for this connector as an additional item in TODO ;-)

[bobbyrullo]

hey @steveej et al:

Sorry, for letting this one go - I couldn't get it working quite right, so I gave up on it for a while. I'm going to tinker some more this week and I should have an update soon.

[ericchiang]

Some comments on the authentication strategy. Currently the connector uses the user's credentials to bind to the LDAP directory, but this can create issues if the user doesn't have permission to search the directory.

Creating an account for the application (in this case dex) seems like a normal strategy. See

Your application therefore should have an account used to authenticate such as cn=My Killer App,ou=Apps,dc=example,dc=com. The directory administrator can then authorize appropriate access for your application, and also monitor your application's requests to help you troubleshoot problems if they arise.

- Best Practices For LDAP Application Developers (OpenDJ Directory Services Project)

And

If your application needs to authenticate, obtain a regular account to authenticate with the directory, rather than using the directory superuser account (such as cn=Directory Manager). When you authenticate as directory superuser, you bypass normal access control mechanisms. Bypassing normal access control renders auditing directory access more difficult.

- LDAP Client Best Practices (Oracle)

That would add another field to the connector config.

  {
    "id": "ldap",
    "type": "ldap",
    "serverHost": "192.168.59.103",
    "serverPort": 8989,
    "emailAttribute": "mail",
    "authAttribute": "cn",
    "baseDN": "dc=example,dc=org",
    "user": "dex",
    "password": "foo"
  }

[fnordahl]

@steveej @bobbyrullo I will try to get some time to look at this again soon! Thank you for the patience and sustained interest :-)

[fnordahl]

@ericchiang having a separate configurable LDAP account for this is indeed one of the often used strategies.

Your second citation refers to authenticating with the superuser, that is not what we are doing here.

However, it is my professional opinion that allowing the individual user to bind to your directory is much more secure. The reason for this is that to authenticate through searching with a separate LDAP account you must give that account read access to the password hashes. In the event that the application with this access is compromised you risk leaking all your password hashes.

There will be implemented support for configuring a separate LDAP account for doing search for DN before binding as that DN (i.e. to allow the user to log in with e-mail address or similar).

I will look into the possibility for making authentication mode configurable to also include your preference, but I cannot make any promises.

[ericchiang]

However, it is my professional opinion that allowing the individual user to bind to your directory is much more secure. The reason for this is that to authenticate through searching with a separate LDAP account you must give that account read access to the password hashes. In the event that the application with this access is compromised you risk leaking all your password hashes.

Wow that's slightly terrifying. Didn't realize search gives you hashes.

core@core-01 ~ $ cid=$(docker run -d osixia/openldap)
core@core-01 ~ $ docker exec -it $cid bash
root@5d4f61b54ff2:/# ldapsearch -x -h localhost -b dc=example,dc=org -D "cn=admin,dc=example,dc=org" -w admin                                                                                                                                                                 
# extended LDIF
#
# LDAPv3
# base <dc=example,dc=org> with scope subtree
# filter: (objectclass=*)
# requesting: ALL
#

# example.org
dn: dc=example,dc=org
objectClass: top
objectClass: dcObject
objectClass: organization
o: Example Inc.
dc: example

# admin, example.org
dn: cn=admin,dc=example,dc=org
objectClass: simpleSecurityObject
objectClass: organizationalRole
cn: admin
description: LDAP administrator
userPassword:: e1NTSEF9Q2VxRVplTXV0bkFHRjd3UWcxanlVZ1FxYkdNajNkUlg=

# search result
search: 2
result: 0 Success

# numResponses: 3
# numEntries: 2
root@5d4f61b54ff2:/# echo -n "e1NTSEF9Q2VxRVplTXV0bkFHRjd3UWcxanlVZ1FxYkdNajNkUlg=" | base64 --decode | xargs echo
{SSHA}CeqEZeMutnAGF7wQg1jyUgQqbGMj3dRX

[fnordahl]

@ericchiang With the admin user or with a not properly configured ACL you indeed get access to the password hashes. You can configure the ACL so that individual users only have access to authenticate against (bind) or write to the userPassword field, not read it.

[bobbyrullo]

If it's at all a common practice to not allow all users to search (as I believe it is), then we need to allow the option of specifying the searching user.

If we don't allow that we are dictating how other people must set up their LDAP systems.

[ericchiang]

You can configure the ACL so that individual users only have access to authenticate against (bind) or write to the userPassword field, not read it.

In this scenario of using a searching user, would it then sufficient to recommend (or even require) that that user has auth but not read access to userPassword hashes?

[bobbyrullo]

In this scenario of using a searching user, would it then sufficient to recommend (or even require) that that user has auth but not read access to userPassword hashes?

I'd say recommend, not require

[fnordahl]

Updated version:

• Redirect to "register-maybe"-page now works (fixed in separate PR)
• TLS/SSL-support implemented ( @steveej )
• Set up good default values for LDAP Attribute names when not configured

[fnordahl]

The Travis CI build check fails on the external dependency on the LDAP library. This has passed before, has there been any updates/changes on the CI? @bobbyrullo @ericchiang

[ericchiang]

@fnordahl you have to vendor the package using Godeps. That's something we should have better documented anyway, so let me draft something for the entire repo and I'll point you to that.

[ericchiang]

@fnordahl please see #289 for instructions on vendoring the LDAP package. If you have any questions or comments feel free to add feedback on that issue.

[fnordahl]

Updated version:

• Add Godeps dependencies for LDAP library
• Implement search before auth functionality, this also meets the operators need for custom search filters.

[ericchiang]

@fnordahl awesome work.

I don't have any feedback beside my inline note on ldapConn race conditions (though that's going to be important). Please let us know if you have any questions around that.

Going to spin up a LDAP server tomorrow and try this connector out.

[fnordahl]

@ericchiang thank you.

In regards to the use of ldapConn I'd have to admit that I have not been thorough enough in my research of what dex or the Go language itself does and does not do for you when it comes to concurrency. I guess I got a bit eager to just cross another item off the list at some point. Thank you for pointing it out to me.

I will look into implementing a connection pool for the LDAP connections.

Failing that we could revert to creating a new connection for each call to Identity() and have the connection pooling be a feature for the next release, depending on how much time we have before we want this released. LDAP connection initialization and tear down tend to be light weight operations for most LDAP servers and workloads out there.

I look forward to hear the results of your trials with the connector.

[ericchiang]

@fnordahl After looking at the code, I think we should just create a single connection every time for this PR. Remove the ldapConn field on the LDAPConnector and just add a Dail() method to the connector which returns an *ldap.Conn object whenever you need one.

Let's get this merged then worry about connection pooling.

[fnordahl]

@ericchiang I agree, let's get this baby flying.

Updated version:

• Revert attempt at LDAP connection re-use, postpone proper connection pooling to a future version.
• Add unit tests for certFile+keyFile assertion.

[fnordahl]

Found and reverted one last occurrence of arguments to function call split over multiple lines to be within 79 character width.

[fnordahl]

Removed the debug log output from the OpenLDAP container during the Travis CI functional-tests.

[fnordahl]

It seems like one of the test runs were unsuccessfull because of transient failure unrelated to the committed changes. Would it be possible to re-run the test? @ericchiang

[fnordahl]

Rebased the PR to trigger a re-check.

[ericchiang]

LGTM

[fnordahl]

\o/