Fixes #36306 - Fixing the UserGroup issue when working with RHDS/POSIX #72

waldirio · 2023-04-16T17:16:14Z

When working with RHDS (RH Directory Server), the UserGroup feature it's not working properly, the group_list method doesn't return the correct group information, even when the plugin memberof it's already available.

With this change, it will check the also the @attr_login attribute and will use it, once there is no value, then we will assume memberuid. Before it was hardcoded to memberuid.

Thank you!
Waldirio

adamruzicka · 2023-04-17T09:08:35Z

lib/ldap_fluff/posix_member_service.rb

      :base => @group_base, :attributes => ["cn"]
    ).each do |entry|
-      groups << entry[:cn][0]
+      entry[:memberof].each do |grp|
+        groups << grp.sub(/.*?cn=(.*?),.*/, '\1')


I don't have a RHDS at hand so I can't really test this and I'm not sure how exactly those strings look so maybe the things I'm about to describe cannot happen at all, but still.

The regexp you used here assumes there is more than one key=value element and I'm not sure if that's always there. Using something like /.*cn=([^,]+).*/ feels like a safer bet.

#sed returns the string as-is if it does not match. I'm not sure if it can happen, but again, something like this would probably be safer

Suggested change

groups << grp.sub(/.*?cn=(.*?),.*/, '\1')

group = grp.match(/cn=([^,]+)/)&.captures&.first

groups << group if group

Isn't something like https://stackoverflow.com/questions/50285542/parse-ldap-dn-string-into-key-value-pairs-in-ruby/57413350#57413350 better?

That would indeed be better

So add require 'net/ldap/dn' (after net/ldap, before it somehow fails for me) and then

Suggested change

groups << grp.sub(/.*?cn=(.*?),.*/, '\1')

dn = Net::LDAP::DN.new(grp)

groups << dn.enum_for(:each_pair).find { |key, _value| key == 'cn' }[1]

Now I do wonder if this works though. In the filter you say you only want the cn attribute, not memberof so I think it won't retrieve the property and entry[:memberof] will be nil. And even if you retrieve it, it may be nil as well for users without memberof set.

So you do need to make it a safe navigator iteration: entry[:memberof]&.each

waldirio · 2023-04-17T18:18:14Z

Hello @adamruzicka and @ekohl
Thank you for your comments and suggestions. Please, let me ask you to review this information on the BZ
https://bugzilla.redhat.com/show_bug.cgi?id=2185681#c6
I did already all the testing covering the possibilities, no member of a group, member of one single group, member of N groups, tested on both sides, rails, and webUI, and in both cases worked as expected.

Please, let me know what you think.
Thank you!
Waldirio

ekohl · 2023-04-17T19:33:29Z

I don't think this refactor is really correct because you will break other use cases.

AFAIK (and it's been a long time since I was an admin for an LDAP server) there are 2 ways to manage groups:

RFC2703's posixGroup
memberOf property (for which I couldn't find a standard)

The current method implements the posixGroup approach, which is (as the name implies) similar to POSIX groups: they're a list of usernames stored in the memberUid field. The way to find the list of groups for a user is to look for every group where the uid is in the memberUid field and collect the names of those groups.

Then there is the memberOf property on users. From your implementation, I'm guessing this is what RHDS implements.

OpenLDAP needs an extension (https://linux.die.net/man/5/slapo-memberof) to support this.

Now looking in the codebase, we already have an implementation for FreeIPA (https://github.com/theforeman/ldap_fluff/blob/master/lib/ldap_fluff/freeipa_member_service.rb) which is really unchanged since 2017. RHDS is the branded version of FreeIPA so I'm guessing it may be something of detecting the flavor.

Edit: Of course I now read through the BZ where you say you selected the POSIX type, but I think you should select the FreeIPA type.

waldirio · 2023-04-17T23:09:05Z

Hello @ekohl

Indeed, I agree. After installing a fresh RHDS, I saw this behavior and didn't see the memberof attribute, after some research, I saw it's a module and it should be enabled in order to work. But I totally agree with you.

Please, allow me some time to review and work with the group information that we have already by default.

# grp_satellite, acme.local
dn: cn=grp_satellite,dc=acme,dc=local
objectClass: top
objectClass: groupOfNames
cn: grp_satellite
member: uid=dcamilo,ou=users,dc=acme,dc=local

# grp2_satellite, acme.local
dn: cn=grp2_satellite,dc=acme,dc=local
objectClass: top
objectClass: groupOfNames
cn: grp2_satellite
description: grp2_satellite
member: uid=dcamilo,ou=users,dc=acme,dc=local
member: cn=waldirio,ou=users,dc=acme,dc=local

I'll do some additional tests creating POSIX and Basic groups, I believe this will be great, and will cover some additional possibilities.

I'll keep you posted.
Waldirio

waldirio · 2023-04-18T02:34:29Z

Hello @adamruzicka and @ekohl

Please, let me share the whole def here, once I'm testing it on Satellite and I did a complete change to filter passing by all the groups, I'll not change it yet on the upstream code.

  def find_user_groups(uid)
    groups = []

    search_filter = Net::LDAP::Filter.eq('objectClass', 'groupOfNames')
    results_attr = ["cn", "member"]

    ldap.search(:filter => search_filter, :attributes => results_attr).each do |grp_info|

      grp_info[:member].each do |login|
        only_uid = login.split(',')[0].split('=')[1]

        if only_uid.include?(uid)
          groups << grp_info[:cn]
        end
      end
    end

    if groups.length > 0
      groups.flatten!
    else
      groups = []
    end

  end

and below, you can see how the group information is on a fresh vanilla RHDS

# grp2_satellite, acme.local
dn: cn=grp2_satellite,dc=acme,dc=local
objectClass: top
objectClass: groupOfNames
cn: grp2_satellite
description: grp2_satellite
member: cn=waldirio,ou=users,dc=acme,dc=local
member: uid=dcamilo,ou=users,dc=acme,dc=local

# grp_pos, acme.local
dn: cn=grp_pos,dc=acme,dc=local
objectClass: top
objectClass: groupOfNames
objectClass: posixGroup
cn: grp_pos
gidNumber: 200000
member: uid=dcamilo,ou=users,dc=acme,dc=local

Above, the first group it's a BASIC group, and the second one is a POSIX group. the code it's checking for groupOfNames, which is present on both of them.

I did all the tests locally and with or without group, the user is able to login and the UserGroup feature it's working as expected.

I also would like to ask you both to suggest the best code implementation, once I'm improving my knowledge of ruby, I can feel that this code it's more like python than anything else ... I'm trying! :-)

Ps.: I'm using all the split parse because the customer can create the user object using cn or uid, as you can see below.

member: cn=waldirio,ou=users,dc=acme,dc=local
member: uid=dcamilo,ou=users,dc=acme,dc=local

Thank you again!
Waldirio

waldirio · 2023-04-24T16:42:39Z

Hello all,

The code was updated. Please, let me know what you think.

Thank you!
Waldirio

ekohl

I still think this breaks plain RFC2307 support. The current LdapFluff::Posix::MemberService class does exactly what it needs to. Technically speaking it should filter on objectClass equals posixGroup AND memberuid equals $uid, but that's redundant.

Reading https://access.redhat.com/documentation/en-us/red_hat_directory_server/11/html/administration_guide/advanced_entry_management they use the objectClass groupOfNames which is completely different. We don't have such support today, so it deserves its own flavor. RHDS is the downstream of 389-ds though purely numeric class names probably don't work in Ruby. Not sure what the best name would be then.

@adamruzicka do you agree it should be its own flavor?

waldirio · 2023-04-26T22:27:25Z

Hello @ekohl and @adamruzicka

Just some news here, when doing the rhds implementation, we can see that the standard objectclass used when creating a group is groupOfNames. However, when checking in the customer today, they are using a different objectclass called groupOfUniqueNames. That said, here are the differences

groupOfNames == member
groupOfUniqueNames == uniqueMember

And I can see the complete description on rfc4519.

That said, for the customer, looking for groupOfUniqueNames worked. What I believe to be interesting here is, once there is two different kind of objectclass for groups, we can test both, and the one that returns, it's the one that we will query. I also was able to add both object classess in the same group, which is totally wrong IMHO, but we can see it live.

# testgrp, Groups, acme.local
dn: cn=testgrp,ou=Groups,dc=acme,dc=local
objectClass: top
objectClass: groupOfNames
objectClass: posixGroup
objectClass: groupofuniquenames
cn: testgrp
description: Test Group
gidNumber: 1234
member: uid=dcamilo,ou=users,dc=acme,dc=local
uniqueMember: uid=dcamilo,ou=users,dc=acme,dc=local

and here we can see one example from the cu env (names removed)

irb(main):006:0> pp conn.find_group('satellite-admins')
[#<Net::LDAP::Entry:0x000055d7d838de88
  @myhash=
   {:dn=>["cn=satellite-admins,ou=groups,o=acme"],
    :"apple-generateduid"=>["3167B8E2-5AB1-49C0-B6CE-4BC4A4EA4ED6"],
    :description=>["Satellite Service Administrators"],
    :objectclass=>["top", "groupOfUniqueNames", "acmeGroup"],
    :uniquemember=>
     ["uid=user1,ou=people,o=acme",
      "uid=user2,ou=people,o=acme",
      "uid=user3,ou=people,o=acme",
      "uid=user4,ou=people,o=acme",
      "uid=user5,ou=people,o=acme"],
    :cn=>["satellite-admins"]}>]

I'll wait for your point of view. I believe covering both, we will catch 2 birds with a single stone.

If you all agree with that, I'll work on the code to reach this goal.

Thank you!
Waldirio

waldirio · 2023-04-26T22:29:25Z

Hello,

Just FYI, the code that worked


def find_user_groups(uid)
    groups = []

    search_filter = Net::LDAP::Filter.eq('objectClass',
'groupOfUniqueNames')
    #search_filter = Net::LDAP::Filter.eq('objectClass', 'groupOfNames')
    results_attr = ["cn", "uniqueMember"]

    ldap.search(:filter => search_filter, :attributes => results_attr).each
do |grp_info|

      grp_info[:uniqueMember].each do |login|
        only_uid = login.split(',')[0].split('=')[1]

        if only_uid.include?(uid)
          groups << grp_info[:cn]
        end
      end
    end

    if groups.length > 0
      groups.flatten!
    else
      groups = []
    end

  end

ares · 2023-05-18T07:03:51Z

@ekohl and @adamruzicka any thoughts on the proposal?

ekohl · 2023-05-22T10:16:55Z

I still think it should be its own flavor and not modify the existing flavors. As for the specific implementation I think it's inefficient because it does client side filtering. For large LDAP instances it will retrieve all groups every single time and then iterate over it which can consume a lot of memory on both the LDAP server and client side. I'd suggest is to offload it to the server, which usually has indexes allowing it to quickly find what you're looking for.

This is untested, but it composes a more complex query that should achieve it.

def find_user_groups(uid)
  group_unique_filter = Net::LDAP::Filter.eq('objectClass', 'groupOfUniqueNames') & Net::LDAP::Filter.eq('uniqueMember', uid)
  group_filter = Net::LDAP::Filter.eq('objectClass', 'groupOfNames') & Net::LDAP::Filter.eq('member', uid)
  filter = group_unique_filter | group_filter

  groups = ldap.search(filter: filter, attributes: ['cn']).map { |group| group[:cn] } 
  groups.flatten!
  groups
end

I'm not 100% sure on the flatten bit. Perhaps group[:cn] can be changed to group[:cn][0] and this would be sufficient:

def find_user_groups(uid)
  group_unique_filter = Net::LDAP::Filter.eq('objectClass', 'groupOfUniqueNames') & Net::LDAP::Filter.eq('uniqueMember', uid)
  group_filter = Net::LDAP::Filter.eq('objectClass', 'groupOfNames') & Net::LDAP::Filter.eq('member', uid)
  filter = group_unique_filter | group_filter

  ldap.search(filter: filter, attributes: ['cn']).map { |group| group[:cn][0] } 
end

adamruzicka · 2023-11-28T13:17:43Z

@waldirio Hi, what's your take on what @ekohl suggested?

waldirio · 2023-12-14T16:33:27Z

Hello @adamruzicka good morning

This whole environment was implemented long ago, I'm pretty sure that the implemented code worked, but I'm not sure about the suggested one.

I would recommend setting up an environment if possible and proceeding with the tests. At this moment, I can't do it. Maybe in the future, but if someone else has already the env and/or the ability to try it out quickly, I believe this would be the must.

Thank you!
Waldirio

Fixing the UserGroup issue when working with RHDS/POSIX

19a9e4e

adamruzicka reviewed Apr 17, 2023

View reviewed changes

checking all the ldap groups instead of memberof plugin

0c277bd

ekohl requested changes Apr 24, 2023

View reviewed changes

ekohl mentioned this pull request Apr 9, 2024

Also find groups added through groupOfUniqueNames #78

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36306 - Fixing the UserGroup issue when working with RHDS/POSIX #72

Fixes #36306 - Fixing the UserGroup issue when working with RHDS/POSIX #72

waldirio commented Apr 16, 2023

adamruzicka Apr 17, 2023 •

edited

Loading

adamruzicka Apr 17, 2023

adamruzicka Apr 17, 2023

ekohl Apr 17, 2023

adamruzicka Apr 17, 2023

ekohl Apr 17, 2023

waldirio commented Apr 17, 2023

ekohl commented Apr 17, 2023 •

edited

Loading

waldirio commented Apr 17, 2023

waldirio commented Apr 18, 2023

waldirio commented Apr 24, 2023

ekohl left a comment

waldirio commented Apr 26, 2023

waldirio commented Apr 26, 2023

ares commented May 18, 2023

ekohl commented May 22, 2023

adamruzicka commented Nov 28, 2023

waldirio commented Dec 14, 2023

	groups << grp.sub(/.?cn=(.?),.*/, '\1')
	group = grp.match(/cn=([^,]+)/)&.captures&.first
	groups << group if group

	groups << grp.sub(/.?cn=(.?),.*/, '\1')
	dn = Net::LDAP::DN.new(grp)
	groups << dn.enum_for(:each_pair).find { \|key, _value\| key == 'cn' }[1]

Fixes #36306 - Fixing the UserGroup issue when working with RHDS/POSIX #72

Are you sure you want to change the base?

Fixes #36306 - Fixing the UserGroup issue when working with RHDS/POSIX #72

Conversation

waldirio commented Apr 16, 2023

adamruzicka Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

adamruzicka Apr 17, 2023

Choose a reason for hiding this comment

adamruzicka Apr 17, 2023

Choose a reason for hiding this comment

ekohl Apr 17, 2023

Choose a reason for hiding this comment

adamruzicka Apr 17, 2023

Choose a reason for hiding this comment

ekohl Apr 17, 2023

Choose a reason for hiding this comment

waldirio commented Apr 17, 2023

ekohl commented Apr 17, 2023 • edited Loading

waldirio commented Apr 17, 2023

waldirio commented Apr 18, 2023

waldirio commented Apr 24, 2023

ekohl left a comment

Choose a reason for hiding this comment

waldirio commented Apr 26, 2023

waldirio commented Apr 26, 2023

ares commented May 18, 2023

ekohl commented May 22, 2023

adamruzicka commented Nov 28, 2023

waldirio commented Dec 14, 2023

adamruzicka Apr 17, 2023 •

edited

Loading

ekohl commented Apr 17, 2023 •

edited

Loading