Merge branch 'group_sync_dn_error_checking' into 'master'

Gracefully handle malformed DNs in LDAP group sync

Customer reported an emergency because group sync was blowing up with 'Badly formed DN'. It turns out their LDAP system inserts a blank member entry at the beginning of each LDAP group. When we try to parse this DN, group sync explodes. Obviously we should handle this error gracefully. This merge request catches the error, logs it, and moves on (skipping that entry).

See merge request !392

CHANGELOG-EE

@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.9.0 (unreleased)
 
 v 8.8.3
+  - Gracefully handle malformed DNs in LDAP group sync
   - Make it clear the license overusage is visible only to admins
   - Reduce load on DB for license upgrade check
 

lib/gitlab/ldap/group_sync.rb

@@ -180,7 +180,14 @@ module Gitlab
       # account for that. See gitlab-ee#442
       def ensure_full_dns!(dns)
         dns.map! do |dn|
-          parsed_dn = Net::LDAP::DN.new(dn).to_a
+          begin
+            parsed_dn = Net::LDAP::DN.new(dn).to_a
+          rescue RuntimeError => e
+            # Net::LDAP raises a generic RuntimeError. Bad library! Bad!
+            logger.error { "Found malformed DN: '#{dn}'. Skipping. #{e.message}" }
+            next
+          end
+
           # If there is more than one key/value set we must have a full DN,
           # or at least the probability is higher.
           if parsed_dn.count > 2
@@ -192,6 +199,9 @@ module Gitlab
             dn
           end
         end
+
+        # Remove `nil` values generated by the rescue above.
+        dns.compact!
       end
 
       def member_uid_to_dn(uid)

spec/lib/gitlab/ldap/group_sync_spec.rb

@@ -548,6 +548,40 @@ describe Gitlab::LDAP::GroupSync, lib: true do
             }.from(nil).to(user1.username)
         end
       end
+
+      context 'with invalid DNs in the LDAP group' do
+        let(:ldap_group) do
+          Gitlab::LDAP::Group.new(
+            Net::LDAP::Entry.from_single_ldif_string(
+              <<-EOS.strip_heredoc
+                dn: cn=ldap_group1,ou=groups,dc=example,dc=com
+                cn: ldap_group1
+                member:
+                member: uid=#{user1.username},ou=users,dc=example,dc=com
+                member: foo, bar
+                description: LDAP Group 1
+                objectclass: top
+                objectclass: groupOfUniqueNames
+              EOS
+            )
+          )
+        end
+
+        # Check that the blank member and malformed member logged an error
+        it 'expects two errors to be logged' do
+          expect(Rails.logger).to receive(:error) do |&block|
+            expect(block.call).to match /^Found malformed DN:/
+          end.twice
+
+          group_sync.sync_groups
+        end
+
+        it 'expects the valid user to be added' do
+          expect { group_sync.sync_groups }
+            .to change { group1.members.where(user_id: user1.id).any? }
+              .from(false).to(true)
+        end
+      end
     end
   end