From b95f3bfb97cc360586e2a07ea766e7a7e6a447f6 Mon Sep 17 00:00:00 2001 From: Alex Lossent Date: Wed, 18 May 2016 15:03:59 +0200 Subject: [PATCH 1/2] Aternative implementation of recursive group membership resolution in AD This avoids the use of the recursive memberOf LDAP search, which becomes very inefficient when the number of user accounts increases. A limitation is that nested groups will only be evaluated if in the LDAP path configured as group_base path. Other groups will be ignored and their members skipped. --- lib/gitlab/ldap/group.rb | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/ldap/group.rb b/lib/gitlab/ldap/group.rb index c5b28782bfa1a9..89053f2d1fa242 100644 --- a/lib/gitlab/ldap/group.rb +++ b/lib/gitlab/ldap/group.rb @@ -38,14 +38,37 @@ def member_uids entry.memberuid end - def member_dns + def dn + entry.dn + end + + def nested_groups + # returns groups within group_base that are directly nested into this group, + # for recursive group membership resolution. + # ATTENTION!!! Nested groups in other OUs will be ignored!!! This works for CERN e-groups + # because all e-groups are in the same OU. + return [] unless adapter && adapter.config.group_base + adapter.ldap_search( + base: adapter.config.group_base, + filter: Net::LDAP::Filter.eq("objectClass", "group") & Net::LDAP::Filter.eq("memberOf", dn), + ).map{|result| Group.new(result, adapter) } + end + + def member_dns(nested_groups_to_skip=[]) dns = [] # There's an edge-case with AD where sometimes a recursive search # doesn't return all users at the top-level. Concat recursive results # with regular results to be safe. See gitlab-ee#484 if active_directory? - dns = adapter.dns_for_filter(active_directory_recursive_memberof_filter) + # ignore that group if we see it again in a nested group + nested_groups_to_skip.push(entry.dn) + # process nested groups + nested_groups.each do |nested_group| + # don't process a same nested group several times. (even though we've already done the most expansive operation of loading that group...) + next if nested_groups_to_skip.include?(nested_group.dn) + dns.concat(nested_group.member_dns(nested_groups_to_skip)) + end end if (entry.respond_to? :member) && (entry.respond_to? :submember) @@ -59,7 +82,11 @@ def member_dns else Rails.logger.warn("Could not find member DNs for LDAP group #{entry.inspect}") end - dns.uniq + # remove the group DNs from the result and filter results to include only members in the configured base DNs + # to more closely match the upstream behavior using active_directory_recursive_memberof_filter (it searches + # members in the base LDAP path only). + base_suffix = ",#{adapter.config.base.downcase}" + (dns - nested_groups_to_skip).select{ |dn| dn.downcase.end_with?(base_suffix)}.uniq end private -- GitLab From 628482ca06dd54e796390f01e6ae979d6604a097 Mon Sep 17 00:00:00 2001 From: Alex Lossent Date: Wed, 18 May 2016 16:56:34 +0200 Subject: [PATCH 2/2] Now correctly supports grousp larger than 1500 members --- lib/gitlab/ldap/group.rb | 43 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ldap/group.rb b/lib/gitlab/ldap/group.rb index 89053f2d1fa242..20c2f1ad8abe55 100644 --- a/lib/gitlab/ldap/group.rb +++ b/lib/gitlab/ldap/group.rb @@ -49,11 +49,45 @@ def nested_groups # because all e-groups are in the same OU. return [] unless adapter && adapter.config.group_base adapter.ldap_search( - base: adapter.config.group_base, + base: adapter.config.group_base, # we might use the directory base to match the behavior of memberof recursive search filter: Net::LDAP::Filter.eq("objectClass", "group") & Net::LDAP::Filter.eq("memberOf", dn), ).map{|result| Group.new(result, adapter) } end + def has_member_range? + member_range_attribute.present? + end + + def member_range_attribute(entry=@entry) + entry.attribute_names.find{ |a| a.to_s.start_with?("member;range=")}.to_s + end + + def has_more_member_ranges?(entry=@entry) + next_member_range_start(entry).present? + end + + # the start of the next member range. nil if we're already at the last member range. + def next_member_range_start(entry=@entry) + match = member_range_attribute(entry).match /^member;range=\d+-(\d+|\*)$/ + return nil unless match.present? + return nil if match[1]=="*" # indicates end of records + match[1].to_i + 1 + end + + def remaining_ranged_members(entry=@entry) + return [] unless has_more_member_ranges?(entry) + next_entry = adapter.ldap_search( + base: dn, + scope: Net::LDAP::SearchScope_BaseObject, + attributes: ["member;range=#{next_member_range_start(entry)}-*"], + ).first + members = [] + members.concat(next_entry[member_range_attribute(next_entry)]) + members.concat(remaining_ranged_members(next_entry)) + members + end + + def member_dns(nested_groups_to_skip=[]) dns = [] @@ -71,7 +105,12 @@ def member_dns(nested_groups_to_skip=[]) end end - if (entry.respond_to? :member) && (entry.respond_to? :submember) + if has_member_range? + # AD requires use of range retrieval for groups with more than 1500 members + # cf. https://msdn.microsoft.com/en-us/library/aa367017(v=vs.85).aspx + dns.concat(entry[member_range_attribute]) + dns.concat(remaining_ranged_members) + elsif (entry.respond_to? :member) && (entry.respond_to? :submember) dns.concat(entry.member + entry.submember) elsif entry.respond_to? :member dns.concat(entry.member) -- GitLab