Add LDAP parent groups search (For permissions to groups and its subgroups) #1058

paroque28 · 2017-09-08T20:38:52Z

This allows group search to add parent groups, used when you want to add permissions to a group and its subgroups.
Now, with the flag "inheritance", the group search adds all the upper groups of certain user. Allowing to have permissions for groups and sub groups.

This first abstracts all the query of groups and use it to reuse code. The method is called queryGroups() and it receives only the DN/uid of an object and returns its groups.
Then I created an iterative loop to look for parent groups of the current group's list. The algorithm skips if the group is already added, preventing infinite loops.
The search loop ends when no more parents are found.

I attached an image with inheritance enabled and one with out

paroque28 · 2017-09-08T20:55:55Z

@ericchiang can you check this?

ericchiang · 2017-09-08T21:00:13Z

Hi @paroque28 thanks for the feature request. We normally require some conversation about what problem you're trying to solve before submitting a PR and doing a code review.

Can you please post an example LDAP schema and query that this new feature enables? The concept of "parent group" is extremely general and can be implemented by LDAP in many different ways. Why can't this be done with the existing code? What LDAP providers implement this?

Also, all large changes to the LDAP code in dex require an integration tests (but after we've agreed to accept the feature) https://github.com/coreos/dex/blob/master/connector/ldap/ldap_test.go#L50

fix ldap group test merge branch

Ldap group unit testing with travis

paroque28 · 2017-09-14T05:04:13Z

Hi @ericchiang I added Integration testing to this feature:
https://github.com/paroque28/dex/blob/master/connector/ldap/ldap_test.go#L213
It passed :):
"--- PASS: TestGroupInheritanceQuery (0.85s)"

The LDAP schema is described in the test but I made an image for convenience

The need for this feature is seen when you want to know if "Jane" (for example) is a member of IT. With actual implementation, you can't know. Therefore you can't, for example, allow all IT members to access certain namespace on Tectonic.

Regards
Pablo

paroque28 · 2017-09-14T05:08:21Z

With that LDAP schema and inheritance flag enabled, Jane would return:

groups: ["team-one", "team-two", "IT", "developers"]

instead of current response:

 groups=["team-one", "team-two"]

As described here https://github.com/paroque28/dex/blob/master/connector/ldap/ldap_test.go#L298

Noticed in dexidp#1058 that our gofmt make target isn't actually erroring if someone commits misformatted code.

paroque28 · 2017-09-14T22:38:45Z

@ericchiang Fixed

ericchiang · 2017-09-14T22:40:57Z

connector/ldap/ldap.go

-			// TODO(ericchiang): Is this going to spam the logs?
-			c.logger.Errorf("ldap: groups search with filter %q returned no groups", filter)
+		// If there's no remaining levels -> exit loop
+		if len(nextLevelGroups) == 0 {


What prevents this from looping infinitely?

The infinite loop is prevented here: https://github.com/paroque28/dex/blob/master/connector/ldap/ldap.go#L542
I'm going to add integration testing for this

ericchiang · 2017-09-14T22:41:50Z

Documentation/ldap-connector.md

@@ -125,6 +125,9 @@ connectors:

      # Represents group name.
      nameAttr: name
+
+      #Add also parents of groups
+      inheritance: true


This doesn't actually define how groups are determined to be a child of another group.

ericchiang · 2017-09-14T22:43:05Z

connector/ldap/ldap.go

+//  Abrstraction query of Groups and reuse code
+func (c *ldapConnector) queryGroups(ctx context.Context, dn string) ([]*ldap.Entry, string, error) {
+	var groups []*ldap.Entry
+	filter := fmt.Sprintf("(%s=%s)", c.GroupSearch.GroupAttr, ldap.EscapeFilter(dn))


I don't think we can assume that the attribute used for determining membership of a user will be the same for determining the membership of the group.

Usually, on ActiveDirectory GroupAttr is "member".
I find it wrong to have a LDAP setup where for example I have the attribute "member" for users and some other word for groups. It can happen of course but I find more confusing for most users to have two different attributes being the same most of the cases.

For example:
It can happen that someone instead of assigning

groupAttr:"member" recursionAttr:"member"

assigns

groupAttr:"member" recursionAttr:"memberOf"

by thinking the attributes make different things. But is up to you to make the decision.
What do you prefer?

ericchiang · 2017-09-14T22:43:34Z

connector/ldap/ldap.go

@@ -50,6 +50,11 @@ import (
 //         groupAttr: member
 //         nameAttr: name
 //
+//         #Used when the filter member:1.2.840.113556.1.4.1941: timesout


About how many groups does it take for this query to timeout?

I don't really know but I tried this and it timed out after one minute. I find it unacceptable to have queries that last longer than 20 seconds (Since it's just a login).
With this implementation, the duration of login reduced in just a couple of seconds

ericchiang · 2017-09-14T22:44:01Z

connector/ldap/ldap.go

@@ -50,6 +50,11 @@ import (
 //         groupAttr: member
 //         nameAttr: name
 //
+//         #Used when the filter member:1.2.840.113556.1.4.1941: timesout
+//         #Better implementation of recursive search


Maybe we can refer to this as "recursive" instead of "inheritance"

paroque28 · 2017-09-19T22:44:39Z

@ericchiang I added an infinite loop test using this LDAP schema

paroque28 · 2017-09-22T21:26:08Z

Any update @ericchiang ?

ericchiang · 2017-09-22T21:35:17Z

@paroque28 I'm still not convinced that the current proposal expresses how groups can be members of other groups in a general enough way. This works well for how AD does it, but what about schema where the CN is used as the attribute value instead of the DN? Can the attribute name be different for user membership instead of group membership? I don't think the current proposed change coves these scenarios.

paroque28 · 2017-09-24T02:30:29Z

Hi @ericchiang , I added an attribute called "recursionGroupAttr" (I didn't know how to name it). I think this solves what you said before.

paroque28 · 2017-09-24T02:31:34Z

connector/ldap/ldap.go

+				if c.GroupSearch.RecursionGroupAttr == "" {
+					return nil, fmt.Errorf("ldap: recursionGroupAttr attribute is not set but recursive search is enabled")
+				}
+				if obtainedGroups, _, err := c.queryGroups(ctx, c.GroupSearch.RecursionGroupAttr, group.DN); err == nil {


This is where the new attribute is used

paroque28 · 2017-09-29T18:00:17Z

Hi @ericchiang I was evaluating Docker Enterprise today and this is the feature I am asking for (read the description of iteration/ recursive search)

srenatus · 2017-11-13T15:38:43Z

This is an interesting feature. Is this still being pursued? (I hope so 🤞)
😄

srenatus · 2017-11-30T07:36:54Z

@paroque28 Two notes/questions that might be useful to you, as this seems to be Active Directory related:

Have you considered using the microsoft connector that has recently been merged? I've verified that it will return nested groups just fine.
If you depend on using LDAP to talk to AD, can you try setting groupAttr to member:1.2.840.113556.1.4.1941:? According to this document, this should return

All groups specified user belongs to,
including due to group nesting

paroque28 · 2017-12-19T19:24:10Z

Hi, @srenatus thank you for your comment. The Microsoft connector sounds excellent, I'll put an eye on that.
And regarding the groupAttr option, unfortunately, I cannot use that since it is a highly inefficient algorithm and my queries times out.

Thank you again

paroque28 · 2018-01-08T17:03:28Z

@srenatus I cannot use Microsoft connector since my LDAP server is in a secured network

thomasem · 2018-01-22T22:03:27Z

This came up for my team recently as we're using Dex to integrate with our FreeIPA backend where memberOf is an attribute on the user with parent groups already resolved. I was wondering what the going thoughts were on taking that approach instead of querying necessarily from the group perspective?

Noticed in dexidp#1058 that our gofmt make target isn't actually erroring if someone commits misformatted code.

vavdoshka · 2021-06-21T16:30:27Z

hello guys, is this feature somehow supported or just abandoned?

vavdoshka · 2021-06-21T17:29:05Z

in case if someone stumbled upon here to find the nested group lookup solution:

thanks, @srenatus for the hint!

groupSearch:
  baseDN: OU=ACME,DC=noway,DC=com
  filter: "(objectClass=group)"
  userAttr: DN
  groupAttr: "member:1.2.840.113556.1.4.1941:"
  nameAttr: cn

lance0805 · 2021-08-02T03:35:52Z

in case if someone stumbled upon here to find the nested group lookup solution:

thanks, @srenatus for the hint!
groupSearch:
  baseDN: OU=ACME,DC=noway,DC=com
  filter: "(objectClass=group)"
  userAttr: DN
  groupAttr: "member:1.2.840.113556.1.4.1941:"
  nameAttr: cn

Saved my life

paroque28 · 2021-09-02T14:19:39Z

Abandoned

anuviswa · 2024-11-15T21:13:29Z

"member:1.2.840.113556.1.4.1941:" this seems an AD specific solution. Can we reconsider this PR? Is there a reason this PR has been abandoned?

jsquyres · 2024-11-21T16:00:27Z

We have interest in reviving this PR, particularly to handle the case @anuviswa mentioned above (using Dex with non-Microsoft LDAP servers).

@nabokihms @sagikazarmark It looks like you both have recent commits in Dex, and you are both listed in MAINTAINERS. If we revive this PR, is there interest in this functionality from the Dex community? We'd be happy to shepherd this PR through the process of getting it into shape that the community would find acceptable.

Add LDAP parent groups search, Active Directory Hierarchy

9476b5e

paroque28 force-pushed the master branch from 35432f5 to 9476b5e Compare September 13, 2017 22:22

paroque28 and others added 6 commits September 13, 2017 17:15

Test Group inheritance

7ec6e22

ldap test

558391c

Fixed duplicated detector for ldap

b29e610

fix ldap group test

56501c4

Merge pull request #1 from paroque28/integration_testing

b8cd81d

fix ldap group test merge branch

Merge pull request #2 from paroque28/ldap

4d67477

Ldap group unit testing with travis

paroque28 force-pushed the master branch from 2efe4f2 to 4d67477 Compare September 14, 2017 05:47

ericchiang pushed a commit to ericchiang/dex that referenced this pull request Sep 14, 2017

Makefile: error out if go files aren't correctly formatted

980400d

Noticed in dexidp#1058 that our gofmt make target isn't actually erroring if someone commits misformatted code.

ericchiang mentioned this pull request Sep 14, 2017

Makefile: error out if go files aren't correctly formatted #1064

Merged

paroque28 added 2 commits September 14, 2017 16:10

Merge branch 'master' of https://github.com/coreos/dex

a0a82ae

Correctly format Go Code

9ddef0d

ericchiang reviewed Sep 14, 2017

View reviewed changes

paroque28 added 2 commits September 19, 2017 15:10

Rename Inheritance -> Recursive

7706433

Integration tests for infinite loops

5854e65

paroque28 force-pushed the master branch from b783705 to 7706433 Compare September 19, 2017 22:18

Formatting

d028a07

LDAP: Attribute for recursion added

a56a0b0

paroque28 commented Sep 24, 2017

View reviewed changes

rithujohn191 self-requested a review November 16, 2017 00:08

pradeephkcisco added a commit to CiscoM31/dex that referenced this pull request Aug 16, 2018

merge changes from dexidp#1058

733d74f

mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019

Makefile: error out if go files aren't correctly formatted

455d33e

Noticed in dexidp#1058 that our gofmt make target isn't actually erroring if someone commits misformatted code.

paroque28 closed this Sep 2, 2021

jsquyres mentioned this pull request Nov 21, 2024

Support LDAP login based on sAMAccountName instead of the email #3849

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LDAP parent groups search (For permissions to groups and its subgroups) #1058

Add LDAP parent groups search (For permissions to groups and its subgroups) #1058

paroque28 commented Sep 8, 2017

paroque28 commented Sep 8, 2017

ericchiang commented Sep 8, 2017

paroque28 commented Sep 14, 2017 •

edited

Loading

paroque28 commented Sep 14, 2017 •

edited

Loading

paroque28 commented Sep 14, 2017

ericchiang Sep 14, 2017

paroque28 Sep 19, 2017

ericchiang Sep 14, 2017

paroque28 Sep 19, 2017

ericchiang Sep 14, 2017

paroque28 Sep 19, 2017

ericchiang Sep 14, 2017

paroque28 Sep 19, 2017

ericchiang Sep 14, 2017

paroque28 Sep 19, 2017

paroque28 commented Sep 19, 2017 •

edited

Loading

paroque28 commented Sep 22, 2017

ericchiang commented Sep 22, 2017

paroque28 commented Sep 24, 2017

paroque28 Sep 24, 2017

paroque28 commented Sep 29, 2017

srenatus commented Nov 13, 2017

srenatus commented Nov 30, 2017

paroque28 commented Dec 19, 2017

paroque28 commented Jan 8, 2018

thomasem commented Jan 22, 2018 •

edited

Loading

vavdoshka commented Jun 21, 2021

vavdoshka commented Jun 21, 2021 •

edited

Loading

lance0805 commented Aug 2, 2021

paroque28 commented Sep 2, 2021

anuviswa commented Nov 15, 2024

jsquyres commented Nov 21, 2024

Add LDAP parent groups search (For permissions to groups and its subgroups) #1058

Add LDAP parent groups search (For permissions to groups and its subgroups) #1058

Conversation

paroque28 commented Sep 8, 2017

paroque28 commented Sep 8, 2017

ericchiang commented Sep 8, 2017

paroque28 commented Sep 14, 2017 • edited Loading

paroque28 commented Sep 14, 2017 • edited Loading

paroque28 commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paroque28 commented Sep 19, 2017 • edited Loading

paroque28 commented Sep 22, 2017

ericchiang commented Sep 22, 2017

paroque28 commented Sep 24, 2017

Choose a reason for hiding this comment

paroque28 commented Sep 29, 2017

srenatus commented Nov 13, 2017

srenatus commented Nov 30, 2017

paroque28 commented Dec 19, 2017

paroque28 commented Jan 8, 2018

thomasem commented Jan 22, 2018 • edited Loading

vavdoshka commented Jun 21, 2021

vavdoshka commented Jun 21, 2021 • edited Loading

lance0805 commented Aug 2, 2021

paroque28 commented Sep 2, 2021

anuviswa commented Nov 15, 2024

jsquyres commented Nov 21, 2024

paroque28 commented Sep 14, 2017 •

edited

Loading

paroque28 commented Sep 14, 2017 •

edited

Loading

paroque28 commented Sep 19, 2017 •

edited

Loading

thomasem commented Jan 22, 2018 •

edited

Loading

vavdoshka commented Jun 21, 2021 •

edited

Loading