Merge branch 'security-sarcila-verify-saml-request-origin' into 'master'

Check that SAML identity linking validates the origin of the request

See merge request gitlab/gitlab-ee!1207

app/controllers/omniauth_callbacks_controller.rb

@@ -40,6 +40,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
 
   def saml
     omniauth_flow(Gitlab::Auth::Saml)
+  rescue Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest
+    redirect_unverified_saml_initiation
   end
 
   def omniauth_error
@@ -92,8 +94,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
       return render_403 unless link_provider_allowed?(oauth['provider'])
 
       log_audit_event(current_user, with: oauth['provider'])
-
-      identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth)
+      identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth, session)
 
       link_identity(identity_linker)
 
@@ -194,6 +195,10 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     redirect_to new_user_session_path
   end
 
+  def redirect_unverified_saml_initiation
+    redirect_to profile_account_path, notice: _('Request to link SAML account must be authorized')
+  end
+
   def handle_disabled_provider
     label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider'])
     flash[:alert] = _("Signing in using %{label} has been disabled") % { label: label }

changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml

+---
+title: Prevent GitLab accounts takeover if SAML is configured
+merge_request:
+author:
+type: security

ee/app/controllers/groups/omniauth_callbacks_controller.rb

@@ -9,10 +9,10 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
     @unauthenticated_group = Group.find_by_full_path(params[:group_id])
     @saml_provider = @unauthenticated_group.saml_provider
 
-    identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(current_user, oauth, @saml_provider, session)
+    identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(current_user, oauth, session, @saml_provider)
 
     omniauth_flow(Gitlab::Auth::GroupSaml, identity_linker: identity_linker)
-  rescue Gitlab::Auth::GroupSaml::IdentityLinker::UnverifiedRequest
+  rescue Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest
     redirect_unverified_saml_initiation
   end
 

ee/app/services/group_saml/sign_up_service.rb

@@ -15,7 +15,7 @@ module GroupSaml
         new_user.managing_group = group if group.saml_provider&.enforced_group_managed_accounts?
 
         if new_user.save
-          identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(new_user, oauth_data, group.saml_provider, session)
+          identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(new_user, oauth_data, session, group.saml_provider)
           identity_linker.link
         end
 

ee/lib/gitlab/auth/group_saml/identity_linker.rb

@@ -4,20 +4,16 @@ module Gitlab
   module Auth
     module GroupSaml
       class IdentityLinker < Gitlab::Auth::Saml::IdentityLinker
-        attr_reader :saml_provider, :session
+        attr_reader :saml_provider
 
-        UnverifiedRequest = Class.new(StandardError)
-
-        def initialize(current_user, oauth, saml_provider, session)
-          super(current_user, oauth)
+        def initialize(current_user, oauth, session, saml_provider)
+          super(current_user, oauth, session)
 
           @saml_provider = saml_provider
-          @session = session
         end
 
+        override :link
         def link
-          raise_unless_request_is_gitlab_initiated! if unlinked?
-
           super
 
           update_group_membership unless failed?
@@ -37,18 +33,6 @@ module Gitlab
         def update_group_membership
           MembershipUpdater.new(current_user, saml_provider).execute
         end
-
-        def raise_unless_request_is_gitlab_initiated!
-          raise UnverifiedRequest unless valid_gitlab_initated_request?
-        end
-
-        def valid_gitlab_initated_request?
-          SamlOriginValidator.new(session).gitlab_initiated?(saml_response)
-        end
-
-        def saml_response
-          oauth.extra.response_object
-        end
       end
     end
   end

ee/lib/gitlab/auth/saml_origin_validator.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Auth
-    class SamlOriginValidator
-      attr_reader :session
-
-      AUTH_REQUEST_SESSION_KEY = "last_authn_request_id".freeze
-
-      def initialize(session)
-        @session = session
-      end
-
-      def store_origin(authn_request)
-        session[AUTH_REQUEST_SESSION_KEY] = authn_request.uuid
-      end
-
-      def gitlab_initiated?(saml_response)
-        return false if identity_provider_initiated?(saml_response)
-
-        matches?(saml_response)
-      end
-
-      private
-
-      def matches?(saml_response)
-        saml_response.in_response_to == expected_request_id
-      end
-
-      def identity_provider_initiated?(saml_response)
-        saml_response.in_response_to.blank?
-      end
-
-      def expected_request_id
-        session[AUTH_REQUEST_SESSION_KEY]
-      end
-    end
-  end
-end

ee/lib/omni_auth/strategies/group_saml.rb

@@ -40,20 +40,6 @@ module OmniAuth
         end
       end
 
-      # NOTE: This method duplicates code from omniauth-saml
-      #       so that we can access authn_request to store it
-      #       See: https://github.com/omniauth/omniauth-saml/issues/172
-      override :request_phase
-      def request_phase
-        authn_request = OneLogin::RubySaml::Authrequest.new
-
-        store_authn_request_id(authn_request)
-
-        with_settings do |settings|
-          redirect(authn_request.create(settings, additional_params_for_authn_request))
-        end
-      end
-
       def emulate_relay_state
         request.query_string.sub!('redirect_to', 'RelayState')
       end
@@ -85,10 +71,6 @@ module OmniAuth
         on_subpath?(:metadata)
       end
 
-      def store_authn_request_id(authn_request)
-        Gitlab::Auth::SamlOriginValidator.new(session).store_origin(authn_request)
-      end
-
       def group_lookup
         @group_lookup ||= Gitlab::Auth::GroupSaml::GroupLookup.new(env)
       end

ee/spec/features/profiles/account_spec.rb

@@ -20,8 +20,8 @@ describe 'Profile > Account' do
 
     def create_linked_identity
       oauth = { 'provider' => 'group_saml', 'uid' => '1' }
-      identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(user, oauth, saml_provider, double(:session))
-      allow(identity_linker).to receive(:valid_gitlab_initated_request?).and_return(true)
+      identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(user, oauth, double(:session), saml_provider)
+      allow(identity_linker).to receive(:valid_gitlab_initiated_request?).and_return(true)
       identity_linker.link
     end
 

ee/spec/lib/gitlab/auth/group_saml/identity_linker_spec.rb

@@ -10,7 +10,7 @@ describe Gitlab::Auth::GroupSaml::IdentityLinker do
   let(:saml_provider) { create(:saml_provider) }
   let(:session) { {} }
 
-  subject { described_class.new(user, oauth, saml_provider, session) }
+  subject { described_class.new(user, oauth, session, saml_provider) }
 
   context 'linked identity exists' do
     let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid, saml_provider: saml_provider) }
@@ -35,7 +35,7 @@ describe Gitlab::Auth::GroupSaml::IdentityLinker do
   context 'identity needs to be created' do
     context 'with identity provider initiated request' do
       it 'attempting to link accounts raises an exception' do
-        expect { subject.link }.to raise_error(Gitlab::Auth::GroupSaml::IdentityLinker::UnverifiedRequest)
+        expect { subject.link }.to raise_error(Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest)
       end
     end
 

ee/spec/services/group_saml/sign_up_service_spec.rb

@@ -14,7 +14,7 @@ describe GroupSaml::SignUpService do
 
     before do
       allow(Gitlab::Auth::GroupSaml::IdentityLinker)
-        .to receive(:new).with(new_user, session['oauth_data'], group.saml_provider, session)
+        .to receive(:new).with(new_user, session['oauth_data'], session, group.saml_provider)
               .and_return(linker_spy)
     end
 

ee/spec/support/helpers/ee/login_helpers.rb

@@ -20,7 +20,7 @@ module EE
 
     def mock_group_saml(uid:)
       allow(Devise).to receive(:omniauth_providers).and_return(%i(group_saml))
-      allow_any_instance_of(::Gitlab::Auth::SamlOriginValidator).to receive(:gitlab_initiated?).and_return(true)
+      allow_any_instance_of(::Gitlab::Auth::Saml::OriginValidator).to receive(:gitlab_initiated?).and_return(true)
       configure_group_saml_mock_auth(uid: uid)
       stub_omniauth_provider(:group_saml)
     end

lib/gitlab/auth/omniauth_identity_linker_base.rb

@@ -3,12 +3,13 @@
 module Gitlab
   module Auth
     class OmniauthIdentityLinkerBase
-      attr_reader :current_user, :oauth
+      attr_reader :current_user, :oauth, :session
 
-      def initialize(current_user, oauth)
+      def initialize(current_user, oauth, session = {})
         @current_user = current_user
         @oauth = oauth
         @changed = false
+        @session = session
       end
 
       def link

lib/gitlab/auth/saml/identity_linker.rb

@@ -4,6 +4,30 @@ module Gitlab
   module Auth
     module Saml
       class IdentityLinker < OmniauthIdentityLinkerBase
+        extend ::Gitlab::Utils::Override
+
+        UnverifiedRequest = Class.new(StandardError)
+
+        override :link
+        def link
+          raise_unless_request_is_gitlab_initiated! if unlinked?
+
+          super
+        end
+
+        protected
+
+        def raise_unless_request_is_gitlab_initiated!
+          raise UnverifiedRequest unless valid_gitlab_initiated_request?
+        end
+
+        def valid_gitlab_initiated_request?
+          OriginValidator.new(session).gitlab_initiated?(saml_response)
+        end
+
+        def saml_response
+          oauth.fetch(:extra, {}).fetch(:response_object, {})
+        end
       end
     end
   end

lib/gitlab/auth/saml/origin_validator.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module Saml
+      class OriginValidator
+        AUTH_REQUEST_SESSION_KEY = "last_authn_request_id".freeze
+
+        def initialize(session)
+          @session = session || {}
+        end
+
+        def store_origin(authn_request)
+          session[AUTH_REQUEST_SESSION_KEY] = authn_request.uuid
+        end
+
+        def gitlab_initiated?(saml_response)
+          return false if identity_provider_initiated?(saml_response)
+
+          matches?(saml_response)
+        end
+
+        private
+
+        attr_reader :session
+
+        def matches?(saml_response)
+          saml_response.in_response_to == expected_request_id
+        end
+
+        def identity_provider_initiated?(saml_response)
+          saml_response.in_response_to.blank?
+        end
+
+        def expected_request_id
+          session[AUTH_REQUEST_SESSION_KEY]
+        end
+      end
+    end
+  end
+end

lib/omni_auth/strategies/saml.rb

+# frozen_string_literal: true
+
+module OmniAuth
+  module Strategies
+    class SAML
+      extend ::Gitlab::Utils::Override
+
+      # NOTE: This method duplicates code from omniauth-saml
+      #       so that we can access authn_request to store it
+      #       See: https://github.com/omniauth/omniauth-saml/issues/172
+      override :request_phase
+      def request_phase
+        authn_request = OneLogin::RubySaml::Authrequest.new
+
+        store_authn_request_id(authn_request)
+
+        with_settings do |settings|
+          redirect(authn_request.create(settings, additional_params_for_authn_request))
+        end
+      end
+
+      private
+
+      def store_authn_request_id(authn_request)
+        Gitlab::Auth::Saml::OriginValidator.new(session).store_origin(authn_request)
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -13206,6 +13206,9 @@ msgstr ""
 msgid "Request Access"
 msgstr ""
 
+msgid "Request to link SAML account must be authorized"
+msgstr ""
+
 msgid "Requested %{time_ago}"
 msgstr ""
 

spec/controllers/omniauth_callbacks_controller_spec.rb

@@ -247,34 +247,70 @@ describe OmniauthCallbacksController, type: :controller do
   end
 
   describe '#saml' do
+    let(:last_request_id) { 'ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685' }
     let(:user) { create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') }
     let(:mock_saml_response) { File.read('spec/fixtures/authentication/saml_response.xml') }
     let(:saml_config) { mock_saml_config_with_upstream_two_factor_authn_contexts }
 
+    def stub_last_request_id(id)
+      session['last_authn_request_id'] = id
+    end
+
     before do
+      stub_last_request_id(last_request_id)
       stub_omniauth_saml_config({ enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'],
                                   providers: [saml_config] })
       mock_auth_hash_with_saml_xml('saml', +'my-uid', user.email, mock_saml_response)
-      request.env["devise.mapping"] = Devise.mappings[:user]
+      request.env['devise.mapping'] = Devise.mappings[:user]
       request.env['omniauth.auth'] = Rails.application.env_config['omniauth.auth']
-      post :saml, params: { SAMLResponse: mock_saml_response }
     end
 
-    context 'when worth two factors' do
-      let(:mock_saml_response) do
-        File.read('spec/fixtures/authentication/saml_response.xml')
+    context 'with GitLab initiated request' do
+      before do
+        post :saml, params: { SAMLResponse: mock_saml_response }
+      end
+
+      context 'when worth two factors' do
+        let(:mock_saml_response) do
+          File.read('spec/fixtures/authentication/saml_response.xml')
             .gsub('urn:oasis:names:tc:SAML:2.0:ac:classes:Password', 'urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorIGTOKEN')
+        end
+
+        it 'expects user to be signed_in' do
+          expect(request.env['warden']).to be_authenticated
+        end
       end
 
-      it 'expects user to be signed_in' do
-        expect(request.env['warden']).to be_authenticated
+      context 'when not worth two factors' do
+        it 'expects user to provide second factor' do
+          expect(response).to render_template('devise/sessions/two_factor')
+          expect(request.env['warden']).not_to be_authenticated
+        end
       end
     end
 
-    context 'when not worth two factors' do
-      it 'expects user to provide second factor' do
-        expect(response).to render_template('devise/sessions/two_factor')
-        expect(request.env['warden']).not_to be_authenticated
+    context 'with IdP initiated request' do
+      let(:user) { create(:user) }
+      let(:last_request_id) { '99999' }
+
+      before do
+        sign_in user
+      end
+
+      it 'lets the user know their account isn\'t linked yet' do
+        post :saml, params: { SAMLResponse: mock_saml_response }
+
+        expect(flash[:notice]).to eq 'Request to link SAML account must be authorized'
+      end
+
+      it 'redirects to profile account page' do
+        post :saml, params: { SAMLResponse: mock_saml_response }
+
+        expect(response).to redirect_to(profile_account_path)
+      end
+
+      it 'doesn\'t link a new identity to the user' do
+        expect { post :saml, params: { SAMLResponse: mock_saml_response } }.not_to change { user.identities.count }
       end
     end
   end

spec/lib/gitlab/auth/saml/identity_linker_spec.rb

@@ -6,45 +6,61 @@ describe Gitlab::Auth::Saml::IdentityLinker do
   let(:user) { create(:user) }
   let(:provider) { 'saml' }
   let(:uid) { user.email }
-  let(:oauth) { { 'provider' => provider, 'uid' => uid } }
+  let(:in_response_to) { '12345' }
+  let(:saml_response) { instance_double(OneLogin::RubySaml::Response, in_response_to: in_response_to) }
+  let(:session) { { 'last_authn_request_id' => in_response_to } }
 
-  subject { described_class.new(user, oauth) }
+  let(:oauth) do
+    OmniAuth::AuthHash.new(provider: provider, uid: uid, extra: { response_object: saml_response })
+  end
 
-  context 'linked identity exists' do
-    let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) }
+  subject { described_class.new(user, oauth, session) }
 
-    it "doesn't create new identity" do
-      expect { subject.link }.not_to change { Identity.count }
-    end
+  context 'with valid GitLab initiated request' do
+    context 'linked identity exists' do
+      let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) }
 
-    it "sets #changed? to false" do
-      subject.link
+      it "doesn't create new identity" do
+        expect { subject.link }.not_to change { Identity.count }
+      end
 
-      expect(subject).not_to be_changed
-    end
-  end
+      it "sets #changed? to false" do
+        subject.link
 
-  context 'identity needs to be created' do
-    it 'creates linked identity' do
-      expect { subject.link }.to change { user.identities.count }
+        expect(subject).not_to be_changed
+      end
     end
 
-    it 'sets identity provider' do
-      subject.link
+    context 'identity needs to be created' do
+      it 'creates linked identity' do
+        expect { subject.link }.to change { user.identities.count }
+      end
 
-      expect(user.identities.last.provider).to eq provider
-    end
+      it 'sets identity provider' do
+        subject.link
 
-    it 'sets identity extern_uid' do
-      subject.link
+        expect(user.identities.last.provider).to eq provider
+      end
 
-      expect(user.identities.last.extern_uid).to eq uid
+      it 'sets identity extern_uid' do
+        subject.link
+
+        expect(user.identities.last.extern_uid).to eq uid
+      end
+
+      it 'sets #changed? to true' do
+        subject.link
+
+        expect(subject).to be_changed
+      end
     end
+  end
 
-    it 'sets #changed? to true' do
-      subject.link
+  context 'with identity provider initiated request' do
+    let(:session) { { 'last_authn_request_id' => nil } }
 
-      expect(subject).to be_changed
+    it 'attempting to link accounts raises an exception' do
+      expect { subject.link }.to raise_error(Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest)
     end
   end
 end

ee/spec/lib/gitlab/auth/saml_origin_validator_spec.rb → spec/lib/gitlab/auth/saml/origin_validator_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::Auth::SamlOriginValidator do
+describe Gitlab::Auth::Saml::OriginValidator do
   let(:session) { instance_double(ActionDispatch::Request::Session) }
 
   subject { described_class.new(session) }

spec/lib/omni_auth/strategies/saml_spec.rb

+require 'spec_helper'
+
+describe OmniAuth::Strategies::SAML, type: :strategy do
+  let(:idp_sso_target_url) { 'https://login.example.com/idp' }
+  let(:strategy) { [OmniAuth::Strategies::SAML, { idp_sso_target_url: idp_sso_target_url }] }
+
+  describe 'POST /users/auth/saml' do
+    it 'redirects to the provider login page' do
+      post '/users/auth/saml'
+
+      expect(last_response).to redirect_to(/\A#{Regexp.quote(idp_sso_target_url)}/)
+    end
+
+    it 'stores request ID during request phase' do
+      request_id = double
+      allow_any_instance_of(OneLogin::RubySaml::Authrequest).to receive(:uuid).and_return(request_id)
+
+      post '/users/auth/saml'
+      expect(session['last_authn_request_id']).to eq(request_id)
+    end
+  end
+end

ee/spec/support/omniauth_strategy.rb → spec/support/omniauth_strategy.rb

@@ -6,7 +6,7 @@ module StrategyHelpers
 
   def post(*args)
     super.tap do
-      @response = ActionDispatch::TestResponse.from_response(last_response) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      @response = ActionDispatch::TestResponse.from_response(last_response)
     end
   end