Merge branch 'jej/group-saml-link-origin-verification' into 'master'

Ensure request to link GroupSAML acount was GitLab initiated

See merge request gitlab/gitlab-ee!746

ee/app/controllers/groups/omniauth_callbacks_controller.rb

@@ -9,9 +9,11 @@ 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)
+    identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(current_user, oauth, @saml_provider, session)
 
     omniauth_flow(Gitlab::Auth::GroupSaml, identity_linker: identity_linker)
+  rescue Gitlab::Auth::GroupSaml::IdentityLinker::UnverifiedRequest
+    redirect_unverified_saml_initiation
   end
 
   private
@@ -44,6 +46,12 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
     super
   end
 
+  def redirect_unverified_saml_initiation
+    flash[:notice] = "Request to link SAML account must be authorized"
+
+    redirect_to sso_group_saml_providers_path(@unauthenticated_group)
+  end
+
   override :after_sign_in_path_for
   def after_sign_in_path_for(resource)
     saml_redirect_path || super

ee/changelogs/unreleased/jej-group-saml-link-origin-verification.yml

+---
+title: Prevent Group SAML authorizing sign in without prior user approval
+merge_request:
+author:
+type: security

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

@@ -4,15 +4,20 @@ module Gitlab
   module Auth
     module GroupSaml
       class IdentityLinker < Gitlab::Auth::Saml::IdentityLinker
-        attr_reader :saml_provider
+        attr_reader :saml_provider, :session
 
-        def initialize(current_user, oauth, saml_provider)
+        UnverifiedRequest = Class.new(StandardError)
+
+        def initialize(current_user, oauth, saml_provider, session)
           super(current_user, oauth)
 
           @saml_provider = saml_provider
+          @session = session
         end
 
         def link
+          raise_unless_request_is_gitlab_initiated! if unlinked?
+
           super
 
           update_group_membership unless failed?
@@ -32,6 +37,18 @@ 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

@@ -36,6 +36,20 @@ 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 self.invalid_group!(path)
         raise ActionController::RoutingError, path
       end
@@ -54,6 +68,10 @@ module OmniAuth
         Feature.enabled?(:group_saml_metadata_available, group_lookup.group)
       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/controllers/groups/omniauth_callbacks_controller_spec.rb

@@ -9,6 +9,9 @@ describe Groups::OmniauthCallbacksController do
   let(:provider) { :group_saml }
   let(:group) { create(:group, :private) }
   let!(:saml_provider) { create(:saml_provider, group: group) }
+  let(:in_response_to) { '12345' }
+  let(:last_request_id) { in_response_to }
+  let(:saml_response) { instance_double(OneLogin::RubySaml::Response, in_response_to: in_response_to) }
 
   before do
     stub_licensed_features(group_saml: true)
@@ -22,6 +25,10 @@ describe Groups::OmniauthCallbacksController do
     create(:omniauth_user, extern_uid: uid, provider: provider, saml_provider: saml_provider)
   end
 
+  def stub_last_request_id(id)
+    session["last_authn_request_id"] = id
+  end
+
   context "when request hasn't been validated by omniauth middleware" do
     it "prevents authentication" do
       sign_in(user)
@@ -34,8 +41,9 @@ describe Groups::OmniauthCallbacksController do
 
   context "valid credentials" do
     before do
-      mock_auth_hash(provider, uid, user.email)
+      mock_auth_hash(provider, uid, user.email, response_object: saml_response)
       stub_omniauth_provider(provider, context: request)
+      stub_last_request_id(last_request_id)
     end
 
     shared_examples "and identity already linked" do
@@ -104,6 +112,22 @@ describe Groups::OmniauthCallbacksController do
 
           expect(flash[:notice]).to match(/SAML for .* was added/)
         end
+
+        context 'with IdP initiated request' do
+          let(:last_request_id) { '99999' }
+
+          it 'redirects to account link page' do
+            post provider, params: { group_id: group }
+
+            expect(response).to redirect_to(sso_group_saml_providers_path(group))
+          end
+
+          it "lets the user know their account isn't linked yet" do
+            post provider, params: { group_id: group }
+
+            expect(flash[:notice]).to eq 'Request to link SAML account must be authorized'
+          end
+        end
       end
     end
 

ee/spec/features/profiles/account_spec.rb

@@ -20,7 +20,9 @@ describe 'Profile > Account' do
 
     def create_linked_identity
       oauth = { 'provider' => 'group_saml', 'uid' => '1' }
-      Gitlab::Auth::GroupSaml::IdentityLinker.new(user, oauth, saml_provider).link
+      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.link
     end
 
     before do

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

@@ -4,10 +4,13 @@ describe Gitlab::Auth::GroupSaml::IdentityLinker do
   let(:user) { create(:user) }
   let(:provider) { 'group_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(:oauth) { OmniAuth::AuthHash.new(provider: provider, uid: uid, extra: { response_object: saml_response }) }
   let(:saml_provider) { create(:saml_provider) }
+  let(:session) { {} }
 
-  subject { described_class.new(user, oauth, saml_provider) }
+  subject { described_class.new(user, oauth, saml_provider, session) }
 
   context 'linked identity exists' do
     let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid, saml_provider: saml_provider) }
@@ -30,38 +33,48 @@ describe Gitlab::Auth::GroupSaml::IdentityLinker do
   end
 
   context 'identity needs to be created' do
-    it 'creates linked identity' do
-      expect { subject.link }.to change { user.identities.count }
+    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)
+      end
     end
 
-    it 'sets identity provider' do
-      subject.link
+    context 'with valid gitlab initiated request' do
+      let(:session) { { 'last_authn_request_id' => in_response_to } }
 
-      expect(user.identities.last.provider).to eq provider
-    end
+      it 'creates linked identity' do
+        expect { subject.link }.to change { user.identities.count }
+      end
 
-    it 'sets saml provider' do
-      subject.link
+      it 'sets identity provider' do
+        subject.link
 
-      expect(user.identities.last.saml_provider).to eq saml_provider
-    end
+        expect(user.identities.last.provider).to eq provider
+      end
 
-    it 'sets identity extern_uid' do
-      subject.link
+      it 'sets saml provider' do
+        subject.link
 
-      expect(user.identities.last.extern_uid).to eq uid
-    end
+        expect(user.identities.last.saml_provider).to eq saml_provider
+      end
 
-    it 'sets #changed? to true' do
-      subject.link
+      it 'sets identity extern_uid' do
+        subject.link
 
-      expect(subject).to be_changed
-    end
+        expect(user.identities.last.extern_uid).to eq uid
+      end
 
-    it 'adds user to group' do
-      subject.link
+      it 'sets #changed? to true' do
+        subject.link
 
-      expect(saml_provider.group.member?(user)).to eq(true)
+        expect(subject).to be_changed
+      end
+
+      it 'adds user to group' do
+        subject.link
+
+        expect(saml_provider.group.member?(user)).to eq(true)
+      end
     end
   end
 end

ee/spec/lib/gitlab/auth/saml_origin_validator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::SamlOriginValidator do
+  let(:session) { instance_double(ActionDispatch::Request::Session) }
+
+  subject { described_class.new(session) }
+
+  describe '#store_origin' do
+    it 'stores the SAML request ID' do
+      request_id = double
+      authn_request = instance_double(OneLogin::RubySaml::Authrequest, uuid: request_id)
+
+      expect(session).to receive(:[]=).with('last_authn_request_id', request_id)
+
+      subject.store_origin(authn_request)
+    end
+  end
+
+  describe '#gitlab_initiated?' do
+    it 'returns false if InResponseTo is not present' do
+      saml_response = instance_double(OneLogin::RubySaml::Response, in_response_to: nil)
+
+      expect(subject.gitlab_initiated?(saml_response)).to eq(false)
+    end
+
+    it 'returns false if InResponseTo does not match stored value' do
+      saml_response = instance_double(OneLogin::RubySaml::Response, in_response_to: "abc")
+      allow(session).to receive(:[]).with('last_authn_request_id').and_return('123')
+
+      expect(subject.gitlab_initiated?(saml_response)).to eq(false)
+    end
+
+    it 'returns true if InResponseTo matches stored value' do
+      saml_response = instance_double(OneLogin::RubySaml::Response, in_response_to: "123")
+      allow(session).to receive(:[]).with('last_authn_request_id').and_return('123')
+
+      expect(subject.gitlab_initiated?(saml_response)).to eq(true)
+    end
+  end
+end

ee/spec/lib/omni_auth/strategies/group_saml_spec.rb

@@ -110,6 +110,15 @@ describe OmniAuth::Strategies::GroupSaml, type: :strategy do
         post '/users/auth/group_saml'
       end.to raise_error(ActionController::RoutingError)
     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/group_saml', group_path: 'my-group'
+
+      expect(session['last_authn_request_id']).to eq(request_id)
+    end
   end
 
   describe 'POST /users/auth/group_saml/metadata' do

lib/gitlab/auth/omniauth_identity_linker_base.rb

@@ -12,7 +12,7 @@ module Gitlab
       end
 
       def link
-        save if identity.new_record?
+        save if unlinked?
       end
 
       def changed?
@@ -35,6 +35,10 @@ module Gitlab
         @changed = identity.save
       end
 
+      def unlinked?
+        identity.new_record?
+      end
+
       # rubocop: disable CodeReuse/ActiveRecord
       def identity
         @identity ||= current_user.identities

spec/controllers/omniauth_callbacks_controller_spec.rb

@@ -193,7 +193,7 @@ describe OmniauthCallbacksController, type: :controller do
     before do
       stub_omniauth_saml_config({ enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'],
                                   providers: [saml_config] })
-      mock_auth_hash('saml', 'my-uid', user.email, mock_saml_response)
+      mock_auth_hash_with_saml_xml('saml', 'my-uid', user.email, mock_saml_response)
       request.env["devise.mapping"] = Devise.mappings[:user]
       request.env['omniauth.auth'] = Rails.application.env_config['omniauth.auth']
       post :saml, params: { SAMLResponse: mock_saml_response }

spec/support/helpers/login_helpers.rb

@@ -47,7 +47,7 @@ module LoginHelpers
   end
 
   def gitlab_sign_in_via(provider, user, uid, saml_response = nil)
-    mock_auth_hash(provider, uid, user.email, saml_response)
+    mock_auth_hash_with_saml_xml(provider, uid, user.email, saml_response)
     visit new_user_session_path
     click_link provider
   end
@@ -87,7 +87,12 @@ module LoginHelpers
     click_link "oauth-login-#{provider}"
   end
 
-  def mock_auth_hash(provider, uid, email, saml_response = nil)
+  def mock_auth_hash_with_saml_xml(provider, uid, email, saml_response)
+    response_object = { document: saml_xml(saml_response) }
+    mock_auth_hash(provider, uid, email, response_object: response_object)
+  end
+
+  def mock_auth_hash(provider, uid, email, response_object: nil)
     # The mock_auth configuration allows you to set per-provider (or default)
     # authentication hashes to return during integration testing.
     OmniAuth.config.mock_auth[provider.to_sym] = OmniAuth::AuthHash.new({
@@ -110,9 +115,7 @@ module LoginHelpers
             image: 'mock_user_thumbnail_url'
           }
         },
-        response_object: {
-          document: saml_xml(saml_response)
-        }
+        response_object: response_object
       }
     })
     Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider.to_sym]