Validate that SAML requests are originated from gitlab

If the request wasn't initiated by gitlab we shouldn't add the new
identity to the user, and instead show that we weren't able to link
the identity to the user.

This should fix: https://gitlab.com/gitlab-org/gitlab-ce/issues/56509

And reuse SAML logic within EE SAMLGroup, this is to avoid duplication
between the code used for CE and EE.

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

@@ -12,7 +12,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) }
@@ -37,7 +37,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

@@ -13242,6 +13242,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

@@ -8,7 +8,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