Canonicalization of Kerberos identities to always include realm

This resolve inconsistences in Kerberos identities, as sometimes
the realm would be included and sometimes not. Cf. gitlab-org/gitlab-ee#41

CHANGELOG-EE

@@ -18,6 +18,7 @@ v 8.2.2
 v 8.2.1
   - Forcefully update builds that didn't want to update with state machine
   - Fix: saving GitLabCiService as Admin Template
+  - Canonicalization of Kerberos identities to always include realm (Alex Lossent)
 
 v 8.2.0
   - Invalidate stored jira password if the endpoint URL is changed
@@ -46,6 +47,8 @@ v 8.1.0
   - Added an issues template (Hannes Rosenögger)
   - Add documentation for "Share project with group" API call 
   - Ability to disable 'Share with Group' feature (via UI and API)
+  - Add documentation for "Share project with group" API call
+  - Abiliy to disable 'Share with Group' feature (via UI and API)
 
 v 8.0.6
   - 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/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 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 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 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 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 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/o_auth/auth_hash_spec.rb

@@ -54,6 +54,29 @@ describe Gitlab::OAuth::AuthHash 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) }