Merge branch 'catch_ldap_group_dn_error' into 'master'

Catch Net::LDAP::DN exceptions in EE::Gitlab::LDAP::Group

Closes #1755

See merge request !1260

changelogs/unreleased-ee/catch_ldap_group_dn_error.yml

+---
+title: Catch Net::LDAP::DN exceptions in EE::Gitlab::LDAP::Group
+merge_request: 1260
+author:

lib/ee/gitlab/ldap/group.rb

@@ -90,13 +90,13 @@ module EE
           members.concat(ranged_members(entry)) if has_member_range?(entry)
           # Process nested group members
           members.concat(nested_members(nested_groups_to_skip))
-
           # Clean dns of groups and users outside the base
           members.reject! { |dn| nested_groups_to_skip.include?(dn) }
-          base = Net::LDAP::DN.new(adapter.config.base.downcase).to_a
-          members.select! { |dn| Net::LDAP::DN.new(dn.downcase).to_a.last(base.length) == base }
 
-          members
+          return [] if members.empty?
+
+          # Only return members within our given base
+          members_within_base(members)
         end
 
         # AD requires use of range retrieval for groups with more than 1500 members
@@ -146,6 +146,37 @@ module EE
 
           match[1].to_i + 1 if match.present? && match[1] != '*'
         end
+
+        # The old AD recursive member filter would exclude any members that
+        # were outside the given search base. To maintain that behavior,
+        # we need to do the same.
+        #
+        # Split the base and each member DN into pairs. Compare the last
+        # base N pairs of the member DN. If they match, the user is within
+        # the base DN.
+        #
+        # Ex.
+        # - Member DN: 'uid=user,ou=users,dc=example,dc=com'
+        # - Base DN:   'dc=example,dc=com'
+        #
+        # Base has 2 pairs ([dc,example], [dc,com]). If the last 2 pairs of
+        # the user DN match, profit!
+        def members_within_base(members)
+          begin
+            base = Net::LDAP::DN.new(adapter.config.base.downcase).to_a
+          rescue RuntimeError
+            Rails.logger.error "Configured LDAP `base` is invalid: '#{adapter.config.base}'"
+            return []
+          end
+
+          members.select do |dn|
+            begin
+              Net::LDAP::DN.new(dn.downcase).to_a.last(base.length) == base
+            rescue RuntimeError
+              Rails.logger.warn "Received invalid member DN from LDAP group '#{cn}': '#{dn}'. Skipping"
+            end
+          end
+        end
       end
     end
   end

spec/lib/ee/gitlab/ldap/group_spec.rb

@@ -122,6 +122,45 @@ describe EE::Gitlab::LDAP::Group, lib: true do
         expect(group.member_dns).not_to include('uid=foo,ou=users,dc=other,dc=com')
         expect(group.member_dns).to include('uid=bar,ou=users,dc=example , dc=com')
       end
+
+      it 'logs an error when the LDAP base is invalid' do
+        stub_ldap_config(
+          active_directory: true,
+          base: 'invalid,dc=example,dc=com'
+        )
+        nested_groups = [group2_entry]
+        stub_ldap_adapter_nested_groups(group.dn, nested_groups, adapter)
+        stub_ldap_adapter_nested_groups(group2_entry.dn, [], adapter)
+
+        expect(Rails.logger)
+          .to receive(:error).with("Configured LDAP `base` is invalid: 'invalid,dc=example,dc=com'")
+        # Users in the top-level group always get added - they're not filtered
+        # through the nested groups shenanigans.
+        expect(group.member_dns).to match_array(
+          %w(
+            uid=user1,ou=users,dc=example,dc=com
+            uid=user2,ou=users,dc=example,dc=com
+          )
+        )
+      end
+
+      it 'logs a warning when an invalid member DN is found in an LDAP group' do
+        group3_entry = ldap_group_entry(
+          ['invalid,ou=user,ou=groups,dc=example,dc=com'],
+          cn: 'ldap_group3',
+          objectclass: 'group',
+          member_attr: 'member',
+          member_of: group1_entry.dn
+        )
+        nested_groups = [group2_entry, group3_entry]
+        stub_ldap_adapter_nested_groups(group.dn, nested_groups, adapter)
+        stub_ldap_adapter_nested_groups(group2_entry.dn, [], adapter)
+        stub_ldap_adapter_nested_groups(group3_entry.dn, [], adapter)
+
+        expect(Rails.logger)
+          .to receive(:warn).with(/Received invalid member/)
+        expect(group.member_dns).not_to include('invalid,ou=user,ou=groups,dc=example,dc=com')
+      end
     end
   end
 end