Merge branch 'feature/import_kerberos_identities_ad' of...

Merge branch 'feature/import_kerberos_identities_ad' of https://gitlab.com/cernvcs/gitlab-ee into ee-master

CHANGELOG-EE

@@ -6,6 +6,9 @@ v 8.3.0
   - Fix bug with negative approvals required
   - Add group contribution analytics page 
   - Add GitLab Pages
+  - Add group contribution statistics page 
+  - Automatically import Kerberos identities from Active Directory when Kerberos is enabled (Alex Lossent)
+  - Canonicalization of Kerberos identities to always include realm (Alex Lossent)
 v 8.2.3
   - No EE-specific changes

db/migrate/20151113115819_canonicalize_kerberos_identities.rb

+class CanonicalizeKerberosIdentities < ActiveRecord::Migration
+  # This migration can be performed online without errors.
+  # It makes sure that all Kerberos identities are in canonical form
+  # with a realm name (`username` => `username@DEFAULT.REALM`).
+  # Before this migration, Kerberos identities using the default realm are typically stored
+  # without the realm part.
+
+  def kerberos_default_realm
+    @kerberos_default_realm ||= begin
+      require "krb5_auth"
+      krb5 = ::Krb5Auth::Krb5.new
+      default_realm = krb5.get_default_realm
+      krb5.close # release memory allocated by the krb5 library
+      default_realm || ''
+    rescue Exception
+      '' # could not find the system's default realm, maybe there's no Kerberos at all
+    end
+  end
+
+  def change
+    reversible do |dir|
+      return unless kerberos_default_realm.present?
+
+      dir.up do
+        # add the default realm to any kerberos identity not having a realm already
+        execute("UPDATE identities SET extern_uid = CONCAT(extern_uid, '@#{quote_string(kerberos_default_realm)}')
+                 WHERE provider = 'kerberos' AND extern_uid NOT LIKE '%@%'")
+      end
+
+      dir.down do
+        # remove the realm from kerberos identities using the default realm
+        execute("UPDATE identities SET extern_uid = REPLACE(extern_uid, '@#{quote_string(kerberos_default_realm)}', '')
+                 WHERE provider = 'kerberos' AND extern_uid LIKE '%@#{quote_string(kerberos_default_realm)}'")
+      end
+    end
+  end
+end

lib/gitlab/backend/grack_auth.rb

@@ -5,7 +5,7 @@ module Grack
     def self.call(env)
       # Avoid issues with instance variables in Grack::Auth persisting across
       # requests by creating a new instance for each request.
-      Auth.new({}).call(env)
+      Auth.new({}).call_with_kerberos_support(env)
     end
   end
@@ -13,6 +13,11 @@ module Grack
     attr_accessor :user, :project, :env
+    def call_with_kerberos_support(env)
+      # Make sure the final leg of Kerberos authentication is applied as per RFC4559
+      apply_negotiate_final_leg(call(env))
+    end
+
     def call(env)
       @env = env
       @request = Rack::Request.new(env)
@@ -42,7 +47,7 @@ module Grack
       elsif @user.nil? && !@ci
         unauthorized
       else
-        apply_negotiate_final_leg(render_not_found)
+        render_not_found
       end
     end
@@ -99,8 +104,7 @@ module Grack
         return unless krb_principal
         # Set @user if authentication succeeded
-        identity = ::Identity.find_by(provider: 'kerberos', extern_uid: krb_principal)
-        identity ||= ::Identity.find_by(provider: 'kerberos', extern_uid: krb_principal.split("@")[0])
+        identity = ::Identity.find_by(provider: :kerberos, extern_uid: krb_principal)
         @user = identity.user if identity
       else
         # Authentication with username and password

lib/gitlab/kerberos/authentication.rb

@@ -4,6 +4,13 @@ require "krb5_auth"
 module Gitlab
   module Kerberos
     class Authentication
+      def self.kerberos_default_realm
+        krb5 = ::Krb5Auth::Krb5.new
+        default_realm = krb5.get_default_realm
+        krb5.close # release memory allocated by the krb5 library
+        default_realm
+      end
+
       def self.login(login, password)
         return unless Devise.omniauth_providers.include?(:kerberos)
         return unless login.present? && password.present?
@@ -25,15 +32,14 @@ module Gitlab
       end
       def login
-        valid? && find_by_login(@login)
+        # get_default_principal consistently returns the canonical Kerberos principal name, with realm
+        valid? && find_by_login(@krb5.get_default_principal)
       end
       private
       def find_by_login(login)
-        identity = ::Identity.
-          where(provider: :kerberos).
-          where('lower(extern_uid) = ?', login).last
+        identity = ::Identity.find_by(provider: :kerberos, extern_uid: login)
         identity && identity.user
       end
     end

lib/gitlab/ldap/access.rb

@@ -72,6 +72,8 @@ module Gitlab
           update_ssh_keys
         end
+        update_kerberos_identity if import_kerberos_identities?
+
         # Skip updating group permissions
         # if instance does not use group_base setting
         return true unless group_base.present?
@@ -118,6 +120,21 @@ module Gitlab
         end
       end
+      # Update user Kerberos identity with Kerberos principal name from Active Directory
+      def update_kerberos_identity
+        # there can be only one Kerberos identity in GitLab; if the user has a Kerberos identity in AD,
+        # replace any existing Kerberos identity for the user
+        return unless ldap_user.kerberos_principal.present?
+        kerberos_identity = user.identities.where(provider: :kerberos).first
+        return if kerberos_identity && kerberos_identity.extern_uid == ldap_user.kerberos_principal
+        kerberos_identity ||= Identity.new(provider: :kerberos, user: user)
+        kerberos_identity.extern_uid = ldap_user.kerberos_principal
+        unless kerberos_identity.save
+          Rails.logger.error "#{self.class.name}: failed to add Kerberos principal #{principal} to #{user.name} (#{user.id})\n"\
+            "error messages: #{new_identity.errors.messages}"
+        end
+      end
+
       # Update user email if it changed in LDAP
       def update_email
         return false unless ldap_user.try(:email)
@@ -183,6 +200,11 @@ module Gitlab
         ldap_config.sync_ssh_keys?
       end
+      def import_kerberos_identities?
+        # Kerberos may be enabled for Git HTTP access and/or as an Omniauth provider
+        ldap_config.active_directory && (Gitlab.config.kerberos.enabled || AuthHelper.kerberos_enabled? )
+      end
+
       def group_base
         ldap_config.group_base
       end

lib/gitlab/ldap/person.rb

@@ -57,6 +57,25 @@ module Gitlab
         end
       end
+      def kerberos_principal
+        # The following is only meaningful for Active Directory
+        return unless entry.respond_to?(:sAMAccountName)
+        entry[:sAMAccountName].first + '@' + windows_domain_name.upcase
+      end
+
+      def windows_domain_name
+        # The following is only meaningful for Active Directory
+        require 'net/ldap/dn'
+        dn_components=[]
+        Net::LDAP::DN.new(dn).each_pair { |name, value| dn_components << { name: name, value: value } }
+        dn_components.
+          reverse.
+          take_while { |rdn| rdn[:name].upcase=='DC' }. # Domain Component
+          map { |rdn| rdn[:value] }.
+          reverse.
+          join('.')
+      end
+
       private
       def entry

lib/gitlab/o_auth/auth_hash.rb

@@ -8,8 +8,21 @@ module Gitlab
         @auth_hash = auth_hash
       end
+      def kerberos_default_realm
+        Gitlab::Kerberos::Authentication.kerberos_default_realm
+      end
+
+      def normalized_uid
+        return auth_hash.uid.to_s unless provider == 'kerberos'
+        # For Kerberos, usernames `principal` and `principal@DEFAULT.REALM` are equivalent and
+        # may be used indifferently, but omniauth_kerberos does not normalize them as of version 0.3.0.
+        # Normalize here the uid to always have the canonical Kerberos principal name with realm.
+        return auth_hash.uid if auth_hash.uid.include?("@")
+        auth_hash.uid + "@" + kerberos_default_realm
+      end
+
       def uid
-        @uid ||= Gitlab::Utils.force_utf8(auth_hash.uid.to_s)
+        @uid ||= Gitlab::Utils.force_utf8(normalized_uid)
       end
       def provider

spec/lib/gitlab/backend/grack_auth_spec.rb

@@ -5,7 +5,6 @@ describe Grack::Auth, lib: true do
   let(:project) { create(:project) }
   let(:app)   { lambda { |env| [200, {}, "Success!"] } }
-  let!(:auth) { Grack::Auth.new(app) }
   let(:env) do
     {
       'rack.input'     => '',
@@ -13,7 +12,7 @@ describe Grack::Auth, lib: true do
       'QUERY_STRING'   => 'service=git-upload-pack'
     }
   end
-  let(:status) { auth.call(env).first }
+  let(:status) { Grack::AuthSpawner::call(env).first }
   describe "#call" do
     context "when the project doesn't exist" do
@@ -58,7 +57,7 @@ describe Grack::Auth, lib: true do
       end
       it "responds with the right project" do
-        response = auth.call(env)
+        response = Grack::AuthSpawner::call(env)
         json_body = ActiveSupport::JSON.decode(response[2][0])
         expect(response.first).to eq(200)
@@ -92,6 +91,83 @@ describe Grack::Auth, lib: true do
           end
         end
+        context "when Kerberos token is provided" do
+          before do
+            allow_any_instance_of(Grack::Auth).to receive(:allow_kerberos_auth?).and_return(true)
+            env["HTTP_AUTHORIZATION"] = "Negotiate #{::Base64.strict_encode64('opaque_request_token')}"
+          end
+
+          shared_examples "RFC4559 compliance" do
+            it "complies with RFC4559" do
+              allow_any_instance_of(Grack::Auth::Request).to receive(:spnego_response_token).and_return("opaque_response_token")
+
+              headers = Grack::AuthSpawner::call(env)[1]
+              expect(headers['WWW-Authenticate'].split("\n")).to include("Negotiate #{::Base64.strict_encode64('opaque_response_token')}")
+            end
+          end
+
+          context "when authentication fails because of invalid Kerberos token" do
+            before do
+              allow_any_instance_of(Grack::Auth::Request).to receive(:spnego_credentials!).and_return(nil)
+            end
+
+            it "responds with status 401" do
+              expect(status).to eq(401)
+            end
+          end
+
+          context "when authentication fails because of unknown Kerberos identity" do
+            before do
+              allow_any_instance_of(Grack::Auth::Request).to receive(:spnego_credentials!).and_return("mylogin@FOO.COM")
+            end
+
+            it "responds with status 401" do
+              expect(status).to eq(401)
+            end
+
+          end
+
+          context "when authentication succeeds" do
+            before do
+              allow_any_instance_of(Grack::Auth::Request).to receive(:spnego_credentials!).and_return("mylogin@FOO.COM")
+              user.identities.build(provider: "kerberos", extern_uid:"mylogin@FOO.COM").save
+            end
+
+            context "when the user has access to the project" do
+              before do
+                project.team << [user, :master]
+              end
+
+              context "when the user is blocked" do
+                before do
+                  user.block
+                  project.team << [user, :master]
+                end
+
+                it "responds with status 404" do
+                  expect(status).to eq(404)
+                end
+              end
+
+              context "when the user isn't blocked" do
+                it "responds with status 200" do
+                  expect(status).to eq(200)
+                end
+              end
+
+              include_examples "RFC4559 compliance"
+            end
+
+            context "when the user doesn't have access to the project" do
+              it "responds with status 404" do
+                expect(status).to eq(404)
+              end
+
+              include_examples "RFC4559 compliance"
+            end
+          end
+        end
+
         context "when username and password are provided" do
           context "when authentication fails" do
             before do
@@ -162,8 +238,7 @@ describe Grack::Auth, lib: true do
                 def attempt_login(include_password)
                   password = include_password ? user.password : ""
                   env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, password)
-                  Grack::Auth.new(app)
-                  auth.call(env).first
+                  Grack::AuthSpawner::call(env).first
                 end
                 it "repeated attempts followed by successful attempt" do

spec/lib/gitlab/kerberos/authentication_spec.rb

@@ -2,26 +2,39 @@ require 'spec_helper'
 describe Gitlab::Kerberos::Authentication do
   let(:klass) { Gitlab::Kerberos::Authentication }
-  let(:user) { create(:omniauth_user, provider: :kerberos, extern_uid: 'gitlab') }
+  let(:user) { create(:omniauth_user, provider: :kerberos, extern_uid: 'gitlab@FOO.COM') }
   let(:login) { 'john' }
   let(:password) { 'password' }
+  describe :kerberos_default_realm do
+    it "returns the default realm exposed by the Kerberos library" do
+      allow_any_instance_of(::Krb5Auth::Krb5).to receive_messages(get_default_realm: "FOO.COM")
+
+      expect(klass.kerberos_default_realm).to eq("FOO.COM")
+    end
+  end
+
   describe :login do
     before do
       allow(Devise).to receive_messages(omniauth_providers: [:kerberos])
+      user # make sure user is instanciated
     end
-    it "finds the user if authentication is successful" do
-      kerberos_realm = user.email.sub(/.*@/, '')
-      allow_any_instance_of(::Krb5Auth::Krb5).to receive_messages(get_init_creds_password: true, get_default_realm: kerberos_realm)
+    it "finds the user if authentication is successful (login without kerberos realm)" do
+      allow_any_instance_of(::Krb5Auth::Krb5).to receive_messages(get_init_creds_password: true, get_default_principal: 'gitlab@FOO.COM')
       expect(klass.login('gitlab', password)).to be_truthy
     end
+    it "finds the user if authentication is successful (login with a kerberos realm)" do
+      allow_any_instance_of(::Krb5Auth::Krb5).to receive_messages(get_init_creds_password: true, get_default_principal: 'gitlab@FOO.COM')
+
+      expect(klass.login('gitlab@FOO.COM', password)).to be_truthy
+    end
+
     it "returns false if there is no such user in kerberos" do
       kerberos_login = "some-login"
-      kerberos_realm = user.email.sub(/.*@/, '')
-      allow_any_instance_of(::Krb5Auth::Krb5).to receive_messages(get_init_creds_password: true, get_default_realm: kerberos_realm)
+      allow_any_instance_of(::Krb5Auth::Krb5).to receive_messages(get_init_creds_password: true, get_default_principal: 'some-login@FOO.COM')
       expect(klass.login(kerberos_login, password)).to be_falsy
     end

spec/lib/gitlab/ldap/access_spec.rb

@@ -93,32 +93,79 @@ describe Gitlab::LDAP::Access, lib: true do
     subject { access.update_permissions }
     it "syncs ssh keys if enabled by configuration" do
-      allow(access).to receive_messages(sync_ssh_keys?: 'sshpublickey')
+      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: 'sshpublickey', import_kerberos_identities?: false)
       expect(access).to receive(:update_ssh_keys).once
       subject
     end
     it "does update group permissions with a group base configured" do
-      allow(access).to receive_messages(group_base: 'my-group-base')
+      allow(access).to receive_messages(group_base: 'my-group-base', sync_ssh_keys?: false, import_kerberos_identities?: false)
       expect(access).to receive(:update_ldap_group_links)
       subject
     end
     it "does not update group permissions without a group base configured" do
-      allow(access).to receive_messages(group_base: '')
+      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: false, import_kerberos_identities?: false)
       expect(access).not_to receive(:update_ldap_group_links)
       subject
     end
     it "does update admin group permissions if admin group is configured" do
-      allow(access).to receive_messages(admin_group: 'my-admin-group', update_ldap_group_links: nil)
+      allow(access).to receive_messages(admin_group: 'my-admin-group', update_ldap_group_links: nil, sync_ssh_keys?: false, import_kerberos_identities?: false)
       expect(access).to receive(:update_admin_status)
       subject
     end
+
+    it "does update Kerberos identities if Kerberos is enabled and the LDAP server is Active Directory" do
+      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: false, import_kerberos_identities?: true)
+
+      expect(access).to receive(:update_kerberos_identity)
+
+      subject
+    end
+  end
+
+  describe :update_kerberos_identity do
+    let(:entry) do
+      Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com")
+    end
+
+    before do
+      allow(access).to receive_messages(ldap_user: Gitlab::LDAP::Person.new(entry, user.ldap_identity.provider))
+    end
+
+    it "should add a Kerberos identity if it is in Active Directory but not in GitLab" do
+      allow_any_instance_of(Gitlab::LDAP::Person).to receive_messages(kerberos_principal: "mylogin@FOO.COM")
+
+      expect{ access.update_kerberos_identity }.to change(user.identities.where(provider: :kerberos), :count).from(0).to(1)
+      expect(user.identities.where(provider: "kerberos").last.extern_uid).to eq("mylogin@FOO.COM")
+    end
+
+    it "should update existing Kerberos identity in GitLab if Active Directory has a different one" do
+      allow_any_instance_of(Gitlab::LDAP::Person).to receive_messages(kerberos_principal: "otherlogin@BAR.COM")
+      user.identities.build(provider: "kerberos", extern_uid: "mylogin@FOO.COM").save
+
+      expect{ access.update_kerberos_identity }.not_to change(user.identities.where(provider: "kerberos"), :count)
+      expect(user.identities.where(provider: "kerberos").last.extern_uid).to eq("otherlogin@BAR.COM")
+    end
+
+    it "should not remove Kerberos identities from GitLab if they are none in the LDAP provider" do
+      allow_any_instance_of(Gitlab::LDAP::Person).to receive_messages(kerberos_principal: nil)
+      user.identities.build(provider: "kerberos", extern_uid: "otherlogin@BAR.COM").save
+
+      expect{ access.update_kerberos_identity }.not_to change(user.identities.where(provider: "kerberos"), :count)
+      expect(user.identities.where(provider: "kerberos").last.extern_uid).to eq("otherlogin@BAR.COM")
+    end
+
+    it "should not modify identities in GitLab if they are no kerberos principal in the LDAP provider" do
+      allow_any_instance_of(Gitlab::LDAP::Person).to receive_messages(kerberos_principal: nil)
+
+      expect{ access.update_kerberos_identity }.not_to change(user.identities, :count)
+    end
   end
   describe :update_ssh_keys do

spec/lib/gitlab/ldap/person_spec.rb

@@ -2,6 +2,35 @@ require "spec_helper"
 describe Gitlab::LDAP::Person do
+  describe "#kerberos_principal" do
+
+    let(:entry) do
+      ldif = "dn: cn=foo, dc=bar, dc=com\n"
+      ldif += "sAMAccountName: #{sam_account_name}\n" if sam_account_name
+      Net::LDAP::Entry.from_single_ldif_string(ldif)
+    end
+
+    subject { Gitlab::LDAP::Person.new(entry, 'ldapmain') }
+
+    context "when sAMAccountName is not defined (non-AD LDAP server)" do
+
+      let(:sam_account_name) { nil }
+
+      it "returns nil" do
+        expect(subject.kerberos_principal).to be_nil
+      end
+    end
+
+    context "when sAMAccountName is defined (AD server)" do
+
+      let(:sam_account_name) { "mylogin" }
+
+      it "returns the principal combining sAMAccountName and DC components of the distinguishedName" do
+        expect(subject.kerberos_principal).to eq("mylogin@BAR.COM")
+      end
+    end
+  end
+
   describe "#ssh_keys" do
     let(:ssh_key) { "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCrSQHff6a1rMqBdHFt+FwIbytMZ+hJKN3KLkTtOWtSvNIriGhnTdn4rs+tjD/w+z+revytyWnMDM9dS7J8vQi006B16+hc9Xf82crqRoPRDnBytgAFFQY1G/55ql2zdfsC5yvpDOFzuwIJq5dNGsojS82t6HNmmKPq130fzsenFnj5v1pl3OJvk513oduUyKiZBGTroWTn7H/eOPtu7s9MD7pAdEjqYKFLeaKmyidiLmLqQlCRj3Tl2U9oyFg4PYNc0bL5FZJ/Z6t0Ds3i/a2RanQiKxrvgu3GSnUKMx7WIX373baL4jeM7cprRGiOY/1NcS+1cAjfJ8oaxQF/1dYj" }

spec/lib/gitlab/o_auth/auth_hash_spec.rb

@@ -54,6 +54,29 @@ describe Gitlab::OAuth::AuthHash, lib: true do
     it { expect(auth_hash.password).not_to be_empty }
   end
+  context 'with kerberos provider' do
+    let(:provider_ascii) { 'kerberos'.force_encoding(Encoding::ASCII_8BIT) }
+
+    context "and uid contains a kerberos realm" do
+      let(:uid_ascii) { 'mylogin@BAR.COM'.force_encoding(Encoding::ASCII_8BIT) }
+
+      it "preserves the canonical uid" do
+        expect(auth_hash.uid).to eq('mylogin@BAR.COM')
+      end
+    end
+
+    context "and uid does not contain a kerberos realm" do
+      let(:uid_ascii) { 'mylogin'.force_encoding(Encoding::ASCII_8BIT) }
+      before do
+        allow(Gitlab::Kerberos::Authentication).to receive(:kerberos_default_realm).and_return("FOO.COM")
+      end
+
+      it "canonicalizes uid with kerberos realm" do
+        expect(auth_hash.uid).to eq('mylogin@FOO.COM')
+      end
+    end
+  end
+
   context 'email not provided' do
     before { info_hash.delete(:email) }