Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Add LDAP group support #734

Closed
5 tasks
mssola opened this issue Feb 15, 2016 · 32 comments
Closed
5 tasks

Add LDAP group support #734

mssola opened this issue Feb 15, 2016 · 32 comments
Assignees

Comments

@mssola
Copy link
Collaborator

mssola commented Feb 15, 2016

LDAP servers store the list of users that exist in the system, and how such users are grouped. Since this is equivalent to our teams concept, maybe we can automate team membership through the configured LDAP server. Since offering some sort of synchronization between the LDAP server and Portus can be tricky, this feature will try to be as minimalistic as possible about it.

The idea

In this feature I'm not thinking about a synchronization job running in the background or anything
like that. Instead, we would provide some useful capabilities for teams that are related to an LDAP group. These capabilities are as follows:

  1. When a team is set to be related to an LDAP group, all of the LDAP users
    that are related to this group and that exist in Portus' database will be
    added to that team as team members automatically.
  2. When an LDAP user signs in Portus for the first time, Portus will add this
    new user to all the teams that are related to LDAP groups in which this user
    belongs to.

There are two ways to create the relationship between a team and an LDAP group:

  1. When creating a new team. Portus will display LDAP-specific options in the form for creating a new team.
  2. In the teams#show page. There should be a form allowing an admin of the team (or admins of Portus) to create the relationship.

When creating the relationship between a team and an LDAP group, the admin will have the ability to specify the role in which the users that are added automatically will have. For example, if I create a team named test and I relate it to an LDAP group with the role viewer, all the users that belong to that group will be added automatically to the test team as viewers.

Moreover, a team related to an LDAP group can add team members that are not
related to the LDAP group, while keeping the LDAP server untouched. That is,
any addition/removal of members in a team are only kept on Portus' side.
Therefore, this integration is only for automating the membership of users
that are known to belong to an LDAP group. In the same spirit, when an admin
creates a relationship between a team and an LDAP group, any existing team
members will not be removed from the team.

In the same way that we can create the relationship between a team and an LDAP
group, we should be able to destroy such relationship. This destroy method
will not, in any way, destroy either the team, or the team members, or the
namespaces etc. currently related to the LDAP group. This method will only be
used to disallow any new user that signs in from then on to be automatically
added to the team. This can be useful to populate a team in a fast way, but
then closing the relationship just in case.

Finally, the table listing teams will also include a column containing the
name of the LDAP group in which a team is related. If no team is related to an
LDAP group, this column might be hidden altogether.

Changes in the configuration

Portus needs the root (base) of the tree where the LDAP group lookup should
take place. In the same spirit as for the base attribute from the ldap
configuration value, there will be the base_group for that purpose. This
attribute will default to an empty string (meaning that LDAP group integration
is disabled).

Drawbacks

The main drawback is that it can be tedious to add lots of relationships. In
order to fix this, we could add a utility in the /admin/teams page to create
multiple relationships at once. I think that this utility can be useful but I think that this can be done in another issue/PR.

Tasks

  • Provide the configuration option.
  • Implement the create/destroy methods for LDAP relationships with teams. Add UI for all of them.
  • Implement all the automation features as described in this issue.
  • Add a rake task to force synchronization. (as suggested by @lonewulf).
  • Update documentation in our gh-pages branch. We could also update the README.md file.

Edit: added the "rake task" idea.

@lonewulf
Copy link
Contributor

When an LDAP user signs in Portus for the first time, Portus will add this new user to all the teams that are related to LDAP groups in which this user belongs to.

I'm seeing some issue with that behavior, it'll lead to an inconsistency between the LDAP directory and Portus when an existing user is added in a group and that user is already known to Portus.

A background job would do the trick, but as you want this feature to be minimalistic, I would strongly advise do add a 'Re-Sync with ldap' function on the team page so that it can be updated at least manually.

Regards,

@lonewulf
Copy link
Contributor

Portus needs the root (base) of the tree where the LDAP group lookup should
take place. In the same spirit as for the base attribute from the ldap
configuration value, there will be the base_group for that purpose. This
attribute will default to an empty string (meaning that LDAP group integration
is disabled).

If you're planning on using the CN you'll probably also want a way to filter entries returned by the Directory, some implementations of LDAP having multiple entries with the same CN. Or you could use the DN but it can get quite long, and is not user-friendly.

Regards,

@mssola
Copy link
Collaborator Author

mssola commented Feb 15, 2016

@lonewulf thanks for reading this despite being a bit big :)

I'm seeing some issue with that behavior, it'll lead to an inconsistency between the LDAP directory and Portus when an existing user is added in a group and that user is already known to Portus.

This is true.

A background job would do the trick, but as you want this feature to be minimalistic, I would strongly advise do add a 'Re-Sync with ldap' function on the team page so that it can be updated at least manually.

At first I thought that since this wouldn't be something that happens too often, if you create a new LDAP group and add a user there (and that user already exists on Portus), then adding it manually on Portus wouldn't be such a big deal. The idea of this issue is to add some automation in regards to team membership (by adding some "hooks").

Having said that, having a "Sync" button in the /admin/teams page could also be useful. That being said, bear in mind that you are talking about synchronization, and I'm not :) What I mean by that is that in this proposal I'm saying that is totally fine to have "unsync'ed" groups. For example, in this proposal I'm allowing something like:

  1. Relate LDAP Group X with team X.
  2. Remove member from team X without doing it on the LDAP server

Note that in this case, if we synchronize, this removed user would be re-added to the team, and the admin didn't want that. You could say then, that we could keep a list of removed users, but then all this gets way more complex than I originally intended :)

Thus, what I want for now is to add some simple hooks that automate team memberships, no more no less. To solve your case, I think that the tool I mentioned in the "Drawbacks" section can be useful. Moreover, this tool could even (contradicting myself :P) include the "Sync" button you're mentioning. It would effectively destroy any modifications you might have done (displaying warnings about the possible outcomes of course), but this should be fine for admins that want to have an exact 1:1 mapping always.

If you're planning on using the CN you'll probably also want a way to filter entries returned by the Directory, some implementations of LDAP having multiple entries with the same CN. Or you could use the DN but it can get quite long, and is not user-friendly.

For now I was only planning to provide the group_base just as we do with the base. If you want to filter further, there's the user_filter value anyways. Wouldn't that be enough ?

@lonewulf
Copy link
Contributor

For now I was only planning to provide the group_base just as we do with the base. If you want to filter further, there's the user_filter value anyways. Wouldn't that be enough ?

I was thinking about a group_filter value, as some directories have groups in multiple places. A good example would be a multi-domain ldap, where groups would then be in at least 2 different places in the LDAP tree. Some implementations have also a compat tree (I have FreeIPA in mind), so that would make it impossible to have a common base for those multiple domains without having the compat groups included (and compat groups have the same CN as the group, but a different objectClass).

Thus, what I want for now is to add some simple hooks that automate team memberships, no more no less. To solve your case, I think that the tool I mentioned in the "Drawbacks" section can be useful. Moreover, this tool could even (contradicting myself :P) include the "Sync" button you're mentioning. It would effectively destroy any modifications you might have done (displaying warnings about the possible outcomes of course), but this should be fine for admins that want to have an exact 1:1 mapping always.

As long as there is a rake task I'll be happy as a clam :P

@mssola
Copy link
Collaborator Author

mssola commented Feb 15, 2016

I was thinking about a group_filter value, as some directories have groups in multiple places. A good example would be a multi-domain ldap, where groups would then be in at least 2 different places in the LDAP tree. Some implementations have also a compat tree (I have FreeIPA in mind), so that would make it impossible to have a common base for those multiple domains without having the compat groups included (and compat groups have the same CN as the group, but a different objectClass).

Bear in mind that base is already using the DN, not the CN. Therefore, these clashes shouldn't occur, no ?

As long as there is a rake task I'll be happy as a clam :P

Seems fair to me. Of course, I'd add a warning about the possible consequences :)

Edit: I've added the rake task in the check list ;)

@lonewulf
Copy link
Contributor

Bear in mind that base is already using the DN, not the CN. Therefore, these clashes shouldn't occur, no ?

As long as you can be specific enough, it should be fine, yes.

@holgerreif
Copy link

I'm fine with the idea, that in addition to the users in the LDAP group some team users might be configured locally in portus' database. However I'm not convinced that kind of copying users belonging to a LDAP group into a team and beeing able to remove them locally (not syncing) would be an expected experience.

For me I either want to use LDAP group memberships permanently or not. Providing for team addition for a period of time (until destroy method is used) sounds strange, especially in combination with the ideas of manual "Re-Syncing"...

For me the "natural" approach during authorization would be

  1. check if user is team member or
  2. query LDAP if user belongs to LDAP group
    If either succeeds, then let the user in.
    To implement the query (based on a query filter with variable parts for groupname and username) should straight forward. Is there a problem with querying LDAP online?

As a bonus in the team page a button "Show team members from LDAP" could be implemented, this is a goodie for the portus admin.

@lonewulf
Copy link
Contributor

@holgerreif 👍

Would make the change easier to implement, too. No automation, no rake tasks.

@holgerreif
Copy link

@lonewulf More important, this does not introduce a new concept which needs explaination and could produce unnecessary bug reports.

@mssola
Copy link
Collaborator Author

mssola commented Feb 16, 2016

Hi @holgerreif, I like your idea. It looks simple and straight-forward. I have some problems with it though:

  • What permissions do this LDAP group members have ? If you can configure it, then note that said membership should be stored on the DB, so the role can be stored for each member.
  • Another problem I have is about performance. It means that whenever we have to check for team membership (which is quite often in pull/push actions), we have to talk with the LDAP server. This can be leveraged with some caching mechanism, but I fear it could be prone to errors.

@holgerreif
Copy link

@mssola

What permissions do this LDAP group members have

Good point. What about a per-role LDAP group membership configuration?
Means configuration of roles for LDAP users is completely done on LDAP side.

Another problem I have is about performance.

My POV is: If you rely on a central directory infrastructure which happen to be used by portus too, then you should have solved any performance (and availability) issues before.

While we are at availibilty: Utilizing more than one LDAP server (config ldap.hostname) seems more important than performance (caching).

@mssola
Copy link
Collaborator Author

mssola commented Feb 16, 2016

Good point. What about a per-role LDAP group membership configuration?
Means configuration of roles for LDAP users is completely done on LDAP side.

I don't really like it. The thing here is that this solution was meant to keep things simple, meaning that:

  • I don't want administrators to perform extra, tedious configuration.
  • I don't want administrators to change their LDAP servers just for Portus.

And I don't see any reasonable way to keep this configuration in Portus either.

My POV is: If you rely on a central directory infrastructure which happen to be used by portus too, then you should have solved any performance (and availability) issues before.

Fair enough.

I first envisioned this feature as a small step that helped users. But after discussing this with you guys, I'm realizing that this solution is half-way what users want. Moreover, the idea to allow teams to be "unsync'ed" with their corresponding LDAP group seems a nice idea at first, but I'm realizing that's not what it's desired: we should either synchronize everything, or don't do anything. Therefore, what do you think about going the other way around:

  • Add a job in crono that kept everything in sync. This way, we are not allowing flexible combinations as I was first proposing, but at least we are getting a more expected behavior. That being said, allowing more flexible setups can be left for future iterations. Moreover, we wouldn't be getting performance penalties, because it would be stored in the DB.
  • Add a rake task for forcing a sync just in case crono is failing or something like that.

@lonewulf @holgerreif what do you think ?

While we are at availibilty: Utilizing more than one LDAP server (config ldap.hostname) seems more important than performance (caching).

Fair enough. I just opened issue #739 for this. As I said there, we have already tons of features/improvements/fixes to implement for the next release, so it's unlikely that it gets included for the next release ;)

@holgerreif
Copy link

Add a job in crono that kept everything in sync.

Fine. But remember one of your previous points:

What permissions do this LDAP group members have

We should introduce configurable default permissions that get assigned to a team member that is found in LDAP but not in portus database yet.

Add a rake task for forcing a sync just in case crono is failing or something like that

Not knowing much about rails, I guess this is for the UI button. Then I'm fine.

@lonewulf
Copy link
Contributor

@mssola Seems a good compromise and we keep a 1:1 mapping between LDAP and Portus.

Though I would allow to map multiple groups to a team, would satisfy organizations using multiple groups to manage their access policy, and those who use only one. With what @holgerreif said, a configurable default permission that is used when the user is first added to the team.

@mssola
Copy link
Collaborator Author

mssola commented Feb 16, 2016

We should introduce configurable default permissions that get assigned to a team member that is found in LDAP but not in portus database yet.

Yeah. This would be done when adding the relationship between a team and an LDAP group. In short, what I said in my original post about adding the relationship would still stand :)

Not knowing much about rails, I guess this is for the UI button. Then I'm fine.

Not really, it's a task to be executed from the CLI :) Ideally we could also add the "Sync" button, but the rake task should be there just in case something really bad happens :)

@mssola
Copy link
Collaborator Author

mssola commented Feb 16, 2016

Though I would allow to map multiple groups to a team, would satisfy organizations using multiple groups to manage their access policy, and those who use only one.

That's a good point. Instead of being a 1:1 mapping, it would be a 1:n.

@mssola
Copy link
Collaborator Author

mssola commented Feb 16, 2016

/ping @jordimassaguerpla @flavio @eotchi grab some ☕, read this discussion and tell me what you think :)

@flavio
Copy link
Member

flavio commented Mar 24, 2016

Just to wrap it up:

  • multiple LDAP groups can be mapped to a team
  • all the freshly imported users will have a default role that is selected when the team is created for the 1st time
  • the default role can be changed later on by a Portus admin or by an owner of the team
  • nobody can remove a user from the team, neither a Portus admin nor a team owner
  • members' roles can be changed as usual
  • a crono job will periodically sync the team members with the contents of LDAP:
    • already imported users will be left unchanged (their role won't be affected)
    • missing members will be added to the team using the default role specified on team creation
    • users not found on LDAP are going to be dropped from the team

Did I get it right? If that's the case I'm fine with this new proposal.

A couple of corner cases I just realized:

  • after a sync there could be a team without owners. I think this is acceptable, the problem can be solved by an admin of Portus.
  • after a sync there could be a team without members. Again I think we can live with that.

@mssola
Copy link
Collaborator Author

mssola commented Mar 24, 2016

Did I get it right? If that's the case I'm fine with this new proposal.

That's about right :)

after a sync there could be a team without owners. I think this is acceptable, the problem can be solved by an admin of Portus.

Agreed.

after a sync there could be a team without members. Again I think we can live with that.

Agreed.

@diranged
Copy link

multiple LDAP groups can be mapped to a team

What's the motivation here? This feels more complicated than it needs to be. You'll end up with a lot more code around merging of user lists than you really need IMO.

missing members will be added to the team using the default role specified on team creation

This is just another reason to move to a model where the team itself is granted specific permissions on a namespace. It reduces complexity I think. The whole "default role" vs "specified role" bit is what I'm referring to here.

@holgerreif
Copy link

@flavio Just have time right now to comment on this

multiple LDAP groups can be mapped to a team

AFAICT this was not the original intent. Maybe you mix it up with comment from @lonewulf "a group_filter value, as some directories have groups in multiple places"

One further note: I'm not sure, whether the idea of having both locally registered and ldap user (a litlte bit like Linux' nss) is present or not: If yes, then each user has to carry a sign whether he was imported from LDAP or added locally in order to remove him from portus once he gets removed from LDAP group.

@flavio
Copy link
Member

flavio commented Mar 29, 2016

One further note: I'm not sure, whether the idea of having both locally registered and ldap user (a litlte bit like Linux' nss) is present or not: If yes, then each user has to carry a sign whether he was imported from LDAP or added locally in order to remove him from portus once he gets removed from LDAP group.

AFAIK when you enable LDAP support it's no longer possible to have non-LDAP users registered. @mssola am I right?

@flavio
Copy link
Member

flavio commented Mar 29, 2016

To @diranged

multiple LDAP groups can be mapped to a team
What's the motivation here? This feels more complicated than it needs to be. You'll end up with a lot more code around merging of user lists than you really need IMO.

That was requested above inside of the thread: #734 (comment)

missing members will be added to the team using the default role specified on team creation
This is just another reason to move to a model where the team itself is granted specific permissions on a namespace. It reduces complexity I think. The whole "default role" vs "specified role" bit is what I'm referring to here.

Right now the roles are managed on the team level by Portus. Moving that to the namespace level would require quite some work and would have to be addressed with a dedicated issue.

If we decide for such a refactoring, then we we would of course extend that change to this issue.

@mssola
Copy link
Collaborator Author

mssola commented Mar 29, 2016

AFAIK when you enable LDAP support it's no longer possible to have non-LDAP users registered.

That's correct. We disallow this to avoid extra-complexity (from both an admin point of view, and implementation-wise).

That was requested above inside of the thread: #734 (comment)

That being said, I'm not entirely sure about this. I'd leave this as a separate issue, and we could implement it in a future release. For now, having a 1:1 mapping should be enough for most administrators, and it would make it easier to implement this feature on the first place.

This is just another reason to move to a model where the team itself is granted specific permissions on a namespace. It reduces complexity I think. The whole "default role" vs "specified role" bit is what I'm referring to here.

Agreed, but as discussed in #771, this change would be too disruptive. We can do that in a future release.

@holgerreif
Copy link

@flavio I found time to read your summerizing comment in detail. Yes, this is my understanding as well.

The only open question seems to be the multiple LDAP groups can be mapped to a team. From the implementation POV it is a 1.1 relationship between a result of an ldap query and a team. Of course it is possible that the ldap query returns results from more than one group.

The complexity is not on the portus implementation but on the portus admin who is responsible to define the base and the filter expressions for a team. This suits both simple LDAP setups and complicated ones where an experienced LDAP amin could help with the LDAP config of the teams.
@lonewulf Do you aggree?

As I suggested before while configuring the relationship (base and filter) it would be helpful for the admin to issue a test query returning the possible members for verification.

@holgerreif
Copy link

While writing #801 I saw a further requirement for LDAP group support:

Add a marker that a specific team is not managed by LDAP (not connected to an LDAP group).

This way portus admin would be able to handle some cases where he does not want LDAP admin to create a group for specific, possibly temporary need in portus.

Whether this local marker is stored explicitely as a type attribute of a team or it's just derived from empty base and filter values is up to the implementation. However having an explicit type attribute (with empty defaulting to local group management for backward compatibility) would allow for easier addition of other group management schemes (e.g. AD support).

@mssola
Copy link
Collaborator Author

mssola commented Mar 31, 2016

As I suggested before while configuring the relationship (base and filter) it would be helpful for the admin to issue a test query returning the possible members for verification.

This sounds like a nice feature 👍

Add a marker that a specific team is not managed by LDAP (not connected to an LDAP group).

I'm not sure I understand this :) For now, this feature states that you can have a mapping between a team and an LDAP group. Nothing is prohibiting you from having teams that are not mapped to any LDAP group, but they are filled with users. Moreover:

  • I agree that when you create a team it should not map into an LDAP group directly, and the admin should provide the needed base and filter attributes in order to create the mapping.
  • There should be an explicit UI marker whenever a team has a relationship with an LDAP group (note that the default would be to not create the mapping, so it makes sense to add the marker when such mapping actually exists).

@holgerreif
Copy link

Add a marker that a specific team is not managed by LDAP (not connected to an LDAP group).

I'm not sure I understand this :) For now, this feature states that you can have a mapping between
a team and an LDAP group. Nothing is prohibiting you from having teams that are not mapped to
any LDAP group, but they are filled with users.

I wasn't sure about it, but this is exactly what I wanted ;-)

@lonewulf
Copy link
Contributor

Hello there,

Well, the reason for multiple groups associated with a namespace would be to have something along those lines.

grp_allow_ro -> group containing users that have read-only access to the namespace
grp_allow_rw -> group containing users that have push/pull access
grp_allow_owners -> group containing group owners (don't know if this type of group would be useful though)

As for filters, why not use the full ldap path of the group when adding it (with a feature to search through ldap in the base provided for a more friendly approach)

@mssola
Copy link
Collaborator Author

mssola commented Mar 31, 2016

@lonewulf this use will be covered with what we are discussing in #771 :)

As for filters, why not use the full ldap path of the group when adding it (with a feature to search through ldap in the base provided for a more friendly approach)

👍

@wstrange
Copy link

Just my 2 cents: Synchronization with LDAP is often fraught with problems. I think most enterprise users would prefer to query LDAP directly for group membership, possibly caching the results when the user logs on.

LDAP lookups are very fast on modern LDAP servers.

@mssola mssola removed the junior job label Sep 27, 2017
@mssola mssola added this to the Release 2.4 milestone Sep 27, 2017
@mssola mssola modified the milestones: Release 2.4, Release 2.5 Jan 18, 2018
@mssola mssola removed this from the Release 2.5 milestone Oct 11, 2018
@ArcticSnowman
Copy link

@mssola - like to add my support/encouragement for this feature.. would make my life easier if membership of a team was added/updated based on LDAP group membership..

@mssola mssola self-assigned this Dec 10, 2018
@mssola mssola removed the discussing label Dec 10, 2018
@mssola mssola added the L label Jan 7, 2019
@mssola mssola mentioned this issue Jan 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants