From 10a5107e4b7ec2abad70c85e14adff05238ed543 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Sat, 6 Sep 2025 17:00:11 +0200 Subject: [PATCH 1/4] Step-up auth: Add OIDC backend support for namespace scope [1/3] This MR introduces the foundational OIDC backend changes needed to support step-up authentication at the namespace/group level. It extends the existing OIDC step-up authentication classes to handle a new 'namespace' scope alongside the existing 'admin_mode' scope. Changes: - Extended OIDC step-up authentication flow to support namespace scope - Updated OAuth callback handling to process namespace step-up auth - Added namespace scope validation and session management - Updated tests to cover namespace scope scenarios Part 1 of 3 in splitting the step-up authentication feature for groups. This provides the backend foundation without UI or enforcement logic. Changelog: added --- .../omniauth_callbacks_controller.rb | 15 +- .../oidc/step_up_auth_before_request_phase.rb | 34 +++- .../omniauth_callbacks_controller_spec.rb | 154 +++++++++++++++++- .../step_up_auth_before_request_phase_spec.rb | 84 +++++++++- .../oidc/step_up_authentication_flow_spec.rb | 11 ++ .../auth/oidc/step_up_authentication_spec.rb | 8 +- 6 files changed, 283 insertions(+), 23 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index fc06c348ad8e4e..3a347e2c0d3fdc 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -426,10 +426,21 @@ def store_redirect_fragment(redirect_fragment) end def handle_step_up_auth - return if Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user) + handle_step_up_auth_for_admin_mode if Feature.enabled?(:omniauth_step_up_auth_for_admin_mode, current_user) + handle_step_up_auth_for_namespace if Feature.enabled?(:omniauth_step_up_auth_for_namespace, current_user) + end + + def handle_step_up_auth_for_admin_mode + handle_step_up_auth_for(::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_ADMIN_MODE) + end + + def handle_step_up_auth_for_namespace + handle_step_up_auth_for(::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_NAMESPACE) + end + def handle_step_up_auth_for(scope) step_up_auth_flow = - ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow(session: session, provider: oauth.provider) + ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow(session: session, provider: oauth.provider, scope: scope) return unless step_up_auth_flow.enabled_by_config? return unless step_up_auth_flow.requested? diff --git a/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb b/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb index 2b75a371fc241f..917d90b8f86281 100644 --- a/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb +++ b/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb @@ -11,16 +11,30 @@ module StepUpAuthBeforeRequestPhase class << self def call(env) return if current_user_from(env).blank? - return if Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user_from(env)) - # If the step-up authentication scope is not included in the request params, - # then step-up authentication is likely not requested and we do not need to proceed. - return unless step_up_auth_requested_for_admin_mode?(env) + return {} if requested_step_up_auth_scope_from(env).blank? + + return {} if requested_step_up_auth_scope_from(env).to_sym == :admin_mode && + Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user_from(env)) + + return {} if requested_step_up_auth_scope_from(env).to_sym == :namespace && + Feature.disabled?(:omniauth_step_up_auth_for_namespace, current_user_from(env)) + + return unless allowed_step_up_auth_scope_from(env) session = session_from(env) + provider = current_provider_from(env) step_up_auth_flow = - ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow(session: session, provider: provider) + ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow( + session: session, + provider: provider, + scope: requested_step_up_auth_scope_from(env) + ) + + # TODO: Integrate the state uuid in the step-up auth session. + # Why? At the moment, there is a small security vulnerability + # where simultaneous authentication requests could lead to privilege escalation, https://gitlab.com/gitlab-org/gitlab/-/issues/555349. return unless step_up_auth_flow.enabled_by_config? @@ -30,9 +44,9 @@ def call(env) private - def step_up_auth_requested_for_admin_mode?(env) - request_param_step_up_auth_scope_from(env) == - ::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_ADMIN_MODE.to_s + def allowed_step_up_auth_scope_from(env) + ::Gitlab::Auth::Oidc::StepUpAuthentication::ALLOWED_SCOPES + .include?(request_param_step_up_auth_scope_from(env).to_sym) end def current_user_from(env) @@ -43,6 +57,10 @@ def current_provider_from(env) env['omniauth.strategy']&.name end + def requested_step_up_auth_scope_from(env) + request_param_step_up_auth_scope_from(env) + end + def request_param_step_up_auth_scope_from(env) env.dig('rack.request.query_hash', 'step_up_auth_scope').to_s end diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index b192e54b67237a..4ce2792951913c 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -926,7 +926,159 @@ it 'does not add session key "step_up_auth"' do get provider - expect(session).not_to include 'step_up_auth' + expect(session).not_to include 'omniauth_step_up_auth' + end + end + + context 'for namespace scope' do + using RSpec::Parameterized::TableSyntax + + let(:ommiauth_provider_config_with_namespace_step_up_auth) do + GitlabSettings::Options.new( + name: "openid_connect", + step_up_auth: { + namespace: { + id_token: { + required: required_id_token_claims, + included: included_id_token_claims + } + } + } + ) + end + + before do + mock_auth_hash(provider, extern_uid, user.email, additional_info: { extra: { raw_info: mock_auth_hash_extra_raw_info } }) + + request.env['omniauth.auth'] = Rails.application.env_config['omniauth.auth'] + session['omniauth_step_up_auth'] = { 'openid_connect' => { 'namespace' => { 'state' => 'requested' } } } + + stub_omniauth_setting(enabled: true, auto_link_user: true, block_auto_created_users: false, providers: [ommiauth_provider_config_with_namespace_step_up_auth]) + end + + where(:required_id_token_claims, :included_id_token_claims, :mock_auth_hash_extra_raw_info, :step_up_auth_authenticated) do + { claim_1: 'silver' } | nil | { claim_1: 'silver' } | 'succeeded' + { claim_1: 'silver' } | nil | { claim_1: 'gold' } | 'failed' + nil | { claim_2: %w[mfa fpt] } | { claim_2: 'mfa' } | 'succeeded' + { claim_1: 'silver' } | { claim_2: %w[mfa fpt] } | { claim_1: 'silver', claim_2: 'mfa' } | 'succeeded' + { claim_1: 'silver' } | { claim_2: %w[mfa fpt] } | { claim_1: 'silver', claim_2: 'other_amr' } | 'failed' + end + + with_them do + context 'when user is signed in' do + before do + sign_in user + end + + it 'evaluates step-up authentication conditions for namespace scope' do + get provider + + expect(session.to_h.dig('omniauth_step_up_auth', 'openid_connect', 'namespace', 'state')) + .to eq step_up_auth_authenticated + end + + context 'when feature flag :omniauth_step_up_auth_for_namespace is disabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_namespace: false) + session.delete 'omniauth_step_up_auth' + end + + it 'does not store step-up authentication evaluation result in session' do + get provider + + expect(session).not_to include 'omniauth_step_up_auth' + end + end + end + end + end + + context 'with both admin_mode and namespace scopes configured' do + let(:ommiauth_provider_config_with_both_scopes) do + GitlabSettings::Options.new( + name: "openid_connect", + step_up_auth: { + admin_mode: { + id_token: { + required: { claim_1: 'gold' } + } + }, + namespace: { + id_token: { + required: { claim_1: 'silver' } + } + } + } + ) + end + + before do + mock_auth_hash(provider, extern_uid, user.email, additional_info: { extra: { raw_info: { claim_1: 'gold' } } }) + request.env['omniauth.auth'] = Rails.application.env_config['omniauth.auth'] + stub_omniauth_setting(enabled: true, auto_link_user: true, block_auto_created_users: false, providers: [ommiauth_provider_config_with_both_scopes]) + sign_in user + end + + context 'when both scopes are requested' do + before do + session['omniauth_step_up_auth'] = { + 'openid_connect' => { + 'admin_mode' => { 'state' => 'requested' }, + 'namespace' => { 'state' => 'requested' } + } + } + end + + it 'evaluates both admin_mode and namespace scopes' do + get provider + + expect(session.to_h.dig('omniauth_step_up_auth', 'openid_connect', 'admin_mode', 'state')) + .to eq 'succeeded' + expect(session.to_h.dig('omniauth_step_up_auth', 'openid_connect', 'namespace', 'state')) + .to eq 'failed' # gold claim doesn't meet silver requirement + end + end + + context 'when only admin_mode feature flag is enabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_namespace: false) + session['omniauth_step_up_auth'] = { + 'openid_connect' => { + 'admin_mode' => { 'state' => 'requested' }, + 'namespace' => { 'state' => 'requested' } + } + } + end + + it 'only evaluates admin_mode scope' do + get provider + + expect(session.to_h.dig('omniauth_step_up_auth', 'openid_connect', 'admin_mode', 'state')) + .to eq 'succeeded' + expect(session.to_h.dig('omniauth_step_up_auth', 'openid_connect', 'namespace', 'state')) + .to eq 'requested' # unchanged + end + end + + context 'when only namespace feature flag is enabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_admin_mode: false) + session['omniauth_step_up_auth'] = { + 'openid_connect' => { + 'admin_mode' => { 'state' => 'requested' }, + 'namespace' => { 'state' => 'requested' } + } + } + end + + it 'only evaluates namespace scope' do + get provider + + expect(session.to_h.dig('omniauth_step_up_auth', 'openid_connect', 'admin_mode', 'state')) + .to eq 'requested' # unchanged + expect(session.to_h.dig('omniauth_step_up_auth', 'openid_connect', 'namespace', 'state')) + .to eq 'failed' + end end end end diff --git a/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb b/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb index 56e41502a2ef95..7896c14fb440c7 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb @@ -19,6 +19,10 @@ { 'omniauth_step_up_auth' => { 'openid_connect' => { 'admin_mode' => { 'state' => 'requested' } } } } end + let(:session_with_namespace_step_up_state_requested) do + { 'omniauth_step_up_auth' => { 'openid_connect' => { 'namespace' => { 'state' => 'requested' } } } } + end + let(:provider_step_up_auth_name) { provider_step_up_auth.name } let(:provider_step_up_auth) do GitlabSettings::Options.new( @@ -30,6 +34,13 @@ acr: 'gold' } } + }, + namespace: { + id_token: { + required: { + acr: 'silver' + } + } } } ) @@ -44,16 +55,27 @@ # rubocop:disable Layout/LineLength -- Avoid formatting to keep one-line table syntax where(:provider, :step_up_auth_scope, :session, :expected_session) do + # admin_mode tests ref(:provider_step_up_auth_name) | 'admin_mode' | {} | ref(:session_with_step_up_state_requested) ref(:provider_step_up_auth_name) | 'admin_mode' | ref(:session_with_step_up_state_requested) | ref(:session_with_step_up_state_requested) ref(:provider_step_up_auth_name) | 'admin_mode' | { 'omniauth_step_up_auth' => { 'other_provider' => { 'admin_mode' => { 'state' => 'requested' } } } } | lazy { session.deep_merge(session_with_step_up_state_requested) } ref(:provider_step_up_auth_name) | 'admin_mode' | { 'other_session_key' => 'other_session_value' } | lazy { session.deep_merge(session_with_step_up_state_requested) } + + # namespace tests + ref(:provider_step_up_auth_name) | 'namespace' | {} | ref(:session_with_namespace_step_up_state_requested) + ref(:provider_step_up_auth_name) | 'namespace' | ref(:session_with_namespace_step_up_state_requested) | ref(:session_with_namespace_step_up_state_requested) + ref(:provider_step_up_auth_name) | 'namespace' | { 'omniauth_step_up_auth' => { 'other_provider' => { 'namespace' => { 'state' => 'requested' } } } } | lazy { session.deep_merge(session_with_namespace_step_up_state_requested) } + ref(:provider_step_up_auth_name) | 'namespace' | { 'other_session_key' => 'other_session_value' } | lazy { session.deep_merge(session_with_namespace_step_up_state_requested) } + + # invalid scope tests ref(:provider_step_up_auth_name) | 'other_scope' | {} | {} ref(:provider_step_up_auth_name) | 'other_scope' | ref(:session_with_step_up_state_requested) | ref(:session_with_step_up_state_requested) ref(:provider_step_up_auth_name) | '' | {} | {} ref(:provider_step_up_auth_name) | nil | {} | {} 'other_provider' | 'admin_mode' | {} | {} + 'other_provider' | 'namespace' | {} | {} nil | 'admin_mode' | {} | {} + nil | 'namespace' | {} | {} nil | nil | {} | {} end # rubocop:enable Layout/LineLength @@ -70,8 +92,32 @@ stub_feature_flags(omniauth_step_up_auth_for_admin_mode: false) end - it 'does not modify the session' do - expect { call_middleware }.not_to change { session } + it 'handles scopes correctly with admin_mode disabled' do + if step_up_auth_scope == 'admin_mode' + # Admin mode should not be processed when disabled + expect { call_middleware }.not_to change { session } + else + # Namespace and other scopes should still work + call_middleware + expect(session).to eq(expected_session) + end + end + end + + context 'when feature flag :omniauth_step_up_auth_for_namespace is disabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_namespace: false) + end + + it 'handles scopes correctly with namespace disabled' do + if step_up_auth_scope == 'namespace' + # Namespace should not be processed when disabled + expect { call_middleware }.not_to change { session } + else + # Admin mode and other scopes should still work + call_middleware + expect(session).to eq(expected_session) + end end end end @@ -79,20 +125,42 @@ context 'when user is not authenticated' do let(:env) { super().merge('warden' => instance_double(Warden::Proxy, user: nil)) } let(:provider) { provider_step_up_auth_name } - let(:step_up_auth_scope) { 'admin_mode' } - it 'does not modify the session' do - expect { call_middleware }.not_to change { session } + context 'with admin_mode scope' do + let(:step_up_auth_scope) { 'admin_mode' } + + it 'does not modify the session' do + expect { call_middleware }.not_to change { session } + end + end + + context 'with namespace scope' do + let(:step_up_auth_scope) { 'namespace' } + + it 'does not modify the session' do + expect { call_middleware }.not_to change { session } + end end end context 'when warden is blank' do let(:env) { super().merge('warden' => nil) } let(:provider) { provider_step_up_auth_name } - let(:step_up_auth_scope) { 'admin_mode' } - it 'does not modify the session' do - expect { call_middleware }.not_to change { session } + context 'with admin_mode scope' do + let(:step_up_auth_scope) { 'admin_mode' } + + it 'does not modify the session' do + expect { call_middleware }.not_to change { session } + end + end + + context 'with namespace scope' do + let(:step_up_auth_scope) { 'namespace' } + + it 'does not modify the session' do + expect { call_middleware }.not_to change { session } + end end end end diff --git a/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb b/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb index 2bcf234489710b..66f87af96583c9 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb @@ -9,6 +9,17 @@ subject(:flow) { described_class.new(session: session, provider: provider, scope: scope) } + describe '#initialize' do + context 'when scope is not in predefined allowed scopes' do + let(:scope) { 'custom_scope' } + + it 'allows initialization with any scope' do + expect { flow }.not_to raise_error + expect(flow.scope).to eq('custom_scope') + end + end + end + describe '#requested?, #succeeded?, #failed?' do using RSpec::Parameterized::TableSyntax diff --git a/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb b/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb index 9a734f610878d2..fc006c3a8b7e48 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb @@ -157,7 +157,7 @@ lazy { { 'openid_connect' => { 'admin_mode' => { 'state' => 'succeeded' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'succeeded' } } } } | true lazy { { 'openid_connect' => { 'admin_mode' => { 'state' => 'failed' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'failed' } } } } | false lazy { { 'openid_connect' => { 'admin_mode' => { 'state' => 'failed' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'succeeded' } } } } | false - lazy { { 'openid_connect' => { 'other_mode' => { 'state' => 'succeeded' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'failed' } } } } | false + lazy { { 'openid_connect' => { 'namespace' => { 'state' => 'succeeded' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'failed' } } } } | false nil | false {} | false @@ -177,7 +177,7 @@ 'omniauth_step_up_auth' => { 'openid_connect' => { 'admin_mode' => { 'state' => 'succeeded' }, - 'other_scope' => { 'state' => 'succeeded' } + 'namespace' => { 'state' => 'succeeded' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'failed' } @@ -199,7 +199,7 @@ disable_step_up_authentication! expect(session['omniauth_step_up_auth']['openid_connect']).not_to have_key('admin_mode') - expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('other_scope') + expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('namespace') expect(session['omniauth_step_up_auth']['other_provider']).not_to have_key('admin_mode') expect(described_class.succeeded?(session)).to be_falsey @@ -218,7 +218,7 @@ disable_step_up_authentication! expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('admin_mode') - expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('other_scope') + expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('namespace') expect(session['omniauth_step_up_auth']['other_provider']).to have_key('admin_mode') end end -- GitLab From 1adc50e9dd0777e786b4073bfe6d53f5f67ce4f9 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 24 Sep 2025 06:43:32 +0000 Subject: [PATCH 2/4] refactor: Apply suggestions from @mkaeppler - Add nested contexts in tests instead of if-else conditions, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/204102#note_2770293564 - Remove proxy method, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/204102#note_2770293546 - Extracting to local variable, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/204102#note_2770293541 - Shortening the constants, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/204102#note_2770293536 - Applying consistent return values, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/204102#note_2770293526 --- .../omniauth_callbacks_controller.rb | 4 +- app/helpers/groups_helper.rb | 2 +- app/models/namespace_setting.rb | 2 +- .../oidc/step_up_auth_before_request_phase.rb | 39 ++++++----- .../auth/oidc/step_up_authentication.rb | 28 ++++---- .../step_up_auth_before_request_phase_spec.rb | 66 ++++++++++++------- 6 files changed, 81 insertions(+), 60 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 3a347e2c0d3fdc..95024a07576893 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -431,11 +431,11 @@ def handle_step_up_auth end def handle_step_up_auth_for_admin_mode - handle_step_up_auth_for(::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_ADMIN_MODE) + handle_step_up_auth_for(::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_ADMIN_MODE) end def handle_step_up_auth_for_namespace - handle_step_up_auth_for(::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_NAMESPACE) + handle_step_up_auth_for(::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE) end def handle_step_up_auth_for(scope) diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index 68785e97e38970..6f085735dc83fe 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -354,7 +354,7 @@ def localized_jobs_to_be_done_choices def available_step_up_auth_providers_for_namespace Gitlab::Auth::Oidc::StepUpAuthentication.enabled_providers( - scope: Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_NAMESPACE + scope: Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE ) end end diff --git a/app/models/namespace_setting.rb b/app/models/namespace_setting.rb index eee4a1ca0a6f4f..91b4879553520a 100644 --- a/app/models/namespace_setting.rb +++ b/app/models/namespace_setting.rb @@ -61,7 +61,7 @@ class NamespaceSetting < ApplicationRecord validates :step_up_auth_required_oauth_provider, presence: true, allow_nil: true validates :step_up_auth_required_oauth_provider, inclusion: { in: ->(_) { Gitlab::Auth::Oidc::StepUpAuthentication - .enabled_providers(scope: Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_NAMESPACE) + .enabled_providers(scope: Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE) .map(&:to_s) } }, allow_nil: true validates :duo_agent_platform_request_count, numericality: { greater_than_or_equal_to: 0 } diff --git a/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb b/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb index 917d90b8f86281..8356ebc6eefa47 100644 --- a/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb +++ b/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb @@ -10,32 +10,32 @@ module Oidc module StepUpAuthBeforeRequestPhase class << self def call(env) - return if current_user_from(env).blank? + requested_scope = request_param_step_up_auth_scope_from(env).to_sym + current_user = current_user_from(env) - return {} if requested_step_up_auth_scope_from(env).blank? + return if current_user.blank? + return if requested_scope.blank? + return unless requested_scope_allowed?(requested_scope) - return {} if requested_step_up_auth_scope_from(env).to_sym == :admin_mode && - Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user_from(env)) + return if requested_scope_admin_mode?(requested_scope) && + Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user) - return {} if requested_step_up_auth_scope_from(env).to_sym == :namespace && - Feature.disabled?(:omniauth_step_up_auth_for_namespace, current_user_from(env)) - - return unless allowed_step_up_auth_scope_from(env) + return if requested_scope_namespace?(requested_scope) && + Feature.disabled?(:omniauth_step_up_auth_for_namespace, current_user) session = session_from(env) - provider = current_provider_from(env) + step_up_auth_flow = ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow( session: session, provider: provider, - scope: requested_step_up_auth_scope_from(env) + scope: requested_scope ) # TODO: Integrate the state uuid in the step-up auth session. # Why? At the moment, there is a small security vulnerability # where simultaneous authentication requests could lead to privilege escalation, https://gitlab.com/gitlab-org/gitlab/-/issues/555349. - return unless step_up_auth_flow.enabled_by_config? # This method will set the state to 'requested' in the session @@ -44,9 +44,16 @@ def call(env) private - def allowed_step_up_auth_scope_from(env) - ::Gitlab::Auth::Oidc::StepUpAuthentication::ALLOWED_SCOPES - .include?(request_param_step_up_auth_scope_from(env).to_sym) + def requested_scope_admin_mode?(scope) + scope == ::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_ADMIN_MODE + end + + def requested_scope_namespace?(scope) + scope == ::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE + end + + def requested_scope_allowed?(scope) + ::Gitlab::Auth::Oidc::StepUpAuthentication::ALLOWED_SCOPES.include?(scope) end def current_user_from(env) @@ -57,10 +64,6 @@ def current_provider_from(env) env['omniauth.strategy']&.name end - def requested_step_up_auth_scope_from(env) - request_param_step_up_auth_scope_from(env) - end - def request_param_step_up_auth_scope_from(env) env.dig('rack.request.query_hash', 'step_up_auth_scope').to_s end diff --git a/lib/gitlab/auth/oidc/step_up_authentication.rb b/lib/gitlab/auth/oidc/step_up_authentication.rb index cc36795933c51a..c993761b9be5ea 100644 --- a/lib/gitlab/auth/oidc/step_up_authentication.rb +++ b/lib/gitlab/auth/oidc/step_up_authentication.rb @@ -10,19 +10,19 @@ module Oidc module StepUpAuthentication SESSION_STORE_KEY = 'omniauth_step_up_auth' - STEP_UP_AUTH_SCOPE_ADMIN_MODE = :admin_mode - STEP_UP_AUTH_SCOPE_NAMESPACE = :namespace + SCOPE_ADMIN_MODE = :admin_mode + SCOPE_NAMESPACE = :namespace ALLOWED_SCOPES = [ - STEP_UP_AUTH_SCOPE_ADMIN_MODE, - STEP_UP_AUTH_SCOPE_NAMESPACE + SCOPE_ADMIN_MODE, + SCOPE_NAMESPACE ].freeze class << self # Checks if step-up authentication is enabled for the step-up auth scope 'admin_mode' # # @return [Boolean] true if any OAuth provider requires step-up auth for admin mode - def enabled_by_config?(scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + def enabled_by_config?(scope: SCOPE_ADMIN_MODE) oauth_providers.any? do |provider| enabled_for_provider?(provider_name: provider, scope: scope) end @@ -33,12 +33,12 @@ def enabled_by_config?(scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) # @param oauth_provider_name [String] the name of the OAuth provider # @param scope [Symbol] the scope to check configuration for (default: :admin_mode) # @return [Boolean] true if configuration exists - def enabled_for_provider?(provider_name:, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + def enabled_for_provider?(provider_name:, scope: SCOPE_ADMIN_MODE) has_required_claims?(provider_name, scope) || has_included_claims?(provider_name, scope) end - def enabled_providers(scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + def enabled_providers(scope: SCOPE_ADMIN_MODE) oauth_providers.select do |provider| enabled_for_provider?(provider_name: provider, scope: scope) end @@ -49,7 +49,7 @@ def enabled_providers(scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) # # @param session [Hash] the session hash containing authentication state # @return [Boolean] true if step-up authentication is authenticated - def succeeded?(session, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + def succeeded?(session, scope: SCOPE_ADMIN_MODE) step_up_auth_flows(session) .select do |step_up_auth_flow| step_up_auth_flow.scope.to_s == scope.to_s @@ -63,7 +63,7 @@ def succeeded?(session, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) # @param oauth [OAuth2::AccessToken] the OAuth object to validate # @param scope [Symbol] the scope to validate conditions for (default: :admin_mode) # @return [Boolean] true if all conditions are fulfilled - def conditions_fulfilled?(oauth_extra_metadata:, provider:, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + def conditions_fulfilled?(oauth_extra_metadata:, provider:, scope: SCOPE_ADMIN_MODE) conditions = [] if has_required_claims?(provider, scope) @@ -79,7 +79,7 @@ def conditions_fulfilled?(oauth_extra_metadata:, provider:, scope: STEP_UP_AUTH_ conditions.present? && conditions.all? end - def build_flow(provider:, session:, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + def build_flow(provider:, session:, scope: SCOPE_ADMIN_MODE) Gitlab::Auth::Oidc::StepUpAuthenticationFlow.new(provider: provider, scope: scope, session: session) end @@ -87,9 +87,9 @@ def build_flow(provider:, session:, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) # # @param oauth_raw_info [Hash] The raw information received from the OAuth provider. # @param provider [String] The name of the OAuth provider. - # @param scope [String] The scope of the authentication request, default is STEP_UP_AUTH_SCOPE_ADMIN_MODE. + # @param scope [String] The scope of the authentication request, default is SCOPE_ADMIN_MODE. # @return [Hash] A hash containing only the relevant ID token claims. - def slice_relevant_id_token_claims(oauth_raw_info:, provider:, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + def slice_relevant_id_token_claims(oauth_raw_info:, provider:, scope: SCOPE_ADMIN_MODE) relevant_id_token_claims = [ *get_id_token_claims_required_conditions(provider, scope)&.keys, *get_id_token_claims_included_conditions(provider, scope)&.keys @@ -101,7 +101,7 @@ def omniauth_step_up_auth_session_data(session) Gitlab::NamespacedSessionStore.new(SESSION_STORE_KEY, session) end - def disable_step_up_authentication!(session:, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + def disable_step_up_authentication!(session:, scope: SCOPE_ADMIN_MODE) omniauth_step_up_auth_session_data(session) &.to_h &.each_value do |step_up_auth_object| @@ -114,7 +114,7 @@ def disable_step_up_authentication!(session:, scope: STEP_UP_AUTH_SCOPE_ADMIN_MO # @param session [Hash] the session hash containing authentication state # @param scope [Symbol] the scope to check (default: :admin_mode) # @return [Array] list of failed authentication flows - def failed_step_up_auth_flows(session, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + def failed_step_up_auth_flows(session, scope: SCOPE_ADMIN_MODE) (step_up_auth_flows(session) || []).select do |flow| flow.enabled_by_config? && flow.failed? && diff --git a/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb b/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb index 7896c14fb440c7..6eb75e2b659d3d 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb @@ -86,38 +86,56 @@ expect(session).to eq(expected_session) end + end + + context 'when feature flag :omniauth_step_up_auth_for_admin_mode is disabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_admin_mode: false) + end + + let(:provider) { provider_step_up_auth_name } + let(:session) { {} } + + context 'with admin_mode scope' do + let(:step_up_auth_scope) { 'admin_mode' } - context 'when feature flag :omniauth_step_up_auth_for_admin_mode is disabled' do - before do - stub_feature_flags(omniauth_step_up_auth_for_admin_mode: false) + it 'does not modify the session' do + expect { call_middleware }.not_to change { session } end + end + + context 'with namespace scope' do + let(:step_up_auth_scope) { 'namespace' } - it 'handles scopes correctly with admin_mode disabled' do - if step_up_auth_scope == 'admin_mode' - # Admin mode should not be processed when disabled - expect { call_middleware }.not_to change { session } - else - # Namespace and other scopes should still work - call_middleware - expect(session).to eq(expected_session) - end + it 'sets the session state to requested for namespace scope' do + call_middleware + expect(session).to eq(session_with_namespace_step_up_state_requested) end end + end + + context 'when feature flag :omniauth_step_up_auth_for_namespace is disabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_namespace: false) + end + + let(:provider) { provider_step_up_auth_name } + let(:session) { {} } + + context 'with namespace scope' do + let(:step_up_auth_scope) { 'namespace' } - context 'when feature flag :omniauth_step_up_auth_for_namespace is disabled' do - before do - stub_feature_flags(omniauth_step_up_auth_for_namespace: false) + it 'does not modify the session' do + expect { call_middleware }.not_to change { session } end + end + + context 'with admin_mode scope' do + let(:step_up_auth_scope) { 'admin_mode' } - it 'handles scopes correctly with namespace disabled' do - if step_up_auth_scope == 'namespace' - # Namespace should not be processed when disabled - expect { call_middleware }.not_to change { session } - else - # Admin mode and other scopes should still work - call_middleware - expect(session).to eq(expected_session) - end + it 'sets the session state to requested for admin_mode scope' do + call_middleware + expect(session).to eq(session_with_step_up_state_requested) end end end -- GitLab From 9878031c95790b4188d9613c3048a2499148f758 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Thu, 25 Sep 2025 11:01:00 +0000 Subject: [PATCH 3/4] refactor: Apply suggestions from @jknabl-gitlab - Add initialize validation in StepUpAuthFlow, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/204102#note_2775958126 --- .../oidc/step_up_auth_before_request_phase.rb | 5 ---- .../auth/oidc/step_up_authentication.rb | 3 ++ .../auth/oidc/step_up_authentication_flow.rb | 9 ++++++ .../step_up_auth_before_request_phase_spec.rb | 12 ++++++-- .../oidc/step_up_authentication_flow_spec.rb | 30 +++++++++++++++---- 5 files changed, 46 insertions(+), 13 deletions(-) diff --git a/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb b/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb index 8356ebc6eefa47..d28f4fea2cd8ae 100644 --- a/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb +++ b/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb @@ -15,7 +15,6 @@ def call(env) return if current_user.blank? return if requested_scope.blank? - return unless requested_scope_allowed?(requested_scope) return if requested_scope_admin_mode?(requested_scope) && Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user) @@ -52,10 +51,6 @@ def requested_scope_namespace?(scope) scope == ::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE end - def requested_scope_allowed?(scope) - ::Gitlab::Auth::Oidc::StepUpAuthentication::ALLOWED_SCOPES.include?(scope) - end - def current_user_from(env) env['warden']&.user end diff --git a/lib/gitlab/auth/oidc/step_up_authentication.rb b/lib/gitlab/auth/oidc/step_up_authentication.rb index c993761b9be5ea..9fccd8b2cec617 100644 --- a/lib/gitlab/auth/oidc/step_up_authentication.rb +++ b/lib/gitlab/auth/oidc/step_up_authentication.rb @@ -186,8 +186,11 @@ def step_up_auth_flows(session) &.flat_map do |provider, step_up_auth_object| step_up_auth_object.map do |step_up_auth_scope, _| build_flow(provider: provider, session: session, scope: step_up_auth_scope) + rescue ArgumentError => e + Gitlab::AppLogger.error("Error building step-up authentication flow: #{e.message}") end end + &.compact end end end diff --git a/lib/gitlab/auth/oidc/step_up_authentication_flow.rb b/lib/gitlab/auth/oidc/step_up_authentication_flow.rb index 7699f30ee69a92..a3f64fe3eaf430 100644 --- a/lib/gitlab/auth/oidc/step_up_authentication_flow.rb +++ b/lib/gitlab/auth/oidc/step_up_authentication_flow.rb @@ -11,6 +11,7 @@ class StepUpAuthenticationFlow attr_reader :session, :provider, :scope def initialize(session:, provider:, scope:) + validate_scope!(scope) @session = session @provider = provider @scope = scope @@ -92,6 +93,14 @@ def conditions_fulfilled?(oidc_id_token_claims) ::Gitlab::Auth::Oidc::StepUpAuthentication .conditions_fulfilled?(oauth_extra_metadata: oidc_id_token_claims, provider: provider, scope: scope) end + + def validate_scope!(scope) + return if ::Gitlab::Auth::Oidc::StepUpAuthentication::ALLOWED_SCOPES.include?(scope&.to_sym) + + allowed = ::Gitlab::Auth::Oidc::StepUpAuthentication::ALLOWED_SCOPES + raise ArgumentError, + "Invalid scope '#{scope}'. Allowed scopes are: #{allowed.join(', ')}" + end end end end diff --git a/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb b/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb index 6eb75e2b659d3d..bbfb1ae604d379 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_auth_before_request_phase_spec.rb @@ -68,8 +68,6 @@ ref(:provider_step_up_auth_name) | 'namespace' | { 'other_session_key' => 'other_session_value' } | lazy { session.deep_merge(session_with_namespace_step_up_state_requested) } # invalid scope tests - ref(:provider_step_up_auth_name) | 'other_scope' | {} | {} - ref(:provider_step_up_auth_name) | 'other_scope' | ref(:session_with_step_up_state_requested) | ref(:session_with_step_up_state_requested) ref(:provider_step_up_auth_name) | '' | {} | {} ref(:provider_step_up_auth_name) | nil | {} | {} 'other_provider' | 'admin_mode' | {} | {} @@ -88,6 +86,16 @@ end end + context 'when invalid scope is given' do + let(:provider) { provider_step_up_auth_name } + let(:session) { {} } + let(:step_up_auth_scope) { 'other_scope' } + + it 'does not modify the session' do + expect { call_middleware }.to raise_error(ArgumentError) + end + end + context 'when feature flag :omniauth_step_up_auth_for_admin_mode is disabled' do before do stub_feature_flags(omniauth_step_up_auth_for_admin_mode: false) diff --git a/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb b/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb index 66f87af96583c9..8aea602473a803 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb @@ -10,14 +10,32 @@ subject(:flow) { described_class.new(session: session, provider: provider, scope: scope) } describe '#initialize' do - context 'when scope is not in predefined allowed scopes' do - let(:scope) { 'custom_scope' } + using RSpec::Parameterized::TableSyntax + + shared_examples 'a valid flow' do + it { is_expected.to have_attributes(scope: scope) } + end - it 'allows initialization with any scope' do - expect { flow }.not_to raise_error - expect(flow.scope).to eq('custom_scope') + shared_examples 'an invalid flow' do + it 'raises an error' do + expect { flow } + .to raise_error(ArgumentError, "Invalid scope '#{scope}'. Allowed scopes are: admin_mode, namespace") end end + + where(:scope, :expected_example) do + 'admin_mode' | 'a valid flow' + :admin_mode | 'a valid flow' + 'namespace' | 'a valid flow' + :namespace | 'a valid flow' + 'custom_scope' | 'an invalid flow' + :invalid_scope | 'an invalid flow' + nil | 'an invalid flow' + end + + with_them do + it_behaves_like params[:expected_example] + end end describe '#requested?, #succeeded?, #failed?' do @@ -145,7 +163,7 @@ where(:provider_config, :provider_name, :scope, :expected_result) do { 'step_up_auth' => { 'admin_mode' => { 'documentation_link' => 'https://example.com/company/internal/auth-help' } } } | 'openid_connect' | 'admin_mode' | 'https://example.com/company/internal/auth-help' { 'step_up_auth' => { 'admin_mode' => { 'documentation_link' => 'https://example.com/company/internal/auth-help' } } } | :openid_connect | 'admin_mode' | 'https://example.com/company/internal/auth-help' - { 'step_up_auth' => { 'custom_scope' => { 'documentation_link' => 'https://example.com/company/internal/custom-auth-help' } } } | 'openid_connect' | 'custom_scope' | 'https://example.com/company/internal/custom-auth-help' + { 'step_up_auth' => { 'namespace' => { 'documentation_link' => 'https://example.com/company/internal/namespace-auth-help' } } } | 'openid_connect' | 'namespace' | 'https://example.com/company/internal/namespace-auth-help' { 'step_up_auth' => { 'admin_mode' => {} } } | 'openid_connect' | 'admin_mode' | nil {} | 'openid_connect' | 'admin_mode' | nil end -- GitLab From 2d9f9389425b96b0c6ad9a3c83efc34b455f1c0f Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Mon, 29 Sep 2025 12:26:18 +0200 Subject: [PATCH 4/4] refactor: Apply suggestions from @jknabl-gitlab --- lib/gitlab/auth/oidc/step_up_authentication.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/gitlab/auth/oidc/step_up_authentication.rb b/lib/gitlab/auth/oidc/step_up_authentication.rb index 9fccd8b2cec617..434c8b26d911e6 100644 --- a/lib/gitlab/auth/oidc/step_up_authentication.rb +++ b/lib/gitlab/auth/oidc/step_up_authentication.rb @@ -188,6 +188,7 @@ def step_up_auth_flows(session) build_flow(provider: provider, session: session, scope: step_up_auth_scope) rescue ArgumentError => e Gitlab::AppLogger.error("Error building step-up authentication flow: #{e.message}") + nil end end &.compact -- GitLab