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 ldap support #815

Merged
merged 65 commits into from
Jun 7, 2019
Merged

add ldap support #815

merged 65 commits into from
Jun 7, 2019

Conversation

kevinpapst
Copy link
Member

@kevinpapst kevinpapst commented May 27, 2019

Description

Documentation can be found at: https://www.kimai.org/documentation/ldap.html

Features:

  • Authenticate against an LDAP
  • New users will be created on the fly
  • Configured attributes will be synced on every login
  • Mapping LDAP groups to Kimai roles is possible

TODO

  • prevent setting an internal password
  • get rid of the service class name parameter
  • convert ldap container parameter to config class
  • add group mapping query

How to test

  • checkout the branch
  • composer install
  • adjust config/packages/local.yaml to your needs (see documentation)
  • bin/console cache:clear

Open questions for EVERY reader

  • How do other apps handle LDAP support?
  • Should an app still offer registration when LDAP is activated?
    • Leave that configurable or turn it off always?
    • I guess we need to avoid problems like a local account with the same account name in LDAP, then the LDAP account would never be checked?
  • Should groups be transferred?
    • How? Is there a general approach?
    • I read something about a "memberof" field/attribute which seems to only a OpenLDAP feature?
  • The login needs to be checked against LDAP every time?
  • What should happen to the local account if an account in LDAP is deleted?
  • Can LDAP accounts be deactivated?

If anything else comes to your mind, please share it. This is one of the features I have to develop without having a proper LDAP setup/knowledge, so I am "flying blindly" here and need YOUR input.

Fixes #193
Fixes #795
Fixes #449

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I verified that my code applies to the guidelines (composer code-check)
  • I updated the documentation (see here)
  • I agree that this code is used in Kimai and will be published under the MIT license

@kevinpapst kevinpapst added this to the 1.0 milestone May 27, 2019
This was referenced May 27, 2019
@army1349
Copy link

Thanks for your work on this, this is my view on your questions:

* How do other apps handle LDAP support?

Some apps I use with LDAP:
Nextcloud
MediaWiki
Support of multiple servers and StartTLS/LDAPS are always good to have.

* Should an app still offer registration when LDAP is activated?
  * Leave that configurable or turn it off always?
  * I guess we need to avoid problems like a local account with the same account name in LDAP, then the LDAP account would never be checked?

Probably no. Local accounts coexisting with LDAP accounts are with exception of some kind of admin account uncommon.

* Should groups be transferred?

If groups are important part of an app, then it is always nice to have support for LDAP groups.

  * How? Is there a general approach?
  * I read something about a "memberof" field/attribute which seems to only a OpenLDAP feature?

People which want to use LDAP generally know how to use it, so I would let them configure it. Group base, group name attribute, group search filter, membership attribute and optionally reverse membership attribute (memberof) could do it. Example:
group base: ou=groups,dc=example,dc=com
name attribute: cn
group search filter: (objectClass=groupOfNames)
member attribute: member
reverse member attribute: memberOf

* The login needs to be checked against LDAP every time?

Yes, correct way is to try to bind to LDAP with user credentials.

* What should happen to the local account if an account in LDAP is deleted?

Some kind of auto cleanup option is nice to have feature, but not without confirmation, or time delay, or other foolproof mechanism.

* Can LDAP accounts be deactivated?

Again, I would leave that to admin. User search filter should do it all. Example:
(&(objectClass=posixAccount)(memberof=cn=kimai,ou=groups,dc=example,dc=com))

@kevinpapst
Copy link
Member Author

Thanks @army1349 for your input!
After more digging and thinking about your input, that leaves me with just one minor open point, before we can start testing a first version:

  • transfer the permission from LDAP to Kimai (some kind of group mapping, still unsure how)

Some feedback to your points:

  • deleting user: won't do anything here, normally you don't want to delete timesheet records for users how left the company
  • users who were imported via LDAP are flagged (empty password, DN is cached) and login attempts will be rejected if they can't be found in the LDAP (AKA deactivated)
  • duplicate local vs ldap accounts: won't do anything here, as this is up to the admin to handle (worst case scenario: rename the user in the DB)
  • SSL and StartTLS seem to be supported, even though I don't know how to test it locally.

A couple of things can't be added easily, as I am using another library for the groundwork. Especially the group / role transfer for permissions is unclear to me. What I can provide as solution, is mapping LDAP attributes to the user and from there to Kimai groups.
For example if a user has the field groupId with value 13 I could offer a configurable mapping 13: ROLE_ADMIN. Would something like that help?

@army1349
Copy link

army1349 commented May 29, 2019

Glad I can help.

A couple of things can't be added easily, as I am using another library for the groundwork. Especially the group / role transfer for permissions is unclear to me. What I can provide as solution, is mapping LDAP attributes to the user and from there to Kimai groups.
For example if a user has the field groupId with value 13 I could offer a configurable mapping 13: ROLE_ADMIN. Would something like that help?

I would not use groupId or gid attribute. Let's split this problem into few smaller ones.

  1. Which LDAP groups should be visible/added?
    Admin could specify that like this:
    group base: ou=groups,dc=example,dc=com
    name attribute: cn
    group search filter: (&(objectClass=groupOfNames)(|(cn=myKimaiGroup1...)(cn=myKimaiGroup2...)))
  2. Who is member of this groups?
  • If admin can specify membership attribute like:
    'member attribute: member'
    then you can query group for that membership attribute and you will get all members.
  • If admin can specify reverse membership attribute like:
    reverse-member attribute: memberOf
    then you can query user for that reverse membership attribute and you will get all groups, which user is member of.

@kevinpapst
Copy link
Member Author

@army1349 Could you maybe setup and share such an example configuration as LDIF file? I am missing the knowledge how to administrate an LDAP server or setup such a structure.
But at least I am able to import the data and build the code around it ;-)

@army1349
Copy link

Here is simple example.
I use OpenLDAP with memberOf overlay, so memberOf attributes are maintained automatically according to member attributes of groups.

cn=group1,ou=groups,dc=example,dc=com:
objectClass: groupOfNames
member: uid=user1,ou=users,dc=example,dc=com
member: uid=user2,ou=users,dc=example,dc=com
...

cn=group2,ou=groups,dc=example,dc=com:
objectClass: groupOfNames
member: uid=user2,ou=users,dc=example,dc=com
member: uid=user3,ou=users,dc=example,dc=com
...

uid=user1,ou=users,dc=example,dc=com:
objectClass: inetOrgPerson
memberOf: cn=group1,ou=groups,dc=example,dc=com
...

uid=user2,ou=users,dc=example,dc=com:
objectClass: inetOrgPerson
memberOf: cn=group1,ou=groups,dc=example,dc=com
memberOf: cn=group2,ou=groups,dc=example,dc=com
...

uid=user3,ou=users,dc=example,dc=com:
objectClass: inetOrgPerson
memberOf: cn=group2,ou=groups,dc=example,dc=com
...

@kevinpapst
Copy link
Member Author

kevinpapst commented May 29, 2019

Thanks for sharing, I could import it after some modifications.

For clarificaton: I can't use your suggested approach above, because I would have to write all the necessary code myself. And I'd like to stick to a tested bundle as far as possible. So we have to find a solution that works with the bundle logic:
the bundle uses a bind to authenticate the user. But if a new user authenticates the first time, it is performing a search to fetch all fields that should be imported into Kimai. These fields can now be mapped to the user and this mapping is under my control.

I don't see the memberof field with that query:

ldapsearch -LLL -x -b "ou=people,dc=test,dc=local"

But only when adding these (operational search attributes?):

ldapsearch -LLL -x -b "ou=people,dc=test,dc=local" "+" "*"

I could patch the used search query to use those attributes, but for now hard-coded, with such an result:
Bildschirmfoto 2019-05-29 um 14 44 15

And now my follow-up questions...

Are these search attributes OpenLDAP specific?
Do they need to be configurable (e.g. when using an AD, would that fail)?

How do we map the memberof to Kimai groups. In the example from the screenshot, I could imagine a simple config like this:

"cn=group1,ou=groups,dc=kimai,dc=org" => ROLE_TEAMLEAD 
"cn=group2,ou=groups,dc=kimai,dc=org" => ROLE_ADMIN

The configuration is completely up to the admin, so both sides are under your control.

Could that work or am I completely on the wrong track here?

@army1349
Copy link

the bundle uses a bind to authenticate the user. But if a new user authenticates the first time, it is performing a search to fetch all fields that should be imported into Kimai.

If this is how it works, then I would leave LDAP groups idea for now.
Group membership can change over time, so onetime LDAP search for membership is not an option.
Groups would have to be checked periodically using dedicated LDAP user.
For example my Nextcloud is checking groups using user:
uid=nextcloud,ou=system,ou=users,dc=example,dc=com
This would require more work and need for this is questionable, so I propose using internal groups with LDAP users for now a and wait for feedback. What do you think?

@kevinpapst
Copy link
Member Author

Thanks for all your input!

I finally found a way to sync all fields on each login (abusing the framework a bit, but who cares, it works), updated the branch and added extensive documentation: https://www.kimai.org/documentation/ldap.html

I wouldn't mind some feedback on that.

@army1349
Copy link

Don't mention it.
Is it OK for Kimai, that group membership will update from LDAP only when user logs in?
Do Kimai roles have fixed names like ROLE_TEAMLEAD, so there has to be a group-role mapping?

@tobybatch
Copy link
Member

@kevinpapst I'm back in the game. I don't know how far you have got but this drupal 8 module https://www.drupal.org/project/ldap uses LDAP to sync users and is written on top of the symfony framework (although not fosuser). The source code is here: https://git.drupalcode.org/project/ldap

I'll take a better look today.

@tobybatch
Copy link
Member

I agree with @army1349, pretty much totally

I think Peter's most salient point is LDAP admins tend to know what they are doing, so Kimai could mandate a group name for each of it's roles and we could add a roadmap entry to adding mapping latter if the LDAP uptake seems popular.

We don't use the memberOf to include users into groups but instead use memberuid in the group to indicate who is in a given group, e.g.

dn: cn=webdev,ou=groups,dc=foo,dc=bar
cn: webdev
gidnumber: 12345
memberuid: rupert
memberuid: harry
memberuid: rob
...

I've just seen @army1349's comment about fixed names and the answer is yes. ROLE_TEAMLEAD is fixed, https://github.com/kevinpapst/kimai2/blob/master/src/Entity/User.php#L33

If it works with your bundle then expecting a ROLE_TEAMLEAD to be in an LDAP group named cn: role_teamlead or kimai_teamlead would work for us.

@kevinpapst
Copy link
Member Author

Is it OK for Kimai, that group membership will update from LDAP only when user logs in?

Not sure that I understand the question. Kimai uses roles exclusively to check permissions while the user is performing tasks. I could add a command that you can use in a cron job, but not in this PR. If it would be a requirement for you to sync roles also in between (e.g. when you configure very long session times or use the "remember me" feature), open up a new feature request later on.

@tobybatch thanks, I'll have a look at the Drupal stuff, but I think I am too far with the other bundle. What I documented (see link in the initial posting) should already work.

I think Peter's most salient point is LDAP admins tend to know what they are doing,

Hahaha, I hope so! The thing is: I don't in this PR (at least for the LDAP part), thats why I need more input.

I don't need exact Kimai role names to be returned, I will leave the mapping as is. So you configure a mapping. This can be kimai_teamlead => ROLE_TEAMLEAD or an arbitrary string like cn=webdev,ou=groups,dc=foo,dc=bar => ROLE_TEAMLEAD.

As I already added my own query to sync attributes during each login, I can add another search to find the user groups. But I'd need detailed information how that works, including:

  • LDIFs with test data
  • query that will match the data
  • explanation how to filter the groups

I guess for the start you need to be able to define one query for the groups?!?
But how is the resultset structured? In your example @tobybatch is seems as if I get the usernames and have to iterate over them to check if one entry matches the user that authenticates. Right? Or will the username be included in the query and I get all groups of that user?

  • Will I get the exact groups of the user and map them to Kimai roles?
  • Will I get a an array of all users and iterate over the resultset to check if the user is included...

Please provide as many input as possible! This could be also a working LDIF example and a working config from another software. I should be able to extract the logic from there as well (but words make it even clearer).

@army1349
Copy link

army1349 commented Jun 7, 2019

Great work. After last commit with accountFilterFormat not set and:
filter: (&(ObjectClass=inetOrgPerson)(memberOf=cn=cloud,ou=groups,dc=example,dc=ch))
everything works like expected.

@kevinpapst
Copy link
Member Author

Docu updated once again: https://www.kimai.org/documentation/ldap.html

@army1349 in that case the initial search will execute SRCH base="yourBaseDn" scope=2 deref=0 filter="(&(uid=xxxx))" - thats probably not what you want?!

@army1349
Copy link

army1349 commented Jun 7, 2019

Docu updated once again: https://www.kimai.org/documentation/ldap.html

@army1349 in that case the initial search will execute SRCH base="yourBaseDn" scope=2 deref=0 filter="(&(uid=xxxx))" - thats probably not what you want?!

Why they are two searches and search filters anyway?

@kevinpapst
Copy link
Member Author

"Internals" - ZendLdap requires one query including the uid=%s for finding the users DN before the bind (user authorization). And I need one without the %s as well for loading the user.
I tried to avoid that, but each of the tested solutions had some kind of drawback.
And then you wrote

It is one-time setup of few value

so I though "you are right, fuck it" - the users has to configure it properly once. And I already spent a lot of hours with that (failed) optimization. Some things sound so easy but then they aren't.
If I get a daily bug report about LDAP configuration, I will maybe re-think it and give it one more try. But for now all I want is to merge that thing :D

@army1349
Copy link

army1349 commented Jun 7, 2019

"Internals" - ZendLdap requires one query including the uid=%s for finding the users DN before the bind (user authorization). And I need one without the %s as well for loading the user.
I tried to avoid that, but each of the tested solutions had some kind of drawback.
And then you wrote

It is one-time setup of few value

so I though "you are right, fuck it" - the users has to configure it properly once. And I already spent a lot of hours with that (failed) optimization. Some things sound so easy but then they aren't.
If I get a daily bug report about LDAP configuration, I will maybe re-think it and give it one more try. But for now all I want is to merge that thing :D

Yeah, I can imagine, just one proposal:
What about taking user.filter and using it twice?
And just before pushing it to ZendLdap modify it like this:
filterForZend = "(&$filter(uid=%s)"
What do you think?

@kevinpapst
Copy link
Member Author

I have to use the configured "usernameAttribute" to build that. Do you know anything about ADs?
I didn't used that approach because I feared to break stuff for the Microsoft folks.

@army1349
Copy link

army1349 commented Jun 7, 2019

I have to use the configured "usernameAttribute" to build that. Do you know anything about ADs?
I didn't used that approach because I feared to break stuff for the Microsoft folks.

Yeah, I know few things about AD. It is not that much different from OpenLDAP for basic usage like bind and searches. Just an LDAP implementation with few specifics and own preferred schemas.

@kevinpapst
Copy link
Member Author

One more "last" try ;-)

@kevinpapst
Copy link
Member Author

@army1349 your idea was implemented, don't know why it didn't work the last time, probably it was simply too late ;-)
If you guys give me a last ok, I will merge the LDAP integration.

And one more time: THANKS FOR ALL YOUR SUPPORT!!!!

@army1349
Copy link

army1349 commented Jun 7, 2019

@army1349 your idea was implemented, don't know why it didn't work the last time, probably it was simply too late ;-)
If you guys give me a last ok, I will merge the LDAP integration.

Nice! It is working for me and correct search filter is in logs.

And one more time: THANKS FOR ALL YOUR SUPPORT!!!!

My pleasure gentlemen. We did good :-)

@kevinpapst kevinpapst merged commit 0c0e9c2 into master Jun 7, 2019
@kevinpapst kevinpapst deleted the ldap branch June 7, 2019 20:48
@kevinpapst
Copy link
Member Author

Ha, 99 comments when it was finished while 99 files were changed:
Bildschirmfoto 2019-06-07 um 22 48 12

Nice one! Thanks @tobybatch thanks @army1349 !

@army1349
Copy link

army1349 commented Jun 7, 2019

Very nice!
Feel free to write me, if you have any questions in future.

@tobybatch
Copy link
Member

@kevinpapst Thanks so much for this.I'm also here to support the LDAP stuff if needed. Do you think we could have a 0.9.1 release including the LDAP auth?

@kevinpapst
Copy link
Member Author

@tobybatch Can't do that right now, as I found out (after merging) that the new ZendLdap package requires the php-ldap extension and therefor Kimail will not install on many systems, as the DLAP package is not included in default installs (my demo server included). I have to find a way around this.
Why do you need the tag? For testing?

@tobybatch
Copy link
Member

I'm only building containers against releases at the moment. I could build us a container against a commit I suppose. Let me have a think about the non-composer requirements too.

Do we have a list of supported architectures? OSX (obviously). Linux, I guess supporting rpm and deb distros should be enough. Do we try and support windows?

@kevinpapst
Copy link
Member Author

There are a couple of Kimai users on Arch (pacman?). But its, like Windows, not very high on my priority list.
I would assume that it should run on any platform with PHP, there are no big problems nowawdays with cross platform compatibility. The only issue could arise from directory traversal with the wrong slash type. But I don't do that afaik outside of tests.
Just for the sake of testing you could fork Kimai and create your own release, but then you have to switch repo URLs and I doubt that its much easier than targeting a commit?!
Otherwise people have to wait for the next release, which will take presumably a couple of weeks... I have a couple of open cleanup tasks and possible BC breaks on my todo list for 1.0.

@tobybatch
Copy link
Member

@kevinpapst We'll probably wait for the 1.0 release. I can test an arch / debian/ fedora release process if you want me to. I can easily spin up VM for each of those and run from a clean install to running kimai.

@army1349
Copy link

@tobybatch Can't do that right now, as I found out (after merging) that the new ZendLdap package requires the php-ldap extension and therefor Kimail will not install on many systems, as the DLAP package is not included in default installs (my demo server included). I have to find a way around this.

This is normal. If someone wants LDAP functionality, php-ldap extension is required.
As long as it is documented and it works without extension when LDAP auth is disabled, there is no better alternative.

@kevinpapst
Copy link
Member Author

The code works without ldap extension. The problem is that the zend-ldap composer package requires php-zend extension instead of suggesting it. That leads to composer stop the installation if LDAP is not available ... one could use the --ignore-platform-reqs composer flag, but that is really bad pratice.

@army1349
Copy link

Oh, I see. How about removing zend-ldap from kimai's composer?
One could simply:
composer require zendframework/zend-ldap
if he wants to use LDAP auth.

@kevinpapst
Copy link
Member Author

I will try, that would be the easiest way.
Lets hope the Symfony cache/autoloader will not complain about a missing dependency...

@kevinpapst
Copy link
Member Author

Found a solution which requires a bit more configuration for the initial setup.
May I ask you both again to test #846 ?

@lock
Copy link

lock bot commented Aug 9, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. If you use Kimai on a daily basis, please consider donating to support further development of Kimai.

@lock lock bot locked and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connect kimai to ldap LDAP - Active Directory Add Ldap support
3 participants