Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/omniauth_callbacks_controller.rb9
-rw-r--r--changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml5
-rw-r--r--lib/gitlab/auth/omniauth_identity_linker_base.rb5
-rw-r--r--lib/gitlab/auth/saml/identity_linker.rb24
-rw-r--r--lib/gitlab/auth/saml/origin_validator.rb41
-rw-r--r--lib/omni_auth/strategies/saml.rb29
-rw-r--r--locale/gitlab.pot3
-rw-r--r--spec/controllers/omniauth_callbacks_controller_spec.rb58
-rw-r--r--spec/lib/gitlab/auth/saml/identity_linker_spec.rb66
-rw-r--r--spec/lib/gitlab/auth/saml/origin_validator_spec.rb42
-rw-r--r--spec/lib/omni_auth/strategies/saml_spec.rb22
-rw-r--r--spec/support/omniauth_strategy.rb39
12 files changed, 303 insertions, 40 deletions
diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index ee2cf47d5cb..755ce3463c4 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/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 }
diff --git a/changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml b/changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml
new file mode 100644
index 00000000000..9022bc8a26f
--- /dev/null
+++ b/changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent GitLab accounts takeover if SAML is configured
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/auth/omniauth_identity_linker_base.rb b/lib/gitlab/auth/omniauth_identity_linker_base.rb
index c620fc5d6bd..a6c247f31a7 100644
--- a/lib/gitlab/auth/omniauth_identity_linker_base.rb
+++ b/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
diff --git a/lib/gitlab/auth/saml/identity_linker.rb b/lib/gitlab/auth/saml/identity_linker.rb
index ae0d6dded4e..93195c3189f 100644
--- a/lib/gitlab/auth/saml/identity_linker.rb
+++ b/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
diff --git a/lib/gitlab/auth/saml/origin_validator.rb b/lib/gitlab/auth/saml/origin_validator.rb
new file mode 100644
index 00000000000..4ecc688888f
--- /dev/null
+++ b/lib/gitlab/auth/saml/origin_validator.rb
@@ -0,0 +1,41 @@
+# 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
diff --git a/lib/omni_auth/strategies/saml.rb b/lib/omni_auth/strategies/saml.rb
new file mode 100644
index 00000000000..ebe062f17e0
--- /dev/null
+++ b/lib/omni_auth/strategies/saml.rb
@@ -0,0 +1,29 @@
+# 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
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 751fc4c85ce..46b45f5a180 100644
--- a/locale/gitlab.pot
+++ b/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 ""
diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb
index 5536b7df64e..9371c434f49 100644
--- a/spec/controllers/omniauth_callbacks_controller_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb b/spec/lib/gitlab/auth/saml/identity_linker_spec.rb
index 89118ff05ba..7912c8fb4b1 100644
--- a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/auth/saml/origin_validator_spec.rb b/spec/lib/gitlab/auth/saml/origin_validator_spec.rb
new file mode 100644
index 00000000000..ae120b328ab
--- /dev/null
+++ b/spec/lib/gitlab/auth/saml/origin_validator_spec.rb
@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::Saml::OriginValidator 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
diff --git a/spec/lib/omni_auth/strategies/saml_spec.rb b/spec/lib/omni_auth/strategies/saml_spec.rb
new file mode 100644
index 00000000000..3c59de86d98
--- /dev/null
+++ b/spec/lib/omni_auth/strategies/saml_spec.rb
@@ -0,0 +1,22 @@
+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
diff --git a/spec/support/omniauth_strategy.rb b/spec/support/omniauth_strategy.rb
new file mode 100644
index 00000000000..eefa04bd9dd
--- /dev/null
+++ b/spec/support/omniauth_strategy.rb
@@ -0,0 +1,39 @@
+module StrategyHelpers
+ include Rack::Test::Methods
+ include ActionDispatch::Assertions::ResponseAssertions
+ include Shoulda::Matchers::ActionController
+ include OmniAuth::Test::StrategyTestCase
+
+ def post(*args)
+ super.tap do
+ @response = ActionDispatch::TestResponse.from_response(last_response)
+ end
+ end
+
+ def auth_hash
+ last_request.env['omniauth.auth']
+ end
+
+ def self.without_test_mode
+ original_mode = OmniAuth.config.test_mode
+ original_on_failure = OmniAuth.config.on_failure
+
+ OmniAuth.config.test_mode = false
+ OmniAuth.config.on_failure = OmniAuth::FailureEndpoint
+
+ yield
+ ensure
+ OmniAuth.config.test_mode = original_mode
+ OmniAuth.config.on_failure = original_on_failure
+ end
+end
+
+RSpec.configure do |config|
+ config.include StrategyHelpers, type: :strategy
+
+ config.around(:all, type: :strategy) do |example|
+ StrategyHelpers.without_test_mode do
+ example.run
+ end
+ end
+end
generated by cgit v1.2.3 (git 2.25.1) at 2024-08-19 02:43:02 +0300