From 1c950a4d249cdc7d38c7cce57e91d8545ec97eae Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Thu, 16 Dec 2021 17:22:34 +0100 Subject: [PATCH 1/3] Treat API requests from the frontend as web traffic in the rate limiter This will allow us to impose stricter rate limits for general API traffic, without affecting interactive API requests made by the frontend during normal GitLab usage. The frontend requests are identified by the inclusion of a CSRF token in the headers. Other rate limits that only affect a subset of API requests (e.g. the Files and Packages APIs, or protected paths) still take precedence, i.e. requests for these paths will always be matched even if they include a CSRF token. Changelog: changed --- .../settings/user_and_ip_rate_limits.md | 4 + lib/gitlab/rack_attack/request.rb | 33 ++++--- .../application_cable/connection_spec.rb | 8 +- spec/lib/gitlab/rack_attack/request_spec.rb | 71 +++++++++++++++- spec/requests/api/commits_spec.rb | 10 +-- spec/requests/rack_attack_global_spec.rb | 17 ++++ .../helpers/rack_attack_spec_helpers.rb | 4 +- spec/support/helpers/session_helpers.rb | 16 ++++ .../mutations/snippets_shared_examples.rb | 11 +-- .../requests/rack_attack_shared_examples.rb | 85 +++++++++++++++++++ 10 files changed, 222 insertions(+), 37 deletions(-) diff --git a/doc/user/admin_area/settings/user_and_ip_rate_limits.md b/doc/user/admin_area/settings/user_and_ip_rate_limits.md index d713ef4b4e0d62..ad61f18343c791 100644 --- a/doc/user/admin_area/settings/user_and_ip_rate_limits.md +++ b/doc/user/admin_area/settings/user_and_ip_rate_limits.md @@ -22,6 +22,10 @@ NOTE: By default, all Git operations are first tried unauthenticated. Because of this, HTTP Git operations may trigger the rate limits configured for unauthenticated requests. +NOTE: +The rate limits for API requests don't affect requests made by the frontend, as these are always +counted as web traffic. + ## Enable unauthenticated API request rate limit To enable the unauthenticated request rate limit: diff --git a/lib/gitlab/rack_attack/request.rb b/lib/gitlab/rack_attack/request.rb index 94ae29af3d0f26..b861df2092ccec 100644 --- a/lib/gitlab/rack_attack/request.rb +++ b/lib/gitlab/rack_attack/request.rb @@ -3,6 +3,8 @@ module Gitlab module RackAttack module Request + include ::Gitlab::Utils::StrongMemoize + FILES_PATH_REGEX = %r{^/api/v\d+/projects/[^/]+/repository/files/.+}.freeze GROUP_PATH_REGEX = %r{^/api/v\d+/groups/[^/]+/?$}.freeze @@ -30,15 +32,15 @@ def api_request? end def api_internal_request? - path =~ %r{^/api/v\d+/internal/} + path.match?(%r{^/api/v\d+/internal/}) end def health_check_request? - path =~ %r{^/-/(health|liveness|readiness|metrics)} + path.match?(%r{^/-/(health|liveness|readiness|metrics)}) end def container_registry_event? - path =~ %r{^/api/v\d+/container_registry_event/} + path.match?(%r{^/api/v\d+/container_registry_event/}) end def product_analytics_collector_request? @@ -58,7 +60,7 @@ def protected_path? end def protected_path_regex - path =~ protected_paths_regex + path.match?(protected_paths_regex) end def throttle?(throttle, authenticated:) @@ -70,6 +72,7 @@ def throttle?(throttle, authenticated:) def throttle_unauthenticated_api? api_request? && !should_be_skipped? && + !frontend_request? && !throttle_unauthenticated_packages_api? && !throttle_unauthenticated_files_api? && !throttle_unauthenticated_deprecated_api? && @@ -78,7 +81,7 @@ def throttle_unauthenticated_api? end def throttle_unauthenticated_web? - web_request? && + (web_request? || frontend_request?) && !should_be_skipped? && # TODO: Column will be renamed in https://gitlab.com/gitlab-org/gitlab/-/issues/340031 Gitlab::Throttle.settings.throttle_unauthenticated_enabled && @@ -87,6 +90,7 @@ def throttle_unauthenticated_web? def throttle_authenticated_api? api_request? && + !frontend_request? && !throttle_authenticated_packages_api? && !throttle_authenticated_files_api? && !throttle_authenticated_deprecated_api? && @@ -94,7 +98,7 @@ def throttle_authenticated_api? end def throttle_authenticated_web? - web_request? && + (web_request? || frontend_request?) && !throttle_authenticated_git_lfs? && Gitlab::Throttle.settings.throttle_authenticated_web_enabled end @@ -178,15 +182,24 @@ def protected_paths_regex end def packages_api_path? - path =~ ::Gitlab::Regex::Packages::API_PATH_REGEX + path.match?(::Gitlab::Regex::Packages::API_PATH_REGEX) end def git_lfs_path? - path =~ Gitlab::PathRegex.repository_git_lfs_route_regex + path.match?(Gitlab::PathRegex.repository_git_lfs_route_regex) end def files_api_path? - path =~ FILES_PATH_REGEX + path.match?(FILES_PATH_REGEX) + end + + def frontend_request? + strong_memoize(:frontend_request) do + next false unless env.include?('HTTP_X_CSRF_TOKEN') && session.include?(:_csrf_token) + + # CSRF tokens are not verified for GET/HEAD requests, so we pretend that we always have a POST request. + Gitlab::RequestForgeryProtection.verified?(env.merge('REQUEST_METHOD' => 'POST')) + end end def deprecated_api_request? @@ -195,7 +208,7 @@ def deprecated_api_request? with_projects = params['with_projects'] with_projects = true if with_projects.blank? - path =~ GROUP_PATH_REGEX && Gitlab::Utils.to_boolean(with_projects) + path.match?(GROUP_PATH_REGEX) && Gitlab::Utils.to_boolean(with_projects) end end end diff --git a/spec/channels/application_cable/connection_spec.rb b/spec/channels/application_cable/connection_spec.rb index c10e0c0cab4511..affde0095cff01 100644 --- a/spec/channels/application_cable/connection_spec.rb +++ b/spec/channels/application_cable/connection_spec.rb @@ -3,15 +3,11 @@ require 'spec_helper' RSpec.describe ApplicationCable::Connection, :clean_gitlab_redis_sessions do - let(:session_id) { Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') } + include SessionHelpers context 'when session cookie is set' do before do - Gitlab::Redis::Sessions.with do |redis| - redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash)) - end - - cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id + stub_session(session_hash) end context 'when user is logged in' do diff --git a/spec/lib/gitlab/rack_attack/request_spec.rb b/spec/lib/gitlab/rack_attack/request_spec.rb index ecdcc23e5881d3..1110399314f2a2 100644 --- a/spec/lib/gitlab/rack_attack/request_spec.rb +++ b/spec/lib/gitlab/rack_attack/request_spec.rb @@ -5,6 +5,19 @@ RSpec.describe Gitlab::RackAttack::Request do using RSpec::Parameterized::TableSyntax + let(:env) { {} } + let(:session) { {} } + let(:request) do + ::Rack::Attack::Request.new( + env.reverse_merge( + 'REQUEST_METHOD' => 'GET', + 'PATH_INFO' => path, + 'rack.input' => StringIO.new, + 'rack.session' => session + ) + ) + end + describe 'FILES_PATH_REGEX' do subject { described_class::FILES_PATH_REGEX } @@ -16,11 +29,63 @@ it { is_expected.not_to match('/api/v4/projects/some/nested/repo/repository/files/README') } end + describe '#api_request?' do + subject { request.api_request? } + + where(:path, :env, :expected) do + '/' | {} | false + '/groups' | {} | false + + '/api' | {} | true + '/api/v4/groups/1' | {} | true + end + + with_them do + it { is_expected.to eq(expected) } + end + end + + describe '#web_request?' do + subject { request.web_request? } + + where(:path, :env, :expected) do + '/' | {} | true + '/groups' | {} | true + + '/api' | {} | false + '/api/v4/groups/1' | {} | false + end + + with_them do + it { is_expected.to eq(expected) } + end + end + + describe '#frontend_request?', :allow_forgery_protection do + subject { request.send(:frontend_request?) } + + let(:path) { '/' } + + # Define these as local variables so we can use them in the `where` block. + valid_token = SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) + other_token = SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) + + where(:session, :env, :expected) do + {} | {} | false # rubocop:disable Lint/BinaryOperatorWithIdenticalOperands + {} | { 'HTTP_X_CSRF_TOKEN' => valid_token } | false + { _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => other_token } | false + { _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => valid_token } | true + end + + with_them do + it { is_expected.to eq(expected) } + end + end + describe '#deprecated_api_request?' do - let(:env) { { 'REQUEST_METHOD' => 'GET', 'rack.input' => StringIO.new, 'PATH_INFO' => path, 'QUERY_STRING' => query } } - let(:request) { ::Rack::Attack::Request.new(env) } + subject { request.send(:deprecated_api_request?) } - subject { !!request.__send__(:deprecated_api_request?) } + let(:env) { { 'QUERY_STRING' => query } } where(:path, :query, :expected) do '/' | '' | false diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 2bc642f8b14806..07c8f76cbbf392 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -5,6 +5,7 @@ RSpec.describe API::Commits do include ProjectForksHelper + include SessionHelpers let(:user) { create(:user) } let(:guest) { create(:user).tap { |u| project.add_guest(u) } } @@ -378,14 +379,7 @@ context 'when using warden' do it 'increments usage counters', :clean_gitlab_redis_sessions do - session_id = Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') - session_hash = { 'warden.user.user.key' => [[user.id], user.encrypted_password[0, 29]] } - - Gitlab::Redis::Sessions.with do |redis| - redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash)) - end - - cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id + stub_session('warden.user.user.key' => [[user.id], user.encrypted_password[0, 29]]) expect(::Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_commits_count) expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).to receive(:track_web_ide_edit_action) diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb index 793438808a5932..f8c8351e1b3027 100644 --- a/spec/requests/rack_attack_global_spec.rb +++ b/spec/requests/rack_attack_global_spec.rb @@ -4,6 +4,7 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_caching do include RackAttackSpecHelpers + include SessionHelpers let(:settings) { Gitlab::CurrentSettings.current_application_settings } @@ -63,6 +64,22 @@ end end + describe 'API requests from the frontend', :api, :clean_gitlab_redis_sessions do + context 'when unauthenticated' do + it_behaves_like 'rate-limited frontend API requests' do + let(:throttle_setting_prefix) { 'throttle_unauthenticated' } + end + end + + context 'when authenticated' do + it_behaves_like 'rate-limited frontend API requests' do + let_it_be(:personal_access_token) { create(:personal_access_token) } + + let(:throttle_setting_prefix) { 'throttle_authenticated' } + end + end + end + describe 'API requests authenticated with personal access token', :api do let_it_be(:user) { create(:user) } let_it_be(:token) { create(:personal_access_token, user: user) } diff --git a/spec/support/helpers/rack_attack_spec_helpers.rb b/spec/support/helpers/rack_attack_spec_helpers.rb index d50a6382a40668..c82a87dc58ec25 100644 --- a/spec/support/helpers/rack_attack_spec_helpers.rb +++ b/spec/support/helpers/rack_attack_spec_helpers.rb @@ -26,14 +26,14 @@ def basic_auth_headers(user, personal_access_token) { 'AUTHORIZATION' => "Basic #{encoded_login}" } end - def expect_rejection(&block) + def expect_rejection(name = nil, &block) yield expect(response).to have_gitlab_http_status(:too_many_requests) expect(response.headers.to_h).to include( 'RateLimit-Limit' => a_string_matching(/^\d+$/), - 'RateLimit-Name' => a_string_matching(/^throttle_.*$/), + 'RateLimit-Name' => name || a_string_matching(/^throttle_.*$/), 'RateLimit-Observed' => a_string_matching(/^\d+$/), 'RateLimit-Remaining' => a_string_matching(/^\d+$/), 'Retry-After' => a_string_matching(/^\d+$/) diff --git a/spec/support/helpers/session_helpers.rb b/spec/support/helpers/session_helpers.rb index 236585296e5713..394a401afcaab4 100644 --- a/spec/support/helpers/session_helpers.rb +++ b/spec/support/helpers/session_helpers.rb @@ -1,6 +1,22 @@ # frozen_string_literal: true module SessionHelpers + # Stub a session in Redis, for use in request specs where we can't mock the session directly. + # This also needs the :clean_gitlab_redis_sessions tag on the spec. + def stub_session(session_hash) + unless RSpec.current_example.metadata[:clean_gitlab_redis_sessions] + raise 'Add :clean_gitlab_redis_sessions to your spec!' + end + + session_id = Rack::Session::SessionId.new(SecureRandom.hex) + + Gitlab::Redis::Sessions.with do |redis| + redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash)) + end + + cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id + end + def expect_single_session_with_authenticated_ttl expect_single_session_with_expiration(Settings.gitlab['session_expire_delay'] * 60) end diff --git a/spec/support/shared_examples/requests/api/graphql/mutations/snippets_shared_examples.rb b/spec/support/shared_examples/requests/api/graphql/mutations/snippets_shared_examples.rb index 8bffd1f71e9d42..a42a1fda62e3f8 100644 --- a/spec/support/shared_examples/requests/api/graphql/mutations/snippets_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/graphql/mutations/snippets_shared_examples.rb @@ -10,6 +10,8 @@ end RSpec.shared_examples 'snippet edit usage data counters' do + include SessionHelpers + context 'when user is sessionless' do it 'does not track usage data actions' do expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).not_to receive(:track_snippet_editor_edit_action) @@ -20,14 +22,7 @@ context 'when user is not sessionless', :clean_gitlab_redis_sessions do before do - session_id = Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') - session_hash = { 'warden.user.user.key' => [[current_user.id], current_user.encrypted_password[0, 29]] } - - Gitlab::Redis::Sessions.with do |redis| - redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash)) - end - - cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id + stub_session('warden.user.user.key' => [[current_user.id], current_user.encrypted_password[0, 29]]) end it 'tracks usage data actions', :clean_gitlab_redis_sessions do diff --git a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb index b294467d482311..c6c6c44dce893c 100644 --- a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb +++ b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb @@ -580,3 +580,88 @@ def do_request end end end + +# Requires let variables: +# * throttle_setting_prefix: "throttle_authenticated", "throttle_unauthenticated" +RSpec.shared_examples 'rate-limited frontend API requests' do + let(:requests_per_period) { 1 } + let(:csrf_token) { SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) } + let(:csrf_session) { { _csrf_token: csrf_token } } + let(:personal_access_token) { nil } + + let(:api_path) { '/projects' } + + # These don't actually exist, so a 404 is the expected response. + let(:files_api_path) { '/projects/1/repository/files/ref/path' } + let(:packages_api_path) { '/projects/1/packages/foo' } + let(:deprecated_api_path) { '/groups/1?with_projects=true' } + + def get_api(path: api_path, csrf: false) + headers = csrf ? { 'X-CSRF-Token' => csrf_token } : nil + get api(path, personal_access_token: personal_access_token), headers: headers + end + + def expect_not_found(&block) + yield + + expect(response).to have_gitlab_http_status(:not_found) + end + + before do + stub_application_setting( + "#{throttle_setting_prefix}_enabled" => true, + "#{throttle_setting_prefix}_requests_per_period" => requests_per_period, + "#{throttle_setting_prefix}_api_enabled" => true, + "#{throttle_setting_prefix}_api_requests_per_period" => requests_per_period, + "#{throttle_setting_prefix}_web_enabled" => true, + "#{throttle_setting_prefix}_web_requests_per_period" => requests_per_period, + "#{throttle_setting_prefix}_files_api_enabled" => true, + "#{throttle_setting_prefix}_packages_api_enabled" => true, + "#{throttle_setting_prefix}_deprecated_api_enabled" => true + ) + + stub_session(csrf_session) + end + + context 'with a CSRF token' do + it 'uses the rate limit for web requests' do + requests_per_period.times { get_api csrf: true } + + expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true } + expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true, path: files_api_path } + expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true, path: packages_api_path } + expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true, path: deprecated_api_path } + + # API rate limit is not triggered yet + expect_ok { get_api } + expect_not_found { get_api path: files_api_path } + expect_not_found { get_api path: packages_api_path } + expect_not_found { get_api path: deprecated_api_path } + end + + context 'without a CSRF session' do + let(:csrf_session) { nil } + + it 'always uses the rate limit for API requests' do + requests_per_period.times { get_api csrf: true } + + expect_rejection("#{throttle_setting_prefix}_api") { get_api csrf: true } + expect_rejection("#{throttle_setting_prefix}_api") { get_api } + end + end + end + + context 'without a CSRF token' do + it 'uses the rate limit for API requests' do + requests_per_period.times { get_api } + + expect_rejection("#{throttle_setting_prefix}_api") { get_api } + + # Web and custom API rate limits are not triggered yet + expect_ok { get_api csrf: true } + expect_not_found { get_api path: files_api_path } + expect_not_found { get_api path: packages_api_path } + expect_not_found { get_api path: deprecated_api_path } + end + end +end -- GitLab From 7c551161472c93ea3bab284595b5deeb53c45644 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Wed, 12 Jan 2022 15:40:28 +0100 Subject: [PATCH 2/3] Add feature flag --- .../rate_limit_frontend_requests.yml | 8 ++++++++ lib/gitlab/rack_attack/request.rb | 2 ++ spec/lib/gitlab/rack_attack/request_spec.rb | 17 +++++++++++++++++ 3 files changed, 27 insertions(+) create mode 100644 config/feature_flags/development/rate_limit_frontend_requests.yml diff --git a/config/feature_flags/development/rate_limit_frontend_requests.yml b/config/feature_flags/development/rate_limit_frontend_requests.yml new file mode 100644 index 00000000000000..624dc303568529 --- /dev/null +++ b/config/feature_flags/development/rate_limit_frontend_requests.yml @@ -0,0 +1,8 @@ +--- +name: rate_limit_frontend_requests +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78082 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/350623 +milestone: '14.8' +type: development +group: group::integrations +default_enabled: false diff --git a/lib/gitlab/rack_attack/request.rb b/lib/gitlab/rack_attack/request.rb index b861df2092ccec..d4a65227a20308 100644 --- a/lib/gitlab/rack_attack/request.rb +++ b/lib/gitlab/rack_attack/request.rb @@ -194,6 +194,8 @@ def files_api_path? end def frontend_request? + return false unless Feature.enabled?(:rate_limit_frontend_requests, default_enabled: :yaml) + strong_memoize(:frontend_request) do next false unless env.include?('HTTP_X_CSRF_TOKEN') && session.include?(:_csrf_token) diff --git a/spec/lib/gitlab/rack_attack/request_spec.rb b/spec/lib/gitlab/rack_attack/request_spec.rb index 1110399314f2a2..3efac08b923b40 100644 --- a/spec/lib/gitlab/rack_attack/request_spec.rb +++ b/spec/lib/gitlab/rack_attack/request_spec.rb @@ -80,6 +80,23 @@ with_them do it { is_expected.to eq(expected) } end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(rate_limit_frontend_requests: false) + end + + where(:session, :env) do + {} | {} # rubocop:disable Lint/BinaryOperatorWithIdenticalOperands + {} | { 'HTTP_X_CSRF_TOKEN' => valid_token } + { _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => other_token } + { _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => valid_token } + end + + with_them do + it { is_expected.to be(false) } + end + end end describe '#deprecated_api_request?' do -- GitLab From 5baa3928b947ae5c76dcb515f1b2bf20db1b76b5 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Thu, 20 Jan 2022 13:41:30 +0100 Subject: [PATCH 3/3] Clean up specs --- spec/lib/gitlab/rack_attack/request_spec.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/lib/gitlab/rack_attack/request_spec.rb b/spec/lib/gitlab/rack_attack/request_spec.rb index 3efac08b923b40..72592832a8966c 100644 --- a/spec/lib/gitlab/rack_attack/request_spec.rb +++ b/spec/lib/gitlab/rack_attack/request_spec.rb @@ -32,12 +32,12 @@ describe '#api_request?' do subject { request.api_request? } - where(:path, :env, :expected) do - '/' | {} | false - '/groups' | {} | false + where(:path, :expected) do + '/' | false + '/groups' | false - '/api' | {} | true - '/api/v4/groups/1' | {} | true + '/api' | true + '/api/v4/groups/1' | true end with_them do @@ -48,12 +48,12 @@ describe '#web_request?' do subject { request.web_request? } - where(:path, :env, :expected) do - '/' | {} | true - '/groups' | {} | true + where(:path, :expected) do + '/' | true + '/groups' | true - '/api' | {} | false - '/api/v4/groups/1' | {} | false + '/api' | false + '/api/v4/groups/1' | false end with_them do -- GitLab