summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSebastian Arcila Valenzuela <sarcila@gitlab.com>2019-08-19 15:19:19 +0200
committerSebastian Arcila Valenzuela <sarcila@gitlab.com>2019-09-16 10:55:07 +0200
commit21338fd70cc1e3e07a7a88f4546899522fb28385 (patch)
tree8cfcb640c4875330a53818157380523f597d9d9f
parent0dbd7139caefb94eac3cc51beac53453663b04c2 (diff)
downloadgitlab-ce-21338fd70cc1e3e07a7a88f4546899522fb28385.tar.gz
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
-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 2a8dd997d04..9a131875bc3 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
@@ -84,8 +86,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)
@@ -178,6 +179,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 f9c15929c7c..53ac629237f 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -8967,6 +8967,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 6e374a8daa7..f41cd55ce8c 100644
--- a/spec/controllers/omniauth_callbacks_controller_spec.rb
+++ b/spec/controllers/omniauth_callbacks_controller_spec.rb
@@ -219,34 +219,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 f3305d574cc..6c4db25a02f 100644
--- a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb
+++ b/spec/lib/gitlab/auth/saml/identity_linker_spec.rb
@@ -4,45 +4,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