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
This commit is contained in:
parent
010e3c5ed4
commit
3692e9f8a2
|
@ -40,6 +40,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
|
||||||
|
|
||||||
def saml
|
def saml
|
||||||
omniauth_flow(Gitlab::Auth::Saml)
|
omniauth_flow(Gitlab::Auth::Saml)
|
||||||
|
rescue Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest
|
||||||
|
redirect_unverified_saml_initiation
|
||||||
end
|
end
|
||||||
|
|
||||||
def omniauth_error
|
def omniauth_error
|
||||||
|
@ -92,8 +94,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
|
||||||
return render_403 unless link_provider_allowed?(oauth['provider'])
|
return render_403 unless link_provider_allowed?(oauth['provider'])
|
||||||
|
|
||||||
log_audit_event(current_user, with: oauth['provider'])
|
log_audit_event(current_user, with: oauth['provider'])
|
||||||
|
identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth, session)
|
||||||
identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth)
|
|
||||||
|
|
||||||
link_identity(identity_linker)
|
link_identity(identity_linker)
|
||||||
|
|
||||||
|
@ -194,6 +195,10 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
|
||||||
redirect_to new_user_session_path
|
redirect_to new_user_session_path
|
||||||
end
|
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
|
def handle_disabled_provider
|
||||||
label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider'])
|
label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider'])
|
||||||
flash[:alert] = _("Signing in using %{label} has been disabled") % { label: label }
|
flash[:alert] = _("Signing in using %{label} has been disabled") % { label: label }
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Prevent GitLab accounts takeover if SAML is configured
|
||||||
|
merge_request:
|
||||||
|
author:
|
||||||
|
type: security
|
|
@ -3,12 +3,13 @@
|
||||||
module Gitlab
|
module Gitlab
|
||||||
module Auth
|
module Auth
|
||||||
class OmniauthIdentityLinkerBase
|
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
|
@current_user = current_user
|
||||||
@oauth = oauth
|
@oauth = oauth
|
||||||
@changed = false
|
@changed = false
|
||||||
|
@session = session
|
||||||
end
|
end
|
||||||
|
|
||||||
def link
|
def link
|
||||||
|
|
|
@ -4,6 +4,30 @@ module Gitlab
|
||||||
module Auth
|
module Auth
|
||||||
module Saml
|
module Saml
|
||||||
class IdentityLinker < OmniauthIdentityLinkerBase
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -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
|
|
@ -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
|
|
@ -13242,6 +13242,9 @@ msgstr ""
|
||||||
msgid "Request Access"
|
msgid "Request Access"
|
||||||
msgstr ""
|
msgstr ""
|
||||||
|
|
||||||
|
msgid "Request to link SAML account must be authorized"
|
||||||
|
msgstr ""
|
||||||
|
|
||||||
msgid "Requested %{time_ago}"
|
msgid "Requested %{time_ago}"
|
||||||
msgstr ""
|
msgstr ""
|
||||||
|
|
||||||
|
|
|
@ -247,16 +247,26 @@ describe OmniauthCallbacksController, type: :controller do
|
||||||
end
|
end
|
||||||
|
|
||||||
describe '#saml' do
|
describe '#saml' do
|
||||||
|
let(:last_request_id) { 'ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685' }
|
||||||
let(:user) { create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') }
|
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(:mock_saml_response) { File.read('spec/fixtures/authentication/saml_response.xml') }
|
||||||
let(:saml_config) { mock_saml_config_with_upstream_two_factor_authn_contexts }
|
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
|
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'],
|
stub_omniauth_saml_config({ enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'],
|
||||||
providers: [saml_config] })
|
providers: [saml_config] })
|
||||||
mock_auth_hash_with_saml_xml('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['devise.mapping'] = Devise.mappings[:user]
|
||||||
request.env['omniauth.auth'] = Rails.application.env_config['omniauth.auth']
|
request.env['omniauth.auth'] = Rails.application.env_config['omniauth.auth']
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with GitLab initiated request' do
|
||||||
|
before do
|
||||||
post :saml, params: { SAMLResponse: mock_saml_response }
|
post :saml, params: { SAMLResponse: mock_saml_response }
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -278,4 +288,30 @@ describe OmniauthCallbacksController, type: :controller do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
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
|
||||||
end
|
end
|
||||||
|
|
|
@ -6,10 +6,17 @@ describe Gitlab::Auth::Saml::IdentityLinker do
|
||||||
let(:user) { create(:user) }
|
let(:user) { create(:user) }
|
||||||
let(:provider) { 'saml' }
|
let(:provider) { 'saml' }
|
||||||
let(:uid) { user.email }
|
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
|
||||||
|
|
||||||
|
subject { described_class.new(user, oauth, session) }
|
||||||
|
|
||||||
|
context 'with valid GitLab initiated request' do
|
||||||
context 'linked identity exists' do
|
context 'linked identity exists' do
|
||||||
let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) }
|
let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) }
|
||||||
|
|
||||||
|
@ -48,3 +55,12 @@ describe Gitlab::Auth::Saml::IdentityLinker do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context 'with identity provider initiated request' do
|
||||||
|
let(:session) { { 'last_authn_request_id' => nil } }
|
||||||
|
|
||||||
|
it 'attempting to link accounts raises an exception' do
|
||||||
|
expect { subject.link }.to raise_error(Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
|
@ -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
|
|
@ -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
|
|
@ -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
|
Loading…
Reference in New Issue