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
parent 2cacd021
...@@ -40,6 +40,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -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
...@@ -84,8 +86,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -84,8 +86,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)
...@@ -178,6 +179,10 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -178,6 +179,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 }
......
---
title: Prevent GitLab accounts takeover if SAML is configured
merge_request:
author:
type: security
...@@ -3,12 +3,13 @@ ...@@ -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 ...@@ -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
......
# 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
# 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
...@@ -13029,6 +13029,9 @@ msgstr "" ...@@ -13029,6 +13029,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 ""
......
...@@ -219,34 +219,70 @@ describe OmniauthCallbacksController, type: :controller do ...@@ -219,34 +219,70 @@ 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']
post :saml, params: { SAMLResponse: mock_saml_response }
end end
context 'when worth two factors' do context 'with GitLab initiated request' do
let(:mock_saml_response) do before do
File.read('spec/fixtures/authentication/saml_response.xml') 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') .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 end
it 'expects user to be signed_in' do context 'when not worth two factors' do
expect(request.env['warden']).to be_authenticated 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
end end
context 'when not worth two factors' do context 'with IdP initiated request' do
it 'expects user to provide second factor' do let(:user) { create(:user) }
expect(response).to render_template('devise/sessions/two_factor') let(:last_request_id) { '99999' }
expect(request.env['warden']).not_to be_authenticated
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 end
......
...@@ -6,45 +6,61 @@ describe Gitlab::Auth::Saml::IdentityLinker do ...@@ -6,45 +6,61 @@ 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
context 'linked identity exists' do subject { described_class.new(user, oauth, session) }
let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) }
it "doesn't create new identity" do context 'with valid GitLab initiated request' do
expect { subject.link }.not_to change { Identity.count } context 'linked identity exists' do
end let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) }
it "sets #changed? to false" do it "doesn't create new identity" do
subject.link expect { subject.link }.not_to change { Identity.count }
end
expect(subject).not_to be_changed it "sets #changed? to false" do
end subject.link
end
context 'identity needs to be created' do expect(subject).not_to be_changed
it 'creates linked identity' do end
expect { subject.link }.to change { user.identities.count }
end end
it 'sets identity provider' do context 'identity needs to be created' do
subject.link it 'creates linked identity' do
expect { subject.link }.to change { user.identities.count }
end
expect(user.identities.last.provider).to eq provider it 'sets identity provider' do
end subject.link
it 'sets identity extern_uid' do expect(user.identities.last.provider).to eq provider
subject.link 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
end
it 'sets #changed? to true' do context 'with identity provider initiated request' do
subject.link 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 end
end end
# 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
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
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment