Commit 356b7729 authored by Markus Koller's avatar Markus Koller

Apply throttling settings for unauthenticated API requests

This applies the new rate limit settings for unauthenticated API
requests, and restricts the previous general rate limit for
unauthenticated requests to web requests.

Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/335300
Changelog: added
parent 004732b0
...@@ -82,8 +82,14 @@ module Gitlab ...@@ -82,8 +82,14 @@ module Gitlab
end end
def self.configure_throttles(rack_attack) def self.configure_throttles(rack_attack)
throttle_or_track(rack_attack, 'throttle_unauthenticated', Gitlab::Throttle.unauthenticated_options) do |req| throttle_or_track(rack_attack, 'throttle_unauthenticated_api', Gitlab::Throttle.unauthenticated_api_options) do |req|
if req.throttle_unauthenticated? if req.throttle_unauthenticated_api?
req.ip
end
end
throttle_or_track(rack_attack, 'throttle_unauthenticated_web', Gitlab::Throttle.unauthenticated_web_options) do |req|
if req.throttle_unauthenticated_web?
req.ip req.ip
end end
end end
...@@ -177,7 +183,15 @@ module Gitlab ...@@ -177,7 +183,15 @@ module Gitlab
return false if dry_run_config.empty? return false if dry_run_config.empty?
return true if dry_run_config == '*' return true if dry_run_config == '*'
dry_run_config.split(',').map(&:strip).include?(name) dry_run_throttles = dry_run_config.split(',').map(&:strip)
# `throttle_unauthenticated` was split into API and web, so to maintain backwards-compatibility
# this throttle name now controls both rate limits.
if dry_run_throttles.include?('throttle_unauthenticated')
dry_run_throttles += %w[throttle_unauthenticated_api throttle_unauthenticated_web]
end
dry_run_throttles.include?(name)
end end
def self.user_allowlist def self.user_allowlist
......
...@@ -60,10 +60,19 @@ module Gitlab ...@@ -60,10 +60,19 @@ module Gitlab
path =~ protected_paths_regex path =~ protected_paths_regex
end end
def throttle_unauthenticated? def throttle_unauthenticated_api?
api_request? &&
!should_be_skipped? && !should_be_skipped? &&
!throttle_unauthenticated_packages_api? && !throttle_unauthenticated_packages_api? &&
!throttle_unauthenticated_files_api? && !throttle_unauthenticated_files_api? &&
Gitlab::Throttle.settings.throttle_unauthenticated_api_enabled &&
unauthenticated?
end
def throttle_unauthenticated_web?
web_request? &&
!should_be_skipped? &&
# 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 &&
unauthenticated? unauthenticated?
end end
......
...@@ -24,7 +24,14 @@ module Gitlab ...@@ -24,7 +24,14 @@ module Gitlab
"HTTP_#{env_value.upcase.tr('-', '_')}" "HTTP_#{env_value.upcase.tr('-', '_')}"
end end
def self.unauthenticated_options def self.unauthenticated_api_options
limit_proc = proc { |req| settings.throttle_unauthenticated_api_requests_per_period }
period_proc = proc { |req| settings.throttle_unauthenticated_api_period_in_seconds.seconds }
{ limit: limit_proc, period: period_proc }
end
def self.unauthenticated_web_options
# TODO: Columns will be renamed in https://gitlab.com/gitlab-org/gitlab/-/issues/340031
limit_proc = proc { |req| settings.throttle_unauthenticated_requests_per_period } limit_proc = proc { |req| settings.throttle_unauthenticated_requests_per_period }
period_proc = proc { |req| settings.throttle_unauthenticated_period_in_seconds.seconds } period_proc = proc { |req| settings.throttle_unauthenticated_period_in_seconds.seconds }
{ limit: limit_proc, period: period_proc } { limit: limit_proc, period: period_proc }
......
...@@ -10,12 +10,19 @@ RSpec.describe Gitlab::RackAttack, :aggregate_failures do ...@@ -10,12 +10,19 @@ RSpec.describe Gitlab::RackAttack, :aggregate_failures do
let(:throttles) do let(:throttles) do
{ {
throttle_unauthenticated: Gitlab::Throttle.unauthenticated_options, throttle_unauthenticated_api: Gitlab::Throttle.unauthenticated_api_options,
throttle_unauthenticated_web: Gitlab::Throttle.unauthenticated_web_options,
throttle_authenticated_api: Gitlab::Throttle.authenticated_api_options, throttle_authenticated_api: Gitlab::Throttle.authenticated_api_options,
throttle_product_analytics_collector: { limit: 100, period: 60 }, throttle_product_analytics_collector: { limit: 100, period: 60 },
throttle_unauthenticated_protected_paths: Gitlab::Throttle.unauthenticated_options, throttle_authenticated_web: Gitlab::Throttle.authenticated_web_options,
throttle_authenticated_protected_paths_api: Gitlab::Throttle.authenticated_api_options, throttle_unauthenticated_protected_paths: Gitlab::Throttle.protected_paths_options,
throttle_authenticated_protected_paths_web: Gitlab::Throttle.authenticated_web_options throttle_authenticated_protected_paths_api: Gitlab::Throttle.protected_paths_options,
throttle_authenticated_protected_paths_web: Gitlab::Throttle.protected_paths_options,
throttle_unauthenticated_packages_api: Gitlab::Throttle.unauthenticated_packages_api_options,
throttle_authenticated_packages_api: Gitlab::Throttle.authenticated_packages_api_options,
throttle_authenticated_git_lfs: Gitlab::Throttle.throttle_authenticated_git_lfs_options,
throttle_unauthenticated_files_api: Gitlab::Throttle.unauthenticated_files_api_options,
throttle_authenticated_files_api: Gitlab::Throttle.authenticated_files_api_options
} }
end end
...@@ -84,6 +91,15 @@ RSpec.describe Gitlab::RackAttack, :aggregate_failures do ...@@ -84,6 +91,15 @@ RSpec.describe Gitlab::RackAttack, :aggregate_failures do
end end
end end
it 'enables dry-runs for `throttle_unauthenticated_api` and `throttle_unauthenticated_web` when selecting `throttle_unauthenticated`' do
stub_env('GITLAB_THROTTLE_DRY_RUN', 'throttle_unauthenticated')
described_class.configure(fake_rack_attack)
expect(fake_rack_attack).to have_received(:track).with('throttle_unauthenticated_api', throttles[:throttle_unauthenticated_api])
expect(fake_rack_attack).to have_received(:track).with('throttle_unauthenticated_web', throttles[:throttle_unauthenticated_web])
end
context 'user allowlist' do context 'user allowlist' do
subject { described_class.user_allowlist } subject { described_class.user_allowlist }
......
This diff is collapsed.
...@@ -388,3 +388,194 @@ RSpec.shared_examples 'tracking when dry-run mode is set' do ...@@ -388,3 +388,194 @@ RSpec.shared_examples 'tracking when dry-run mode is set' do
end end
end end
end end
# Requires let variables:
# * throttle_name: "throttle_unauthenticated_api", "throttle_unauthenticated_web"
# * throttle_setting_prefix: "throttle_unauthenticated_api", "throttle_unauthenticated"
# * url_that_does_not_require_authentication
# * url_that_is_not_matched
# * requests_per_period
# * period_in_seconds
# * period
RSpec.shared_examples 'rate-limited unauthenticated requests' do
before do
# Set low limits
settings_to_set[:"#{throttle_setting_prefix}_requests_per_period"] = requests_per_period
settings_to_set[:"#{throttle_setting_prefix}_period_in_seconds"] = period_in_seconds
end
context 'when the throttle is enabled' do
before do
settings_to_set[:"#{throttle_setting_prefix}_enabled"] = true
stub_application_setting(settings_to_set)
end
it 'rejects requests over the rate limit' do
# At first, allow requests under the rate limit.
requests_per_period.times do
get url_that_does_not_require_authentication
expect(response).to have_gitlab_http_status(:ok)
end
# the last straw
expect_rejection { get url_that_does_not_require_authentication }
end
context 'with custom response text' do
before do
stub_application_setting(rate_limiting_response_text: 'Custom response')
end
it 'rejects requests over the rate limit' do
# At first, allow requests under the rate limit.
requests_per_period.times do
get url_that_does_not_require_authentication
expect(response).to have_gitlab_http_status(:ok)
end
# the last straw
expect_rejection { get url_that_does_not_require_authentication }
expect(response.body).to eq("Custom response\n")
end
end
it 'allows requests after throttling and then waiting for the next period' do
requests_per_period.times do
get url_that_does_not_require_authentication
expect(response).to have_gitlab_http_status(:ok)
end
expect_rejection { get url_that_does_not_require_authentication }
travel_to(period.from_now) do
requests_per_period.times do
get url_that_does_not_require_authentication
expect(response).to have_gitlab_http_status(:ok)
end
expect_rejection { get url_that_does_not_require_authentication }
end
end
it 'counts requests from different IPs separately' do
requests_per_period.times do
get url_that_does_not_require_authentication
expect(response).to have_gitlab_http_status(:ok)
end
expect_next_instance_of(Rack::Attack::Request) do |instance|
expect(instance).to receive(:ip).at_least(:once).and_return('1.2.3.4')
end
# would be over limit for the same IP
get url_that_does_not_require_authentication
expect(response).to have_gitlab_http_status(:ok)
end
context 'when the request is not matched by the throttle' do
it 'does not throttle the requests' do
(1 + requests_per_period).times do
get url_that_is_not_matched
expect(response).to have_gitlab_http_status(:ok)
end
end
end
context 'when the request is to the api internal endpoints' do
it 'allows requests over the rate limit' do
(1 + requests_per_period).times do
get '/api/v4/internal/check', params: { secret_token: Gitlab::Shell.secret_token }
expect(response).to have_gitlab_http_status(:ok)
end
end
end
context 'when the request is authenticated by a runner token' do
let(:request_jobs_url) { '/api/v4/jobs/request' }
let(:runner) { create(:ci_runner) }
it 'does not count as unauthenticated' do
(1 + requests_per_period).times do
post request_jobs_url, params: { token: runner.token }
expect(response).to have_gitlab_http_status(:no_content)
end
end
end
context 'when the request is to a health endpoint' do
let(:health_endpoint) { '/-/metrics' }
it 'does not throttle the requests' do
(1 + requests_per_period).times do
get health_endpoint
expect(response).to have_gitlab_http_status(:ok)
end
end
end
context 'when the request is to a container registry notification endpoint' do
let(:secret_token) { 'secret_token' }
let(:events) { [{ action: 'push' }] }
let(:registry_endpoint) { '/api/v4/container_registry_event/events' }
let(:registry_headers) { { 'Content-Type' => ::API::ContainerRegistryEvent::DOCKER_DISTRIBUTION_EVENTS_V1_JSON } }
before do
allow(Gitlab.config.registry).to receive(:notification_secret) { secret_token }
event = spy(:event)
allow(::ContainerRegistry::Event).to receive(:new).and_return(event)
allow(event).to receive(:supported?).and_return(true)
end
it 'does not throttle the requests' do
(1 + requests_per_period).times do
post registry_endpoint,
params: { events: events }.to_json,
headers: registry_headers.merge('Authorization' => secret_token)
expect(response).to have_gitlab_http_status(:ok)
end
end
end
it 'logs RackAttack info into structured logs' do
requests_per_period.times do
get url_that_does_not_require_authentication
expect(response).to have_gitlab_http_status(:ok)
end
arguments = a_hash_including({
message: 'Rack_Attack',
env: :throttle,
remote_ip: '127.0.0.1',
request_method: 'GET',
path: url_that_does_not_require_authentication,
matched: throttle_name
})
expect(Gitlab::AuthLogger).to receive(:error).with(arguments)
get url_that_does_not_require_authentication
end
it_behaves_like 'tracking when dry-run mode is set' do
def do_request
get url_that_does_not_require_authentication
end
end
end
context 'when the throttle is disabled' do
before do
settings_to_set[:"#{throttle_setting_prefix}_enabled"] = false
stub_application_setting(settings_to_set)
end
it 'allows requests over the rate limit' do
(1 + requests_per_period).times do
get url_that_does_not_require_authentication
expect(response).to have_gitlab_http_status(:ok)
end
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