From fd0f495aea9211afc344e4c9cf6a80f24e9a24b2 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Fri, 13 May 2016 13:08:27 -0500
Subject: [PATCH 1/2] Clean DNs before storing extern_uid

---
 .../20160513152412_clean_ldap_extern_uids.rb  | 30 +++++++++++++++++++
 db/schema.rb                                  |  2 +-
 lib/gitlab/ldap/auth_hash.rb                  |  6 ++++
 lib/gitlab/ldap/person.rb                     | 15 +++++++++-
 lib/gitlab/o_auth/user.rb                     | 15 ++++++++--
 5 files changed, 64 insertions(+), 4 deletions(-)
 create mode 100644 db/migrate/20160513152412_clean_ldap_extern_uids.rb

diff --git a/db/migrate/20160513152412_clean_ldap_extern_uids.rb b/db/migrate/20160513152412_clean_ldap_extern_uids.rb
new file mode 100644
index 000000000000..78aee310fd74
--- /dev/null
+++ b/db/migrate/20160513152412_clean_ldap_extern_uids.rb
@@ -0,0 +1,30 @@
+require 'net/ldap/dn'
+
+class CleanLdapExternUids < ActiveRecord::Migration
+  def up
+    select_all("SELECT id, extern_uid FROM identities WHERE identities.provider LIKE 'ldap%'").each do |ldap_identity|
+      id = ldap_identity['id']
+      extern_uid_was = ldap_identity['extern_uid'].clone
+      extern_uid = clean_dn(ldap_identity['extern_uid'])
+
+      if extern_uid != extern_uid_was
+        execute("UPDATE identities SET extern_uid = '#{quote_string(extern_uid)}' WHERE id = #{id}")
+      end
+    end
+  end
+
+  def down
+    # There's no going back! Shouldn't be a big deal.
+  end
+
+  private
+
+  def clean_dn(dn)
+    clean_pairs = []
+    Net::LDAP::DN.new(dn).each_pair do |k,v|
+      # No need to escape the value. `each_pair` already escapes for us.
+      clean_pairs << "#{k}=#{v}"
+    end
+    Net::LDAP::DN.new(clean_pairs.join(',')).to_s
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 9b5aa640cb05..e3fd4c62da37 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160509201028) do
+ActiveRecord::Schema.define(version: 20160513152412) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
diff --git a/lib/gitlab/ldap/auth_hash.rb b/lib/gitlab/ldap/auth_hash.rb
index bf4dd9542d5d..0b0806fa730e 100644
--- a/lib/gitlab/ldap/auth_hash.rb
+++ b/lib/gitlab/ldap/auth_hash.rb
@@ -3,6 +3,12 @@
 module Gitlab
   module LDAP
     class AuthHash < Gitlab::OAuth::AuthHash
+      def uid
+        @uid ||= Gitlab::LDAP::Person.clean_dn(
+          Gitlab::Utils.force_utf8(auth_hash.uid.to_s)
+        )
+      end
+
       private
 
       def get_info(key)
diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb
index b81f3e8e8f5b..5e46205f26fa 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/lib/gitlab/ldap/person.rb
@@ -1,3 +1,5 @@
+require 'net/ldap/dn'
+
 module Gitlab
   module LDAP
     class Person
@@ -21,6 +23,17 @@ def self.disabled_via_active_directory?(dn, adapter)
         adapter.dn_matches_filter?(dn, AD_USER_DISABLED)
       end
 
+      # Sometimes LDAP contains dirty DNs. `Net::LDAP::DN.each_pair` does some
+      # magical clean up and then we put it all back together. See gitlab-ee#365
+      def self.clean_dn(dn)
+        clean_pairs = []
+        Net::LDAP::DN.new(dn).each_pair do |k,v|
+          # No need to escape the value. `each_pair` already escapes for us.
+          clean_pairs << "#{k}=#{v}"
+        end
+        Net::LDAP::DN.new(clean_pairs.join(',')).to_s
+      end
+
       def initialize(entry, provider)
         Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
         @entry = entry
@@ -44,7 +57,7 @@ def email
       end
 
       def dn
-        entry.dn
+        self.class.clean_dn(entry.dn)
       end
 
       private
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index 356e96fcbab1..4bc9b3df3202 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -72,7 +72,10 @@ def find_or_create_ldap_user
         # set up a Gitlab user with dual LDAP and Omniauth identities.
         if user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider)
           # Case when a LDAP user already exists in Gitlab. Add the Omniauth identity to existing account.
-          user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider)
+          user.identities.build(
+            extern_uid: Gitlab::LDAP::Person.clean_dn(auth_hash.uid),
+            provider: auth_hash.provider
+          )
         else
           # No account in Gitlab yet: create it and add the LDAP identity
           user = build_new_user
@@ -96,7 +99,15 @@ def ldap_person
         # Look for a corresponding person with same uid in any of the configured LDAP providers
         Gitlab::LDAP::Config.providers.each do |provider|
           adapter = Gitlab::LDAP::Adapter.new(provider)
-          @ldap_person = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter)
+
+          begin
+            dn = Gitlab::LDAP::Person.clean_dn(dn)
+            @ldap_person = Gitlab::LDAP::Person.find_by_dn(dn, adapter)
+          rescue RuntimeError => e
+            log.warn "Unable to parse DN. #{e.message}"
+          rescue RuntimeError => e
+          end
+
           break if @ldap_person
         end
         @ldap_person
-- 
GitLab


From 455a61f1b3e7117b7f424c457a36fbd8641d79f3 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Fri, 13 May 2016 14:13:54 -0500
Subject: [PATCH 2/2] Fix

---
 lib/gitlab/ldap/auth_hash.rb | 18 +++++++++++++++---
 lib/gitlab/o_auth/user.rb    |  3 +--
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/lib/gitlab/ldap/auth_hash.rb b/lib/gitlab/ldap/auth_hash.rb
index 0b0806fa730e..40a40d92ba6b 100644
--- a/lib/gitlab/ldap/auth_hash.rb
+++ b/lib/gitlab/ldap/auth_hash.rb
@@ -4,9 +4,17 @@ module Gitlab
   module LDAP
     class AuthHash < Gitlab::OAuth::AuthHash
       def uid
-        @uid ||= Gitlab::LDAP::Person.clean_dn(
-          Gitlab::Utils.force_utf8(auth_hash.uid.to_s)
-        )
+        return @uid if @uid
+
+        utf8_dn = Gitlab::Utils.force_utf8(auth_hash.uid.to_s)
+        begin
+          @uid = Gitlab::LDAP::Person.clean_dn(utf8_dn)
+        rescue RuntimeError => e
+          logger.error { "Badly formed DN sent from LDAP auth_hash: #{utf8_dn}; #{e.message}" }
+          @uid = utf8_dn
+        end
+
+        @uid
       end
 
       private
@@ -37,6 +45,10 @@ def get_raw(key)
       def ldap_config
         @ldap_config ||= Gitlab::LDAP::Config.new(self.provider)
       end
+
+      def logger
+        Rails.logger
+      end
     end
   end
 end
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index 4bc9b3df3202..e9a51c9da42e 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -104,8 +104,7 @@ def ldap_person
             dn = Gitlab::LDAP::Person.clean_dn(dn)
             @ldap_person = Gitlab::LDAP::Person.find_by_dn(dn, adapter)
           rescue RuntimeError => e
-            log.warn "Unable to parse DN. #{e.message}"
-          rescue RuntimeError => e
+            log.error "Unable to parse DN. #{e.message}"
           end
 
           break if @ldap_person
-- 
GitLab