Make rate limiting of /users/:id configurable

In order to better discriminate between short bursts of legitimate
requests and sustained misuse such as user enumeration attacks, we
increase both the rate limit and the interval to 300 per 10 minutes
(instead of 10 per minute).

Additionally, the limit is now configurable in `ApplicationSetting`, so
it can be set per-instance. This is important also in order to avoid
hitting the limit on staging when running tests.

Enable changing the limit via UI or API
Admin users can set the rate limit via the UI (under Admin Area >
Settings > Network) or via the `/application/settings` API.

Allow configuring a user allowlist

Changelog: changed

app/helpers/application_settings_helper.rb

@@ -423,7 +423,9 @@ module ApplicationSettingsHelper
       :sidekiq_job_limiter_compression_threshold_bytes,
       :sidekiq_job_limiter_limit_bytes,
       :suggest_pipeline_enabled,
-      :user_email_lookup_limit
+      :user_email_lookup_limit,
+      :users_get_by_id_limit,
+      :users_get_by_id_limit_allowlist_raw
     ].tap do |settings|
       settings << :deactivate_dormant_users unless Gitlab.com?
     end

app/models/application_setting.rb

@@ -563,6 +563,12 @@ class ApplicationSetting < ApplicationRecord
     presence: true, length: { maximum: 255 },
     if: :sentry_enabled?
 
+  validates :users_get_by_id_limit,
+    numericality: { only_integer: true, greater_than_or_equal_to: 0 }
+  validates :users_get_by_id_limit_allowlist,
+          length: { maximum: 100, message: N_('is too long (maximum is 100 entries)') },
+          allow_nil: false
+
   attr_encrypted :asset_proxy_secret_key,
                  mode: :per_attribute_iv,
                  key: Settings.attr_encrypted_db_key_base_truncated,

app/models/application_setting_implementation.rb

@@ -231,7 +231,9 @@ module ApplicationSettingImplementation
         rate_limiting_response_text: nil,
         whats_new_variant: 0,
         user_deactivation_emails_enabled: true,
-        user_email_lookup_limit: 60
+        user_email_lookup_limit: 60,
+        users_get_by_id_limit: 300,
+        users_get_by_id_limit_allowlist: []
       }
     end
 
@@ -334,6 +336,14 @@ module ApplicationSettingImplementation
     self.notes_create_limit_allowlist = strings_to_array(values).map(&:downcase)
   end
 
+  def users_get_by_id_limit_allowlist_raw
+    array_to_string(self.users_get_by_id_limit_allowlist)
+  end
+
+  def users_get_by_id_limit_allowlist_raw=(values)
+    self.users_get_by_id_limit_allowlist = strings_to_array(values).map(&:downcase)
+  end
+
   def asset_proxy_whitelist=(values)
     values = strings_to_array(values) if values.is_a?(String)
 

app/models/instance_configuration.rb

@@ -118,7 +118,12 @@ class InstanceConfiguration
       group_export_download: application_setting_limit_per_minute(:group_download_export_limit),
       group_import: application_setting_limit_per_minute(:group_import_limit),
       raw_blob: application_setting_limit_per_minute(:raw_blob_request_limit),
-      user_email_lookup: application_setting_limit_per_minute(:user_email_lookup_limit)
+      user_email_lookup: application_setting_limit_per_minute(:user_email_lookup_limit),
+      users_get_by_id: {
+        enabled: application_settings[:users_get_by_id_limit] > 0,
+        requests_per_period: application_settings[:users_get_by_id_limit],
+        period_in_seconds: 10.minutes
+      }
     }
   end
 

app/views/admin/application_settings/_note_limits.html.haml

@@ -9,7 +9,7 @@
       = f.label :notes_create_limit_allowlist, _('Users to exclude from the rate limit'), class: 'label-bold'
       = f.text_area :notes_create_limit_allowlist_raw, placeholder: 'username1, username2', class: 'form-control gl-form-input', rows: 5
       .form-text.text-muted
-        = _('Comma-separated list of users allowed to exceed the rate limit.')
+        = _('List of users allowed to exceed the rate limit.')
 
 
   = f.submit _('Save changes'), class: "gl-button btn btn-confirm", data: { qa_selector: 'save_changes_button' }

app/views/admin/application_settings/_users_api_limits.html.haml

+= form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-users-api-limits-settings'), html: { class: 'fieldset-form' } do |f|
+  = form_errors(@application_setting)
+
+  %fieldset
+    .form-group
+      = f.label :users_get_by_id_limit, _('Maximum requests per 10 minutes per user'), class: 'label-bold'
+      = f.number_field :users_get_by_id_limit, class: 'form-control gl-form-input'
+    .form-group
+      = f.label :users_get_by_id_limit_allowlist_raw, _('Users to exclude from the rate limit'), class: 'label-bold'
+      = f.text_area :users_get_by_id_limit_allowlist_raw, placeholder: 'username1, username2', class: 'form-control gl-form-input', rows: 5
+      .form-text.text-muted
+        = _('List of users allowed to exceed the rate limit.')
+
+  = f.submit _('Save changes'), class: "gl-button btn btn-confirm", data: { qa_selector: 'save_changes_button' }

app/views/admin/application_settings/network.html.haml

@@ -122,6 +122,18 @@
   .settings-content
     = render 'note_limits'
 
+%section.settings.as-users-api-limits.no-animate#js-users-api-limits-settings{ class: ('expanded' if expanded_by_default?) }
+  .settings-header
+    %h4
+      = _('Users API rate limit')
+    %button.btn.gl-button.btn-default.js-settings-toggle{ type: 'button' }
+      = expanded_by_default? ? _('Collapse') : _('Expand')
+    %p
+      = _('Set the per-user rate limit for getting a user by ID via the API.')
+      = link_to _('Learn more.'), help_page_path('user/admin_area/settings/rate_limit_on_users_api.md'), target: '_blank', rel: 'noopener noreferrer'
+  .settings-content
+    = render 'users_api_limits'
+
 %section.settings.as-import-export-limits.no-animate#js-import-export-limits-settings{ class: ('expanded' if expanded_by_default?) }
   .settings-header
     %h4

db/migrate/20220207083129_add_users_get_by_id_limit_to_application_setting.rb

+# frozen_string_literal: true
+
+class AddUsersGetByIdLimitToApplicationSetting < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def up
+    add_column :application_settings, :users_get_by_id_limit, :integer, null: false, default: 300
+    add_column :application_settings, :users_get_by_id_limit_allowlist, :text, array: true, limit: 255, null: false, default: []
+  end
+
+  def down
+    remove_column :application_settings, :users_get_by_id_limit
+    remove_column :application_settings, :users_get_by_id_limit_allowlist
+  end
+end

db/schema_migrations/20220207083129

+01cc0139097235991fa2caf8b780ccd1c3ce580647197418424ade83ce9be77e
\ No newline at end of file

db/structure.sql

@@ -10620,6 +10620,8 @@ CREATE TABLE application_settings (
     project_runner_token_expiration_interval integer,
     ecdsa_sk_key_restriction integer DEFAULT 0 NOT NULL,
     ed25519_sk_key_restriction integer DEFAULT 0 NOT NULL,
+    users_get_by_id_limit integer DEFAULT 300 NOT NULL,
+    users_get_by_id_limit_allowlist text[] DEFAULT '{}'::text[] NOT NULL,
     CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)),
     CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)),
     CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)),

doc/user/admin_area/settings/index.md

@@ -137,6 +137,7 @@ The **Network** settings contain:
 - [Incident Management Limits](../../../operations/incident_management/index.md) - Limit the
   number of inbound alerts that can be sent to a project.
 - [Notes creation limit](rate_limit_on_notes_creation.md) - Set a rate limit on the note creation requests.
+- [Get single user limit](rate_limit_on_users_api.md) - Set a rate limit on users API endpoint to get a user by ID.
 
 ### Preferences
 

doc/user/admin_area/settings/rate_limit_on_users_api.md

+---
+type: reference
+stage: Manage
+group: Authentication & Authorization
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments
+---
+
+# Rate limits on Users API **(FREE SELF)**
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78364) in GitLab 14.8.
+
+You can configure the per-user rate limit for requests to the users API endpoint to get a user by ID.
+
+To change getting a single user rate limit:
+
+1. On the top bar, select **Menu > Admin**.
+1. On the left sidebar, select **Settings > Network**.
+1. Expand **Users API rate limit**.
+1. In the **Maximum requests per 10 minutes** text box, enter the new value.
+1. Optional. In the **Users to exclude from the rate limit** box, list users allowed to exceed the limit.
+1. Select **Save changes**.
+
+This limit is:
+
+- Applied independently per user.
+- Not applied per IP address.
+
+The default value is `300`.
+
+Requests over the rate limit are logged into the `auth.log` file.
+
+For example, if you set a limit of 300, requests to the `GET /users/:id` API endpoint
+exceeding a rate of 300 per 10 minutes are blocked. Access to the endpoint is allowed after ten minutes have elapsed.

lib/api/settings.rb

@@ -177,6 +177,7 @@ module API
       optional :floc_enabled, type: Grape::API::Boolean, desc: 'Enable FloC (Federated Learning of Cohorts)'
       optional :user_deactivation_emails_enabled, type: Boolean, desc: 'Send emails to users upon account deactivation'
       optional :suggest_pipeline_enabled, type: Boolean, desc: 'Enable pipeline suggestion banner'
+      optional :users_get_by_id_limit, type: Integer, desc: "Maximum number of calls to the /users/:id API per 10 minutes per user. Set to 0 for unlimited requests."
 
       ApplicationSetting::SUPPORTED_KEY_TYPES.each do |type|
         optional :"#{type}_key_restriction",

lib/api/users.rb

@@ -143,7 +143,12 @@ module API
         forbidden!('Not authorized!') unless current_user
 
         if Feature.enabled?(:rate_limit_user_by_id_endpoint, type: :development)
-          check_rate_limit! :users_get_by_id, scope: current_user unless current_user.admin?
+          unless current_user.admin?
+            check_rate_limit!(:users_get_by_id,
+              scope: current_user,
+              users_allowlist: Gitlab::CurrentSettings.current_application_settings.users_get_by_id_limit_allowlist
+            )
+          end
         end
 
         user = User.find_by(id: params[:id])

lib/gitlab/application_rate_limiter.rb

@@ -14,7 +14,7 @@ module Gitlab
       # Threshold value can be either an Integer or a Proc
       # in order to not evaluate it's value every time this method is called
       # and only do that when it's needed.
-      def rate_limits
+      def rate_limits # rubocop:disable Metrics/AbcSize
         {
           issues_create:                { threshold: -> { application_settings.issues_create_limit }, interval: 1.minute },
           notes_create:                 { threshold: -> { application_settings.notes_create_limit }, interval: 1.minute },
@@ -32,7 +32,7 @@ module Gitlab
           group_testing_hook:           { threshold: 5, interval: 1.minute },
           profile_add_new_email:        { threshold: 5, interval: 1.minute },
           web_hook_calls:               { interval: 1.minute },
-          users_get_by_id:              { threshold: 10, interval: 1.minute },
+          users_get_by_id:              { threshold: -> { application_settings.users_get_by_id_limit }, interval: 10.minutes },
           username_exists:              { threshold: 20, interval: 1.minute },
           user_sign_up:                 { threshold: 20, interval: 1.minute },
           profile_resend_email_confirmation:  { threshold: 5, interval: 1.minute },

locale/gitlab.pot

@@ -8751,9 +8751,6 @@ msgstr ""
 msgid "Comma-separated list of email addresses."
 msgstr ""
 
-msgid "Comma-separated list of users allowed to exceed the rate limit."
-msgstr ""
-
 msgid "Comma-separated, e.g. '1.1.1.1, 2.2.2.0/24'"
 msgstr ""
 
@@ -21725,6 +21722,9 @@ msgstr ""
 msgid "List of all merge commits"
 msgstr ""
 
+msgid "List of users allowed to exceed the rate limit."
+msgstr ""
+
 msgid "List options"
 msgstr ""
 
@@ -22352,6 +22352,9 @@ msgstr ""
 msgid "Maximum push size (MB)"
 msgstr ""
 
+msgid "Maximum requests per 10 minutes per user"
+msgstr ""
+
 msgid "Maximum requests per minute"
 msgstr ""
 
@@ -33131,6 +33134,9 @@ msgstr ""
 msgid "Set the milestone to %{milestone_reference}."
 msgstr ""
 
+msgid "Set the per-user rate limit for getting a user by ID via the API."
+msgstr ""
+
 msgid "Set the per-user rate limit for notes created by web or API requests."
 msgstr ""
 
@@ -39576,6 +39582,9 @@ msgstr ""
 msgid "Users"
 msgstr ""
 
+msgid "Users API rate limit"
+msgstr ""
+
 msgid "Users can launch a development environment from a GitLab browser tab when the %{linkStart}Gitpod%{linkEnd} integration is enabled."
 msgstr ""
 

spec/features/admin/admin_settings_spec.rb

@@ -631,6 +631,20 @@ RSpec.describe 'Admin updates settings' do
         expect(current_settings.issues_create_limit).to eq(0)
       end
 
+      it 'changes Users API rate limits settings' do
+        visit network_admin_application_settings_path
+
+        page.within('.as-users-api-limits') do
+          fill_in 'Maximum requests per 10 minutes per user', with: 0
+          fill_in 'Users to exclude from the rate limit', with: 'someone, someone_else'
+          click_button 'Save changes'
+        end
+
+        expect(page).to have_content "Application settings saved successfully"
+        expect(current_settings.users_get_by_id_limit).to eq(0)
+        expect(current_settings.users_get_by_id_limit_allowlist).to eq(%w[someone someone_else])
+      end
+
       shared_examples 'regular throttle rate limit settings' do
         it 'changes rate limit settings' do
           visit network_admin_application_settings_path

spec/helpers/application_settings_helper_spec.rb

@@ -46,6 +46,15 @@ RSpec.describe ApplicationSettingsHelper do
       expect(helper.visible_attributes).to include(:deactivate_dormant_users)
     end
 
+    it 'contains rate limit parameters' do
+      expect(helper.visible_attributes).to include(*%i(
+        issues_create_limit notes_create_limit project_export_limit
+        project_download_export_limit project_export_limit project_import_limit
+        raw_blob_request_limit group_export_limit group_download_export_limit
+        group_import_limit users_get_by_id_limit user_email_lookup_limit
+      ))
+    end
+
     context 'when GitLab.com' do
       before do
         allow(Gitlab).to receive(:com?).and_return(true)

spec/models/application_setting_spec.rb

@@ -141,7 +141,7 @@ RSpec.describe ApplicationSetting do
     it { is_expected.not_to allow_value('default' => 101).for(:repository_storages_weighted).with_message("value for 'default' must be between 0 and 100") }
     it { is_expected.not_to allow_value('default' => 100, shouldntexist: 50).for(:repository_storages_weighted).with_message("can't include: shouldntexist") }
 
-    %i[notes_create_limit user_email_lookup_limit].each do |setting|
+    %i[notes_create_limit user_email_lookup_limit users_get_by_id_limit].each do |setting|
       it { is_expected.to allow_value(400).for(setting) }
       it { is_expected.not_to allow_value('two').for(setting) }
       it { is_expected.not_to allow_value(nil).for(setting) }
@@ -158,6 +158,11 @@ RSpec.describe ApplicationSetting do
     it { is_expected.not_to allow_value(nil).for(:notes_create_limit_allowlist) }
     it { is_expected.to allow_value([]).for(:notes_create_limit_allowlist) }
 
+    it { is_expected.to allow_value(many_usernames(100)).for(:users_get_by_id_limit_allowlist) }
+    it { is_expected.not_to allow_value(many_usernames(101)).for(:users_get_by_id_limit_allowlist) }
+    it { is_expected.not_to allow_value(nil).for(:users_get_by_id_limit_allowlist) }
+    it { is_expected.to allow_value([]).for(:users_get_by_id_limit_allowlist) }
+
     it { is_expected.to allow_value('all_tiers').for(:whats_new_variant) }
     it { is_expected.to allow_value('current_tier').for(:whats_new_variant) }
     it { is_expected.to allow_value('disabled').for(:whats_new_variant) }

spec/models/instance_configuration_spec.rb

@@ -206,7 +206,8 @@ RSpec.describe InstanceConfiguration do
             group_download_export_limit: 1019,
             group_import_limit: 1020,
             raw_blob_request_limit: 1021,
-            user_email_lookup_limit: 1022
+            user_email_lookup_limit: 1022,
+            users_get_by_id_limit: 1023
           )
         end
 
@@ -230,6 +231,7 @@ RSpec.describe InstanceConfiguration do
           expect(rate_limits[:group_import]).to eq({ enabled: true, requests_per_period: 1020, period_in_seconds: 60 })
           expect(rate_limits[:raw_blob]).to eq({ enabled: true, requests_per_period: 1021, period_in_seconds: 60 })
           expect(rate_limits[:user_email_lookup]).to eq({ enabled: true, requests_per_period: 1022, period_in_seconds: 60 })
+          expect(rate_limits[:users_get_by_id]).to eq({ enabled: true, requests_per_period: 1023, period_in_seconds: 600 })
         end
       end
     end

spec/requests/api/settings_spec.rb

@@ -141,7 +141,8 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do
             personal_access_token_prefix: "GL-",
             user_deactivation_emails_enabled: false,
             admin_mode: true,
-            suggest_pipeline_enabled: false
+            suggest_pipeline_enabled: false,
+            users_get_by_id_limit: 456
           }
 
         expect(response).to have_gitlab_http_status(:ok)
@@ -196,6 +197,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do
         expect(json_response['admin_mode']).to be(true)
         expect(json_response['user_deactivation_emails_enabled']).to be(false)
         expect(json_response['suggest_pipeline_enabled']).to be(false)
+        expect(json_response['users_get_by_id_limit']).to eq(456)
       end
     end
 

spec/requests/api/users_spec.rb

@@ -499,7 +499,8 @@ RSpec.describe API::Users do
     let_it_be(:user2, reload: true) { create(:user, username: 'another_user') }
 
     before do
-      allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:users_get_by_id, scope: user).and_return(false)
+      allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?)
+        .with(:users_get_by_id, scope: user, users_allowlist: []).and_return(false)
     end
 
     it "returns a user by id" do
@@ -600,7 +601,7 @@ RSpec.describe API::Users do
     context 'when the rate limit is not exceeded' do
       it 'returns a success status' do
         expect(Gitlab::ApplicationRateLimiter)
-          .to receive(:throttled?).with(:users_get_by_id, scope: user)
+          .to receive(:throttled?).with(:users_get_by_id, scope: user, users_allowlist: [])
           .and_return(false)
 
         get api("/users/#{user.id}", user)
@@ -613,7 +614,7 @@ RSpec.describe API::Users do
       context 'when feature flag is enabled' do
         it 'returns "too many requests" status' do
           expect(Gitlab::ApplicationRateLimiter)
-            .to receive(:throttled?).with(:users_get_by_id, scope: user)
+            .to receive(:throttled?).with(:users_get_by_id, scope: user, users_allowlist: [])
             .and_return(true)
 
           get api("/users/#{user.id}", user)
@@ -629,6 +630,24 @@ RSpec.describe API::Users do
 
           expect(response).to have_gitlab_http_status(:ok)
         end
+
+        it 'allows users whose username is in the allowlist' do
+          allowlist = [user.username]
+          current_settings = Gitlab::CurrentSettings.current_application_settings
+
+          # Necessary to ensure the same object is returned on each call
+          allow(Gitlab::CurrentSettings).to receive(:current_application_settings).and_return current_settings
+
+          allow(current_settings).to receive(:users_get_by_id_limit_allowlist).and_return(allowlist)
+
+          expect(Gitlab::ApplicationRateLimiter)
+            .to receive(:throttled?).with(:users_get_by_id, scope: user, users_allowlist: allowlist)
+            .and_call_original
+
+          get api("/users/#{user.id}", user)
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
       end
 
       context 'when feature flag is disabled' do

spec/services/application_settings/update_service_spec.rb

@@ -475,6 +475,24 @@ RSpec.describe ApplicationSettings::UpdateService do
     end
   end
 
+  context 'when users_get_by_id_limit and users_get_by_id_limit_allowlist_raw are passed' do
+    let(:params) do
+      {
+        users_get_by_id_limit: 456,
+        users_get_by_id_limit_allowlist_raw: 'someone, someone_else'
+      }
+    end
+
+    it 'updates users_get_by_id_limit and users_get_by_id_limit_allowlist value' do
+      subject.execute
+
+      application_settings.reload
+
+      expect(application_settings.users_get_by_id_limit).to eq(456)
+      expect(application_settings.users_get_by_id_limit_allowlist).to eq(%w[someone someone_else])
+    end
+  end
+
   context 'when require_admin_approval_after_user_signup changes' do
     context 'when it goes from enabled to disabled' do
       let(:params) { { require_admin_approval_after_user_signup: false } }