Commit ce0e54f3 authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

Merge branch 'security-sarcila-verify-saml-request-origin-12-2' into '12-2-stable'

Check that SAML identity linking validates the origin of the request

See merge request gitlab/gitlabhq!3377
parents 565e9b22 71636c27
......@@ -40,6 +40,8 @@ def failure_message
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 @@ def omniauth_flow(auth_module, identity_linker: nil)
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 @@ def fail_login_with_message(message)
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 }
......
---
title: Prevent GitLab accounts takeover if SAML is configured
merge_request:
author:
type: security
......@@ -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
......
......@@ -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
......
# 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
......@@ -9528,6 +9528,9 @@ msgstr ""
msgid "Request Access"
msgstr ""
msgid "Request to link SAML account must be authorized"
msgstr ""
msgid "Requested %{time_ago}"
msgstr ""
......
......@@ -247,34 +247,70 @@ def stub_route_as(path)
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
......
......@@ -6,45 +6,61 @@
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
# 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