Trim extraneous spaces from DNs

lib/gitlab/ldap/auth_hash.rb

@@ -3,6 +3,10 @@
 module Gitlab
   module LDAP
     class AuthHash < Gitlab::OAuth::AuthHash
+      def uid
+        Gitlab::LDAP::Person.normalize_dn(super)
+      end
+
       private
 
       def get_info(key)

lib/gitlab/ldap/person.rb

@@ -36,6 +36,12 @@ module Gitlab
         ]
       end
 
+      def self.normalize_dn(dn)
+        dn.split(/([,+=])/).map do |part|
+          normalize_dn_part(part)
+        end.join('')
+      end
+
       def initialize(entry, provider)
         Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
         @entry = entry
@@ -58,10 +64,43 @@ module Gitlab
         attribute_value(:email)
       end
 
-      delegate :dn, to: :entry
+      def dn
+        self.class.normalize_dn(entry.dn)
+      end
 
       private
 
+      def self.normalize_dn_part(part)
+        cleaned = part.strip
+
+        if cleaned.ends_with?('\\')
+          # If it ends with an escape character that is not followed by a
+          # character to be escaped, then this part may be malformed. But let's
+          # not worry too much about it, and just return it unmodified.
+          #
+          # Why? Because the reason we clean DNs is to make our simplistic
+          # string comparisons work better, even though there are all kinds of
+          # ways that equivalent DNs can vary as strings. If we run into a
+          # strange DN, we should just try to work with it.
+          #
+          # See https://www.ldap.com/ldap-dns-and-rdns for more.
+          return part unless part.ends_with?(' ')
+
+          # Ends with an escaped space (which is valid).
+          cleaned = cleaned + ' '
+        end
+
+        # Get rid of blanks. This can happen if a split character is followed by
+        # whitespace and then another split character.
+        #
+        # E.g. this DN: 'uid=john+telephoneNumber= +1 555-555-5555'
+        #
+        # Should be returned as: 'uid=john+telephoneNumber=+1 555-555-5555'
+        cleaned = '' if cleaned.blank?
+
+        cleaned
+      end
+
       def entry
         @entry
       end

spec/lib/gitlab/ldap/auth_hash_spec.rb

@@ -4,7 +4,7 @@ describe Gitlab::LDAP::AuthHash do
   let(:auth_hash) do
     described_class.new(
       OmniAuth::AuthHash.new(
-        uid: '123456',
+        uid: given_uid,
         provider: 'ldapmain',
         info: info,
         extra: {
@@ -32,6 +32,8 @@ describe Gitlab::LDAP::AuthHash do
   end
 
   context "without overridden attributes" do
+    let(:given_uid) { 'uid=John Smith,ou=People,dc=example,dc=com' }
+
     it "has the correct username" do
       expect(auth_hash.username).to eq("123456")
     end
@@ -42,6 +44,8 @@ describe Gitlab::LDAP::AuthHash do
   end
 
   context "with overridden attributes" do
+    let(:given_uid) { 'uid=John Smith,ou=People,dc=example,dc=com' }
+
     let(:attributes) do
       {
         'username'  => %w(mail email),
@@ -61,4 +65,14 @@ describe Gitlab::LDAP::AuthHash do
       expect(auth_hash.name).to eq("John Smith")
     end
   end
+
+  describe '#uid' do
+    context 'when there is extraneous (but valid) whitespace' do
+      let(:given_uid) { 'uid     =John Smith ,  ou = People, dc=  example,dc =com' }
+
+      it 'removes the extraneous whitespace' do
+        expect(auth_hash.uid).to eq('uid=John Smith,ou=People,dc=example,dc=com')
+      end
+    end
+  end
 end

spec/lib/gitlab/ldap/person_spec.rb

@@ -16,6 +16,136 @@ describe Gitlab::LDAP::Person do
     )
   end
 
+  describe '.normalize_dn' do
+    context 'when there is extraneous (but valid) whitespace' do
+      it 'removes the extraneous whitespace' do
+        given    = 'uid     =John Smith ,  ou = People, dc=  example,dc =com'
+        expected = 'uid=John Smith,ou=People,dc=example,dc=com'
+        expect(described_class.normalize_dn(given)).to eq(expected)
+      end
+
+      context 'for a DN with a single RDN' do
+        it 'removes the extraneous whitespace' do
+          given    = 'uid  =  John Smith'
+          expected = 'uid=John Smith'
+          expect(described_class.normalize_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'when there are escaped characters' do
+        it 'removes extraneous whitespace without changing the escaped characters' do
+          given    = 'uid   =  Sebasti\\c3\\a1n\\ C.\\20Smith\\   ,   ou=People (aka. \\22humans\\")  ,dc=example, dc=com'
+          expected = 'uid=Sebasti\\c3\\a1n\\ C.\\20Smith\\ ,ou=People (aka. \\22humans\\"),dc=example,dc=com'
+          expect(described_class.normalize_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'with a multivalued RDN' do
+        it 'removes extraneous whitespace without modifying the multivalued RDN' do
+          given    = 'uid = John Smith  + telephoneNumber  = +1 555-555-5555 , ou = People,dc=example,dc=com'
+          expected = 'uid=John Smith+telephoneNumber=+1 555-555-5555,ou=People,dc=example,dc=com'
+          expect(described_class.normalize_dn(given)).to eq(expected)
+        end
+
+        context 'with a telephoneNumber with a space after the plus sign' do
+          # I am not sure whether a space after the telephoneNumber plus sign is valid,
+          # and I am not sure if this is "proper" behavior under these conditions, and
+          # I am not sure if it matters to us or anyone else, so rather than dig
+          # through RFCs, I am only documenting the behavior here.
+          it 'removes the space after the plus sign in the telephoneNumber' do
+            given    = 'uid = John Smith  + telephoneNumber  = + 1 555-555-5555 , ou = People,dc=example,dc=com'
+            expected = 'uid=John Smith+telephoneNumber=+1 555-555-5555,ou=People,dc=example,dc=com'
+            expect(described_class.normalize_dn(given)).to eq(expected)
+          end
+        end
+      end
+    end
+
+    context 'for a null DN (empty string)' do
+      it 'returns empty string and does not error' do
+        given    = ''
+        expected = ''
+        expect(described_class.normalize_dn(given)).to eq(expected)
+      end
+    end
+
+    context 'when there is an escaped leading space in an attribute value' do
+      it 'does not remove the escaped leading space (and does not error like Net::LDAP::DN.new does)' do
+        given    = 'uid=\\ John Smith,ou=People,dc=example,dc=com'
+        expected = 'uid=\\ John Smith,ou=People,dc=example,dc=com'
+        expect(described_class.normalize_dn(given)).to eq(expected)
+      end
+    end
+
+    context 'when there is an escaped trailing space in an attribute value' do
+      it 'does not remove the escaped trailing space' do
+        given    = 'uid=John Smith\\ ,ou=People,dc=example,dc=com'
+        expected = 'uid=John Smith\\ ,ou=People,dc=example,dc=com'
+        expect(described_class.normalize_dn(given)).to eq(expected)
+      end
+    end
+
+    context 'when there is an escaped leading newline in an attribute value' do
+      it 'does not remove the escaped leading newline' do
+        given    = 'uid=\\\nJohn Smith,ou=People,dc=example,dc=com'
+        expected = 'uid=\\\nJohn Smith,ou=People,dc=example,dc=com'
+        expect(described_class.normalize_dn(given)).to eq(expected)
+      end
+    end
+
+    context 'when there is an escaped trailing newline in an attribute value' do
+      it 'does not remove the escaped trailing newline' do
+        given    = 'uid=John Smith\\\n,ou=People,dc=example,dc=com'
+        expected = 'uid=John Smith\\\n,ou=People,dc=example,dc=com'
+        expect(described_class.normalize_dn(given)).to eq(expected)
+      end
+    end
+
+    context 'when there is an unescaped leading newline in an attribute value' do
+      it 'does not remove the unescaped leading newline' do
+        given    = 'uid=\nJohn Smith,ou=People,dc=example,dc=com'
+        expected = 'uid=\nJohn Smith,ou=People,dc=example,dc=com'
+        expect(described_class.normalize_dn(given)).to eq(expected)
+      end
+    end
+
+    context 'when there is an unescaped trailing newline in an attribute value' do
+      it 'does not remove the unescaped trailing newline' do
+        given    = 'uid=John Smith\n ,ou=People,dc=example,dc=com'
+        expected = 'uid=John Smith\n,ou=People,dc=example,dc=com'
+        expect(described_class.normalize_dn(given)).to eq(expected)
+      end
+    end
+
+    context 'with uppercase characters' do
+      # We may need to normalize casing at some point.
+      # I am just making it explicit that we don't at this time.
+      it 'returns the DN with unmodified casing' do
+        given    = 'UID=John Smith,ou=People,dc=example,dc=com'
+        expected = 'UID=John Smith,ou=People,dc=example,dc=com'
+        expect(described_class.normalize_dn(given)).to eq(expected)
+      end
+    end
+
+    context 'with a malformed DN' do
+      context 'when passed a UID instead of a DN' do
+        it 'returns the UID (with whitespace stripped)' do
+          given    = '  John C. Smith '
+          expected = 'John C. Smith'
+          expect(described_class.normalize_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'when an equal sign is escaped' do
+        it 'returns the DN completely unmodified' do
+          given    = 'uid= foo\\=bar'
+          expected = 'uid= foo\\=bar'
+          expect(described_class.normalize_dn(given)).to eq(expected)
+        end
+      end
+    end
+  end
+
   describe '#name' do
     it 'uses the configured name attribute and handles values as an array' do
       name = 'John Doe'