Commit a5440d5b authored by Markus Koller's avatar Markus Koller Committed by Matthias Käppler

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
parent 7a7c032c
---
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
...@@ -22,6 +22,10 @@ NOTE: ...@@ -22,6 +22,10 @@ NOTE:
By default, all Git operations are first tried unauthenticated. Because of this, HTTP Git operations By default, all Git operations are first tried unauthenticated. Because of this, HTTP Git operations
may trigger the rate limits configured for unauthenticated requests. 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 ## Enable unauthenticated API request rate limit
To enable the unauthenticated request rate limit: To enable the unauthenticated request rate limit:
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
module Gitlab module Gitlab
module RackAttack module RackAttack
module Request module Request
include ::Gitlab::Utils::StrongMemoize
FILES_PATH_REGEX = %r{^/api/v\d+/projects/[^/]+/repository/files/.+}.freeze FILES_PATH_REGEX = %r{^/api/v\d+/projects/[^/]+/repository/files/.+}.freeze
GROUP_PATH_REGEX = %r{^/api/v\d+/groups/[^/]+/?$}.freeze GROUP_PATH_REGEX = %r{^/api/v\d+/groups/[^/]+/?$}.freeze
...@@ -30,15 +32,15 @@ module Gitlab ...@@ -30,15 +32,15 @@ module Gitlab
end end
def api_internal_request? def api_internal_request?
path =~ %r{^/api/v\d+/internal/} path.match?(%r{^/api/v\d+/internal/})
end end
def health_check_request? def health_check_request?
path =~ %r{^/-/(health|liveness|readiness|metrics)} path.match?(%r{^/-/(health|liveness|readiness|metrics)})
end end
def container_registry_event? def container_registry_event?
path =~ %r{^/api/v\d+/container_registry_event/} path.match?(%r{^/api/v\d+/container_registry_event/})
end end
def product_analytics_collector_request? def product_analytics_collector_request?
...@@ -58,7 +60,7 @@ module Gitlab ...@@ -58,7 +60,7 @@ module Gitlab
end end
def protected_path_regex def protected_path_regex
path =~ protected_paths_regex path.match?(protected_paths_regex)
end end
def throttle?(throttle, authenticated:) def throttle?(throttle, authenticated:)
...@@ -70,6 +72,7 @@ module Gitlab ...@@ -70,6 +72,7 @@ module Gitlab
def throttle_unauthenticated_api? def throttle_unauthenticated_api?
api_request? && api_request? &&
!should_be_skipped? && !should_be_skipped? &&
!frontend_request? &&
!throttle_unauthenticated_packages_api? && !throttle_unauthenticated_packages_api? &&
!throttle_unauthenticated_files_api? && !throttle_unauthenticated_files_api? &&
!throttle_unauthenticated_deprecated_api? && !throttle_unauthenticated_deprecated_api? &&
...@@ -78,7 +81,7 @@ module Gitlab ...@@ -78,7 +81,7 @@ module Gitlab
end end
def throttle_unauthenticated_web? def throttle_unauthenticated_web?
web_request? && (web_request? || frontend_request?) &&
!should_be_skipped? && !should_be_skipped? &&
# TODO: Column will be renamed in https://gitlab.com/gitlab-org/gitlab/-/issues/340031 # TODO: Column will be renamed in https://gitlab.com/gitlab-org/gitlab/-/issues/340031
Gitlab::Throttle.settings.throttle_unauthenticated_enabled && Gitlab::Throttle.settings.throttle_unauthenticated_enabled &&
...@@ -87,6 +90,7 @@ module Gitlab ...@@ -87,6 +90,7 @@ module Gitlab
def throttle_authenticated_api? def throttle_authenticated_api?
api_request? && api_request? &&
!frontend_request? &&
!throttle_authenticated_packages_api? && !throttle_authenticated_packages_api? &&
!throttle_authenticated_files_api? && !throttle_authenticated_files_api? &&
!throttle_authenticated_deprecated_api? && !throttle_authenticated_deprecated_api? &&
...@@ -94,7 +98,7 @@ module Gitlab ...@@ -94,7 +98,7 @@ module Gitlab
end end
def throttle_authenticated_web? def throttle_authenticated_web?
web_request? && (web_request? || frontend_request?) &&
!throttle_authenticated_git_lfs? && !throttle_authenticated_git_lfs? &&
Gitlab::Throttle.settings.throttle_authenticated_web_enabled Gitlab::Throttle.settings.throttle_authenticated_web_enabled
end end
...@@ -178,15 +182,26 @@ module Gitlab ...@@ -178,15 +182,26 @@ module Gitlab
end end
def packages_api_path? def packages_api_path?
path =~ ::Gitlab::Regex::Packages::API_PATH_REGEX path.match?(::Gitlab::Regex::Packages::API_PATH_REGEX)
end end
def git_lfs_path? def git_lfs_path?
path =~ Gitlab::PathRegex.repository_git_lfs_route_regex path.match?(Gitlab::PathRegex.repository_git_lfs_route_regex)
end end
def files_api_path? def files_api_path?
path =~ FILES_PATH_REGEX path.match?(FILES_PATH_REGEX)
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)
# 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 end
def deprecated_api_request? def deprecated_api_request?
...@@ -195,7 +210,7 @@ module Gitlab ...@@ -195,7 +210,7 @@ module Gitlab
with_projects = params['with_projects'] with_projects = params['with_projects']
with_projects = true if with_projects.blank? 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 end
end end
......
...@@ -3,15 +3,11 @@ ...@@ -3,15 +3,11 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe ApplicationCable::Connection, :clean_gitlab_redis_sessions do 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 context 'when session cookie is set' do
before do before do
Gitlab::Redis::Sessions.with do |redis| stub_session(session_hash)
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 end
context 'when user is logged in' do context 'when user is logged in' do
......
...@@ -5,6 +5,19 @@ require 'spec_helper' ...@@ -5,6 +5,19 @@ require 'spec_helper'
RSpec.describe Gitlab::RackAttack::Request do RSpec.describe Gitlab::RackAttack::Request do
using RSpec::Parameterized::TableSyntax 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 describe 'FILES_PATH_REGEX' do
subject { described_class::FILES_PATH_REGEX } subject { described_class::FILES_PATH_REGEX }
...@@ -16,11 +29,80 @@ RSpec.describe Gitlab::RackAttack::Request do ...@@ -16,11 +29,80 @@ RSpec.describe Gitlab::RackAttack::Request do
it { is_expected.not_to match('/api/v4/projects/some/nested/repo/repository/files/README') } it { is_expected.not_to match('/api/v4/projects/some/nested/repo/repository/files/README') }
end end
describe '#api_request?' do
subject { request.api_request? }
where(:path, :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, :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
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 describe '#deprecated_api_request?' do
let(:env) { { 'REQUEST_METHOD' => 'GET', 'rack.input' => StringIO.new, 'PATH_INFO' => path, 'QUERY_STRING' => query } } subject { request.send(:deprecated_api_request?) }
let(:request) { ::Rack::Attack::Request.new(env) }
subject { !!request.__send__(:deprecated_api_request?) } let(:env) { { 'QUERY_STRING' => query } }
where(:path, :query, :expected) do where(:path, :query, :expected) do
'/' | '' | false '/' | '' | false
......
...@@ -5,6 +5,7 @@ require 'mime/types' ...@@ -5,6 +5,7 @@ require 'mime/types'
RSpec.describe API::Commits do RSpec.describe API::Commits do
include ProjectForksHelper include ProjectForksHelper
include SessionHelpers
let(:user) { create(:user) } let(:user) { create(:user) }
let(:guest) { create(:user).tap { |u| project.add_guest(u) } } let(:guest) { create(:user).tap { |u| project.add_guest(u) } }
...@@ -378,14 +379,7 @@ RSpec.describe API::Commits do ...@@ -378,14 +379,7 @@ RSpec.describe API::Commits do
context 'when using warden' do context 'when using warden' do
it 'increments usage counters', :clean_gitlab_redis_sessions do it 'increments usage counters', :clean_gitlab_redis_sessions do
session_id = Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') stub_session('warden.user.user.key' => [[user.id], user.encrypted_password[0, 29]])
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
expect(::Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_commits_count) expect(::Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_commits_count)
expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).to receive(:track_web_ide_edit_action) expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).to receive(:track_web_ide_edit_action)
......
...@@ -4,6 +4,7 @@ require 'spec_helper' ...@@ -4,6 +4,7 @@ require 'spec_helper'
RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_caching do RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_caching do
include RackAttackSpecHelpers include RackAttackSpecHelpers
include SessionHelpers
let(:settings) { Gitlab::CurrentSettings.current_application_settings } let(:settings) { Gitlab::CurrentSettings.current_application_settings }
...@@ -63,6 +64,22 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac ...@@ -63,6 +64,22 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
end end
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 describe 'API requests authenticated with personal access token', :api do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:token) { create(:personal_access_token, user: user) } let_it_be(:token) { create(:personal_access_token, user: user) }
......
...@@ -26,14 +26,14 @@ module RackAttackSpecHelpers ...@@ -26,14 +26,14 @@ module RackAttackSpecHelpers
{ 'AUTHORIZATION' => "Basic #{encoded_login}" } { 'AUTHORIZATION' => "Basic #{encoded_login}" }
end end
def expect_rejection(&block) def expect_rejection(name = nil, &block)
yield yield
expect(response).to have_gitlab_http_status(:too_many_requests) expect(response).to have_gitlab_http_status(:too_many_requests)
expect(response.headers.to_h).to include( expect(response.headers.to_h).to include(
'RateLimit-Limit' => a_string_matching(/^\d+$/), '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-Observed' => a_string_matching(/^\d+$/),
'RateLimit-Remaining' => a_string_matching(/^\d+$/), 'RateLimit-Remaining' => a_string_matching(/^\d+$/),
'Retry-After' => a_string_matching(/^\d+$/) 'Retry-After' => a_string_matching(/^\d+$/)
......
# frozen_string_literal: true # frozen_string_literal: true
module SessionHelpers 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 def expect_single_session_with_authenticated_ttl
expect_single_session_with_expiration(Settings.gitlab['session_expire_delay'] * 60) expect_single_session_with_expiration(Settings.gitlab['session_expire_delay'] * 60)
end end
......
...@@ -10,6 +10,8 @@ RSpec.shared_examples 'when the snippet is not found' do ...@@ -10,6 +10,8 @@ RSpec.shared_examples 'when the snippet is not found' do
end end
RSpec.shared_examples 'snippet edit usage data counters' do RSpec.shared_examples 'snippet edit usage data counters' do
include SessionHelpers
context 'when user is sessionless' do context 'when user is sessionless' do
it 'does not track usage data actions' do it 'does not track usage data actions' do
expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).not_to receive(:track_snippet_editor_edit_action) expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).not_to receive(:track_snippet_editor_edit_action)
...@@ -20,14 +22,7 @@ RSpec.shared_examples 'snippet edit usage data counters' do ...@@ -20,14 +22,7 @@ RSpec.shared_examples 'snippet edit usage data counters' do
context 'when user is not sessionless', :clean_gitlab_redis_sessions do context 'when user is not sessionless', :clean_gitlab_redis_sessions do
before do before do
session_id = Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') stub_session('warden.user.user.key' => [[current_user.id], current_user.encrypted_password[0, 29]])
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
end end
it 'tracks usage data actions', :clean_gitlab_redis_sessions do it 'tracks usage data actions', :clean_gitlab_redis_sessions do
......
...@@ -580,3 +580,88 @@ RSpec.shared_examples 'rate-limited unauthenticated requests' do ...@@ -580,3 +580,88 @@ RSpec.shared_examples 'rate-limited unauthenticated requests' do
end end
end 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
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