Merge branch '335300-rate-limit-for-unauthenticated-api-requests-3-rack-attack' into 'master'

[3/5] Apply throttling settings for unauthenticated API requests

See merge request gitlab-org/gitlab!69388

lib/gitlab/rack_attack.rb

@@ -82,8 +82,14 @@ module Gitlab
     end
 
     def self.configure_throttles(rack_attack)
-      throttle_or_track(rack_attack, 'throttle_unauthenticated', Gitlab::Throttle.unauthenticated_options) do |req|
-        if req.throttle_unauthenticated?
+      throttle_or_track(rack_attack, 'throttle_unauthenticated_api', Gitlab::Throttle.unauthenticated_api_options) do |req|
+        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
         end
       end
@@ -177,7 +183,15 @@ module Gitlab
       return false if dry_run_config.empty?
       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
 
     def self.user_allowlist

lib/gitlab/rack_attack/request.rb

@@ -60,10 +60,19 @@ module Gitlab
         path =~ protected_paths_regex
       end
 
-      def throttle_unauthenticated?
+      def throttle_unauthenticated_api?
+        api_request? &&
         !should_be_skipped? &&
         !throttle_unauthenticated_packages_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 &&
         unauthenticated?
       end

lib/gitlab/throttle.rb

@@ -24,7 +24,14 @@ module Gitlab
       "HTTP_#{env_value.upcase.tr('-', '_')}"
     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 }
       period_proc = proc { |req| settings.throttle_unauthenticated_period_in_seconds.seconds }
       { limit: limit_proc, period: period_proc }

spec/lib/gitlab/rack_attack_spec.rb

@@ -10,12 +10,19 @@ RSpec.describe Gitlab::RackAttack, :aggregate_failures 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_product_analytics_collector: { limit: 100, period: 60 },
-        throttle_unauthenticated_protected_paths: Gitlab::Throttle.unauthenticated_options,
-        throttle_authenticated_protected_paths_api: Gitlab::Throttle.authenticated_api_options,
-        throttle_authenticated_protected_paths_web: Gitlab::Throttle.authenticated_web_options
+        throttle_authenticated_web: Gitlab::Throttle.authenticated_web_options,
+        throttle_unauthenticated_protected_paths: Gitlab::Throttle.protected_paths_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
 
@@ -84,6 +91,15 @@ RSpec.describe Gitlab::RackAttack, :aggregate_failures do
       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
       subject { described_class.user_allowlist }
 

spec/requests/rack_attack_global_spec.rb

@@ -11,6 +11,8 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
   # the right settings are being exercised
   let(:settings_to_set) do
     {
+      throttle_unauthenticated_api_requests_per_period: 100,
+      throttle_unauthenticated_api_period_in_seconds: 1,
       throttle_unauthenticated_requests_per_period: 100,
       throttle_unauthenticated_period_in_seconds: 1,
       throttle_authenticated_api_requests_per_period: 100,
@@ -39,186 +41,21 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
 
   include_context 'rack attack cache store'
 
-  describe 'unauthenticated requests' do
-    let(:url_that_does_not_require_authentication) { '/users/sign_in' }
-    let(:url_api_internal) { '/api/v4/internal/check' }
-
-    before do
-      # Disabling protected paths throttle, otherwise requests to
-      # '/users/sign_in' are caught by this throttle.
-      settings_to_set[:throttle_protected_paths_enabled] = false
-
-      # Set low limits
-      settings_to_set[:throttle_unauthenticated_requests_per_period] = requests_per_period
-      settings_to_set[:throttle_unauthenticated_period_in_seconds] = period_in_seconds
-    end
-
-    context 'when the throttle is enabled' do
-      before do
-        settings_to_set[:throttle_unauthenticated_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 to the api internal endpoints' do
-        it 'allows requests over the rate limit' do
-          (1 + requests_per_period).times do
-            get url_api_internal, 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: '/users/sign_in',
-          matched: 'throttle_unauthenticated'
-        })
-
-        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
-        let(:throttle_name) { 'throttle_unauthenticated' }
-
-        def do_request
-          get url_that_does_not_require_authentication
-        end
-      end
+  describe 'unauthenticated API requests' do
+    it_behaves_like 'rate-limited unauthenticated requests' do
+      let(:throttle_name) { 'throttle_unauthenticated_api' }
+      let(:throttle_setting_prefix) { 'throttle_unauthenticated_api' }
+      let(:url_that_does_not_require_authentication) { '/api/v4/projects' }
+      let(:url_that_is_not_matched) { '/users/sign_in' }
     end
+  end
 
-    context 'when the throttle is disabled' do
-      before do
-        settings_to_set[:throttle_unauthenticated_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
+  describe 'unauthenticated web requests' do
+    it_behaves_like 'rate-limited unauthenticated requests' do
+      let(:throttle_name) { 'throttle_unauthenticated_web' }
+      let(:throttle_setting_prefix) { 'throttle_unauthenticated' }
+      let(:url_that_does_not_require_authentication) { '/users/sign_in' }
+      let(:url_that_is_not_matched) { '/api/v4/projects' }
     end
   end
 
@@ -479,9 +316,9 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
 
         context 'when unauthenticated api throttle is enabled' do
           before do
-            settings_to_set[:throttle_unauthenticated_requests_per_period] = requests_per_period
-            settings_to_set[:throttle_unauthenticated_period_in_seconds] = period_in_seconds
-            settings_to_set[:throttle_unauthenticated_enabled] = true
+            settings_to_set[:throttle_unauthenticated_api_requests_per_period] = requests_per_period
+            settings_to_set[:throttle_unauthenticated_api_period_in_seconds] = period_in_seconds
+            settings_to_set[:throttle_unauthenticated_api_enabled] = true
             stub_application_setting(settings_to_set)
           end
 
@@ -494,6 +331,22 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
             expect_rejection { do_request }
           end
         end
+
+        context 'when unauthenticated web throttle is enabled' do
+          before do
+            settings_to_set[:throttle_unauthenticated_web_requests_per_period] = requests_per_period
+            settings_to_set[:throttle_unauthenticated_web_period_in_seconds] = period_in_seconds
+            settings_to_set[:throttle_unauthenticated_web_enabled] = true
+            stub_application_setting(settings_to_set)
+          end
+
+          it 'ignores unauthenticated web throttle' do
+            (1 + requests_per_period).times do
+              do_request
+              expect(response).to have_gitlab_http_status(:ok)
+            end
+          end
+        end
       end
 
       context 'when unauthenticated packages api throttle is enabled' do
@@ -515,9 +368,9 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
 
         context 'when unauthenticated api throttle is lower' do
           before do
-            settings_to_set[:throttle_unauthenticated_requests_per_period] = 0
-            settings_to_set[:throttle_unauthenticated_period_in_seconds] = period_in_seconds
-            settings_to_set[:throttle_unauthenticated_enabled] = true
+            settings_to_set[:throttle_unauthenticated_api_requests_per_period] = 0
+            settings_to_set[:throttle_unauthenticated_api_period_in_seconds] = period_in_seconds
+            settings_to_set[:throttle_unauthenticated_api_enabled] = true
             stub_application_setting(settings_to_set)
           end
 
@@ -748,9 +601,9 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
 
         context 'when unauthenticated api throttle is enabled' do
           before do
-            settings_to_set[:throttle_unauthenticated_requests_per_period] = requests_per_period
-            settings_to_set[:throttle_unauthenticated_period_in_seconds] = period_in_seconds
-            settings_to_set[:throttle_unauthenticated_enabled] = true
+            settings_to_set[:throttle_unauthenticated_api_requests_per_period] = requests_per_period
+            settings_to_set[:throttle_unauthenticated_api_period_in_seconds] = period_in_seconds
+            settings_to_set[:throttle_unauthenticated_api_enabled] = true
             stub_application_setting(settings_to_set)
           end
 
@@ -763,6 +616,22 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
             expect_rejection { do_request }
           end
         end
+
+        context 'when unauthenticated web throttle is enabled' do
+          before do
+            settings_to_set[:throttle_unauthenticated_web_requests_per_period] = requests_per_period
+            settings_to_set[:throttle_unauthenticated_web_period_in_seconds] = period_in_seconds
+            settings_to_set[:throttle_unauthenticated_web_enabled] = true
+            stub_application_setting(settings_to_set)
+          end
+
+          it 'ignores unauthenticated web throttle' do
+            (1 + requests_per_period).times do
+              do_request
+              expect(response).to have_gitlab_http_status(:ok)
+            end
+          end
+        end
       end
 
       context 'when unauthenticated files api throttle is enabled' do
@@ -797,9 +666,9 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
 
         context 'when unauthenticated api throttle is lower' do
           before do
-            settings_to_set[:throttle_unauthenticated_requests_per_period] = 0
-            settings_to_set[:throttle_unauthenticated_period_in_seconds] = period_in_seconds
-            settings_to_set[:throttle_unauthenticated_enabled] = true
+            settings_to_set[:throttle_unauthenticated_api_requests_per_period] = 0
+            settings_to_set[:throttle_unauthenticated_api_period_in_seconds] = period_in_seconds
+            settings_to_set[:throttle_unauthenticated_api_enabled] = true
             stub_application_setting(settings_to_set)
           end
 

spec/support/shared_examples/requests/rack_attack_shared_examples.rb

@@ -388,3 +388,194 @@ RSpec.shared_examples 'tracking when dry-run mode is set' do
     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