From 61bc00cd8cd1999cf0cd090fa39ba2b057a50e3e Mon Sep 17 00:00:00 2001 From: Manoj M J Date: Fri, 17 Feb 2023 14:46:18 +0530 Subject: [PATCH] Add rate limits for access of Projects API Add rate limits for access of unauthenticated access of Projects API Changelog: added --- app/helpers/application_settings_helper.rb | 3 +- app/models/application_setting.rb | 14 ++--- .../application_setting_implementation.rb | 3 +- .../_projects_api_limits.html.haml | 21 +++++++ .../application_settings/network.html.haml | 3 + ...or_unauthenticated_projects_api_access.yml | 8 +++ ...unauthenticated_to_application_settings.rb | 7 +++ db/schema_migrations/20230217065736 | 1 + db/structure.sql | 1 + doc/api/settings.md | 4 +- .../settings/rate_limit_on_projects_api.md | 33 ++++++++++ lib/api/projects.rb | 8 +++ lib/gitlab/application_rate_limiter.rb | 5 +- locale/gitlab.pot | 9 +++ spec/features/admin/admin_settings_spec.rb | 12 ++++ spec/models/application_setting_spec.rb | 3 +- spec/requests/api/projects_spec.rb | 60 +++++++++++++++++++ spec/requests/api/settings_spec.rb | 5 +- .../network.html.haml_spec.rb | 33 ++++++++++ 19 files changed, 219 insertions(+), 14 deletions(-) create mode 100644 app/views/admin/application_settings/_projects_api_limits.html.haml create mode 100644 config/feature_flags/development/rate_limit_for_unauthenticated_projects_api_access.yml create mode 100644 db/migrate/20230217065736_add_projects_api_rate_limit_unauthenticated_to_application_settings.rb create mode 100644 db/schema_migrations/20230217065736 create mode 100644 doc/user/admin_area/settings/rate_limit_on_projects_api.md create mode 100644 spec/views/admin/application_settings/network.html.haml_spec.rb diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 59ba5fa65d396b..adb99a523c2b84 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -479,7 +479,8 @@ def visible_attributes :bulk_import_enabled, :allow_runner_registration_token, :user_defaults_to_private_profile, - :deactivation_email_additional_text + :deactivation_email_additional_text, + :projects_api_rate_limit_unauthenticated ].tap do |settings| next if Gitlab.com? diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 5319f4aee77c43..c80ff359a69199 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -563,14 +563,12 @@ def self.kroki_formats_attributes validates :throttle_protected_paths_period_in_seconds end - validates :notes_create_limit, - numericality: { only_integer: true, greater_than_or_equal_to: 0 } - - validates :search_rate_limit, - numericality: { only_integer: true, greater_than_or_equal_to: 0 } - - validates :search_rate_limit_unauthenticated, - numericality: { only_integer: true, greater_than_or_equal_to: 0 } + with_options(numericality: { only_integer: true, greater_than_or_equal_to: 0 }) do + validates :notes_create_limit + validates :search_rate_limit + validates :search_rate_limit_unauthenticated + validates :projects_api_rate_limit_unauthenticated + end validates :notes_create_limit_allowlist, length: { maximum: 100, message: N_('is too long (maximum is 100 entries)') }, diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index a5f262f2e1ed05..4e2ec0d4ecd2c3 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -249,7 +249,8 @@ def defaults # rubocop:disable Metrics/AbcSize can_create_group: true, bulk_import_enabled: false, allow_runner_registration_token: true, - user_defaults_to_private_profile: false + user_defaults_to_private_profile: false, + projects_api_rate_limit_unauthenticated: 400 }.tap do |hsh| hsh.merge!(non_production_defaults) unless Rails.env.production? end diff --git a/app/views/admin/application_settings/_projects_api_limits.html.haml b/app/views/admin/application_settings/_projects_api_limits.html.haml new file mode 100644 index 00000000000000..4efab4d77a9ed8 --- /dev/null +++ b/app/views/admin/application_settings/_projects_api_limits.html.haml @@ -0,0 +1,21 @@ +%section.settings.as-projects-api-limits.no-animate#js-projects-api-limits-settings{ class: ('expanded' if expanded_by_default?) } + .settings-header + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only + = _('Projects API rate limit') + = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do + = expanded_by_default? ? _('Collapse') : _('Expand') + %p + = _('Set the per-IP address rate limit applicable to unauthenticated requests for getting a list of projects via the API.') + = link_to _('Learn more.'), help_page_path('user/admin_area/settings/rate_limit_on_projects_api.md'), target: '_blank', rel: 'noopener noreferrer' + .settings-content + = gitlab_ui_form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-projects-api-limits-settings'), html: { class: 'fieldset-form' } do |f| + = form_errors(@application_setting) + + %fieldset + .form-group + = f.label :projects_api_rate_limit_unauthenticated, _('Maximum requests per 10 minutes per IP address'), class: 'label-bold' + = f.number_field :projects_api_rate_limit_unauthenticated, class: 'form-control gl-form-input' + .form-text.gl-text-gray-600 + = _("Set this number to 0 to disable the limit.") + + = f.submit _('Save changes'), data: { qa_selector: 'save_changes_button' }, pajamas_button: true diff --git a/app/views/admin/application_settings/network.html.haml b/app/views/admin/application_settings/network.html.haml index 779263b439f96b..fe5f0960d01a1d 100644 --- a/app/views/admin/application_settings/network.html.haml +++ b/app/views/admin/application_settings/network.html.haml @@ -146,6 +146,9 @@ .settings-content = render 'users_api_limits' +- if Feature.enabled?(:rate_limit_for_unauthenticated_projects_api_access) + = render 'projects_api_limits' + %section.settings.as-import-export-limits.no-animate#js-import-export-limits-settings{ class: ('expanded' if expanded_by_default?) } .settings-header %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only diff --git a/config/feature_flags/development/rate_limit_for_unauthenticated_projects_api_access.yml b/config/feature_flags/development/rate_limit_for_unauthenticated_projects_api_access.yml new file mode 100644 index 00000000000000..8707cba0676827 --- /dev/null +++ b/config/feature_flags/development/rate_limit_for_unauthenticated_projects_api_access.yml @@ -0,0 +1,8 @@ +--- +name: rate_limit_for_unauthenticated_projects_api_access +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112283 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/391922 +milestone: '15.10' +type: development +group: group::organization +default_enabled: false diff --git a/db/migrate/20230217065736_add_projects_api_rate_limit_unauthenticated_to_application_settings.rb b/db/migrate/20230217065736_add_projects_api_rate_limit_unauthenticated_to_application_settings.rb new file mode 100644 index 00000000000000..f11560c33e9dd4 --- /dev/null +++ b/db/migrate/20230217065736_add_projects_api_rate_limit_unauthenticated_to_application_settings.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddProjectsApiRateLimitUnauthenticatedToApplicationSettings < Gitlab::Database::Migration[2.1] + def change + add_column :application_settings, :projects_api_rate_limit_unauthenticated, :integer, default: 400, null: false + end +end diff --git a/db/schema_migrations/20230217065736 b/db/schema_migrations/20230217065736 new file mode 100644 index 00000000000000..a355b107c40c31 --- /dev/null +++ b/db/schema_migrations/20230217065736 @@ -0,0 +1 @@ +c772f3d2b46d48bfae68f2b420d38851ecea3105029e5154a58bed29359393f2 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 364f697dcebc16..4f255cf4064bd4 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -11727,6 +11727,7 @@ CREATE TABLE application_settings ( git_rate_limit_users_alertlist integer[] DEFAULT '{}'::integer[] NOT NULL, allow_deploy_tokens_and_keys_with_external_authn boolean DEFAULT false NOT NULL, security_policy_global_group_approvers_enabled boolean DEFAULT true NOT NULL, + projects_api_rate_limit_unauthenticated integer DEFAULT 400 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_container_registry_pre_import_tags_rate_positive CHECK ((container_registry_pre_import_tags_rate >= (0)::numeric)), CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)), diff --git a/doc/api/settings.md b/doc/api/settings.md index 5c7890fef2372a..2b7bb3ab626bef 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -227,7 +227,8 @@ Example response: "can_create_group": false, "jira_connect_application_key": "123", "jira_connect_proxy_url": "http://gitlab.example.com", - "user_defaults_to_private_profile": true + "user_defaults_to_private_profile": true, + "projects_api_rate_limit_unauthenticated": 400 } ``` @@ -435,6 +436,7 @@ listed in the descriptions of the relevant settings. | `plantuml_url` | string | required by: `plantuml_enabled` | The PlantUML instance URL for integration. | | `polling_interval_multiplier` | decimal | no | Interval multiplier used by endpoints that perform polling. Set to `0` to disable polling. | | `project_export_enabled` | boolean | no | Enable project export. | +| `projects_api_rate_limit_unauthenticated` | integer | no | [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112283) in GitLab 15.10. Max number of requests per 10 minutes per IP address for unauthenticated requests to the [list all projects API](projects.md#list-all-projects). Default: 400. To disable throttling set to 0.| | `prometheus_metrics_enabled` | boolean | no | Enable Prometheus metrics. | | `protected_ci_variables` | boolean | no | CI/CD variables are protected by default. | | `push_event_activities_limit` | integer | no | Number of changes (branches or tags) in a single push to determine whether individual push events or bulk push events are created. [Bulk push events are created](../user/admin_area/settings/push_event_activities_limit.md) if it surpasses that value. | diff --git a/doc/user/admin_area/settings/rate_limit_on_projects_api.md b/doc/user/admin_area/settings/rate_limit_on_projects_api.md new file mode 100644 index 00000000000000..eb4066fed00b52 --- /dev/null +++ b/doc/user/admin_area/settings/rate_limit_on_projects_api.md @@ -0,0 +1,33 @@ +--- +type: reference +stage: Manage +group: Organization +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +--- + +# Rate limit on Projects API **(FREE SELF)** + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112283) in GitLab 15.10 behind a feature flag, disabled by default. + +You can configure the rate limit per IP address for unauthenticated requests to the [list all projects API](../../../api/projects.md#list-all-projects). + +To change the rate limit: + +1. On the top bar, select **Main menu > Admin**. +1. On the left sidebar, select **Settings > Network**. +1. Expand **Projects API rate limit**. +1. In the **Maximum requests per 10 minutes per IP address** text box, enter the new value. +1. Select **Save changes**. + +The rate limit: + +- Applies per IP address. +- Doesn't apply to authenticated requests. +- Can be set to 0 to disable rate limiting. + +The default value of the rate limit is `400`. + +Requests over the rate limit are logged into the `auth.log` file. + +For example, if you set a limit of 400, unauthenticated requests to the `GET /projects` API endpoint that +exceed a rate of 400 within 10 minutes are blocked. Access to the endpoint is restored after ten minutes have elapsed. diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 6eea56ea117f72..fbe30aad120caa 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -94,6 +94,12 @@ def log_if_upload_exceed_max_size(user_project, file) Gitlab::AppLogger.info({ message: "File exceeds maximum size", file_bytes: file.size, project_id: user_project.id, project_path: user_project.full_path, upload_allowed: allowed }) end end + + def validate_projects_api_rate_limit_for_unauthenticated_users! + return unless Feature.enabled?(:rate_limit_for_unauthenticated_projects_api_access) + + check_rate_limit!(:projects_api_rate_limit_unauthenticated, scope: [ip_address]) if current_user.blank? + end end helpers do @@ -266,6 +272,8 @@ def add_import_params(params) end # TODO: Set higher urgency https://gitlab.com/gitlab-org/gitlab/-/issues/211495 get feature_category: :projects, urgency: :low do + validate_projects_api_rate_limit_for_unauthenticated_users! + present_projects load_projects end diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index 466538df56e890..8c1cd7c21c2f8e 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -56,7 +56,10 @@ def rate_limits # rubocop:disable Metrics/AbcSize namespace_exists: { threshold: 20, interval: 1.minute }, fetch_google_ip_list: { threshold: 10, interval: 1.minute }, jobs_index: { threshold: 600, interval: 1.minute }, - bulk_import: { threshold: 6, interval: 1.minute } + bulk_import: { threshold: 6, interval: 1.minute }, + projects_api_rate_limit_unauthenticated: { + threshold: -> { application_settings.projects_api_rate_limit_unauthenticated }, interval: 10.minutes + } }.freeze end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index c215985ce9ec8f..8b4ac533b28a4d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -26298,6 +26298,9 @@ msgstr "" msgid "Maximum push size (MB)" msgstr "" +msgid "Maximum requests per 10 minutes per IP address" +msgstr "" + msgid "Maximum requests per 10 minutes per user" msgstr "" @@ -34261,6 +34264,9 @@ msgstr "" msgid "Projects API" msgstr "" +msgid "Projects API rate limit" +msgstr "" + msgid "Projects Successfully Retrieved" msgstr "" @@ -39733,6 +39739,9 @@ msgstr "" msgid "Set the milestone to %{milestone_reference}." msgstr "" +msgid "Set the per-IP address rate limit applicable to unauthenticated requests for getting a list of projects via the API." +msgstr "" + msgid "Set the per-user rate limit for getting a user by ID via the API." msgstr "" diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 26efed8551360c..740ac202011e8a 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -762,6 +762,18 @@ expect(current_settings.users_get_by_id_limit_allowlist).to eq(%w[someone someone_else]) end + it 'changes Projects API rate limits settings' do + visit network_admin_application_settings_path + + page.within('.as-projects-api-limits') do + fill_in 'Maximum requests per 10 minutes per IP address', with: 100 + click_button 'Save changes' + end + + expect(page).to have_content "Application settings saved successfully" + expect(current_settings.projects_api_rate_limit_unauthenticated).to eq(100) + end + shared_examples 'regular throttle rate limit settings' do it 'changes rate limit settings' do visit network_admin_application_settings_path diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 762b613a585746..8de9932a51964f 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -182,7 +182,8 @@ 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 search_rate_limit search_rate_limit_unauthenticated users_get_by_id_limit].each do |setting| + %i[notes_create_limit search_rate_limit search_rate_limit_unauthenticated users_get_by_id_limit + projects_api_rate_limit_unauthenticated].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) } diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index e78ef2f763028d..de30c6ff420cb7 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1109,6 +1109,66 @@ end.not_to exceed_query_limit(control) end end + + context 'rate limiting' do + let_it_be(:current_user) { create(:user) } + + shared_examples_for 'does not log request and does not block the request' do + specify do + request + request + + expect(response).not_to have_gitlab_http_status(:too_many_requests) + expect(Gitlab::AuthLogger).not_to receive(:error) + end + end + + before do + stub_application_setting(projects_api_rate_limit_unauthenticated: 1) + end + + context 'when the user is signed in' do + it_behaves_like 'does not log request and does not block the request' do + def request + get api('/projects', current_user) + end + end + end + + context 'when the user is not signed in' do + let_it_be(:current_user) { nil } + + it_behaves_like 'rate limited endpoint', rate_limit_key: :projects_api_rate_limit_unauthenticated do + def request + get api('/projects', current_user) + end + end + end + + context 'when the feature flag `rate_limit_for_unauthenticated_projects_api_access` is disabled' do + before do + stub_feature_flags(rate_limit_for_unauthenticated_projects_api_access: false) + end + + context 'when the user is not signed in' do + let_it_be(:current_user) { nil } + + it_behaves_like 'does not log request and does not block the request' do + def request + get api('/projects', current_user) + end + end + end + + context 'when the user is signed in' do + it_behaves_like 'does not log request and does not block the request' do + def request + get api('/projects', current_user) + end + end + end + end + end end describe 'POST /projects' do diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index cc377e94038e3d..49403cf41424a8 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -67,6 +67,7 @@ expect(json_response['jira_connect_proxy_url']).to eq(nil) expect(json_response['user_defaults_to_private_profile']).to eq(false) expect(json_response['default_syntax_highlighting_theme']).to eq(1) + expect(json_response['projects_api_rate_limit_unauthenticated']).to eq(400) end end @@ -171,7 +172,8 @@ bulk_import_enabled: false, allow_runner_registration_token: true, user_defaults_to_private_profile: true, - default_syntax_highlighting_theme: 2 + default_syntax_highlighting_theme: 2, + projects_api_rate_limit_unauthenticated: 100 } expect(response).to have_gitlab_http_status(:ok) @@ -240,6 +242,7 @@ expect(json_response['allow_runner_registration_token']).to be(true) expect(json_response['user_defaults_to_private_profile']).to be(true) expect(json_response['default_syntax_highlighting_theme']).to eq(2) + expect(json_response['projects_api_rate_limit_unauthenticated']).to be(100) end end diff --git a/spec/views/admin/application_settings/network.html.haml_spec.rb b/spec/views/admin/application_settings/network.html.haml_spec.rb new file mode 100644 index 00000000000000..3df55be92d5d9d --- /dev/null +++ b/spec/views/admin/application_settings/network.html.haml_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'admin/application_settings/network.html.haml', feature_category: :projects do + let_it_be(:admin) { build_stubbed(:admin) } + let_it_be(:application_setting) { build(:application_setting) } + + before do + assign(:application_setting, application_setting) + allow(view).to receive(:current_user) { admin } + end + + context 'for Projects API rate limit' do + it 'renders the `projects_api_rate_limit_unauthenticated` field' do + render + + expect(rendered).to have_field('application_setting_projects_api_rate_limit_unauthenticated') + end + + context 'when the feature flag `rate_limit_for_unauthenticated_projects_api_access` is turned off' do + before do + stub_feature_flags(rate_limit_for_unauthenticated_projects_api_access: false) + end + + it 'does not render the `projects_api_rate_limit_unauthenticated` field' do + render + + expect(rendered).not_to have_field('application_setting_projects_api_rate_limit_unauthenticated') + end + end + end +end -- GitLab