diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 3847a35fbab4a66de0ab3d3dee0a5a2adb427e0a..a646b62027a27976f9997195ab8eff5a595fae0b 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -159,7 +159,8 @@ module ApplicationSettingsHelper :after_sign_up_text, :akismet_api_key, :akismet_enabled, - :allow_local_requests_from_hooks_and_services, + :allow_local_requests_from_web_hooks_and_services, + :allow_local_requests_from_system_hooks, :dns_rebinding_protection_enabled, :archive_builds_in_human_readable, :authorized_keys_enabled, diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 4bb09bf3b538a7998fa90d49d8e7c55a0492160d..b7a4d7aa803b25564ef262b20243b417f3e12887 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -21,7 +21,8 @@ module ApplicationSettingImplementation { after_sign_up_text: nil, akismet_enabled: false, - allow_local_requests_from_hooks_and_services: false, + allow_local_requests_from_web_hooks_and_services: false, + allow_local_requests_from_system_hooks: true, dns_rebinding_protection_enabled: true, authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand container_registry_token_expire_delay: 5, diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb index 90b4588a3258de389b87cb327398b91e40be3980..b83913c845f610ff7942f04b2f712b78591d7d04 100644 --- a/app/models/hooks/system_hook.rb +++ b/app/models/hooks/system_hook.rb @@ -14,8 +14,11 @@ class SystemHook < WebHook default_value_for :repository_update_events, true default_value_for :merge_requests_events, false + validates :url, presence: true, public_url: false, system_hook_url: { allow_localhost: lambda(&:allow_local_requests?), + allow_local_network: lambda(&:allow_local_requests?) } + # Allow urls pointing localhost and the local network def allow_local_requests? - true + Gitlab::CurrentSettings.allow_local_requests_from_system_hooks? end end diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index daf7ff4b771ed2a94a9687b781e9e423ba159bce..3203bb024ab401a2bc83d5589e65b2a8af16a478 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -35,6 +35,6 @@ class WebHook < ApplicationRecord # Allow urls pointing localhost and the local network def allow_local_requests? - false + Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? end end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 6d675c026bbe3debb7de288ffe0461ea0f95a610..8c294218708dc06c9b84477f959b30477a37381f 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -17,8 +17,10 @@ class WebHookService @hook = hook @data = data @hook_name = hook_name.to_s - @request_options = { timeout: Gitlab.config.gitlab.webhook_timeout } - @request_options.merge!(allow_local_requests: true) if @hook.is_a?(SystemHook) + @request_options = { + timeout: Gitlab.config.gitlab.webhook_timeout, + allow_local_requests: hook.allow_local_requests? + } end def execute diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb index 273e15ef925f9ae2e88e7fb074ed83d843cffedc..bb445499cee5e07dc1172cfaa7f95c14553569fa 100644 --- a/app/validators/addressable_url_validator.rb +++ b/app/validators/addressable_url_validator.rb @@ -107,6 +107,6 @@ class AddressableUrlValidator < ActiveModel::EachValidator # calls this validator. # # See https://gitlab.com/gitlab-org/gitlab-ee/issues/9833 - ApplicationSetting.current&.allow_local_requests_from_hooks_and_services? + ApplicationSetting.current&.allow_local_requests_from_web_hooks_and_services? end end diff --git a/app/validators/system_hook_url_validator.rb b/app/validators/system_hook_url_validator.rb new file mode 100644 index 0000000000000000000000000000000000000000..c8c0007e35b931a8c0b239637ce1bdd655c0d2d8 --- /dev/null +++ b/app/validators/system_hook_url_validator.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +# SystemHookUrlValidator +# +# Custom validator specifically for SystemHook URLs. This validator works like AddressableUrlValidator but +# it blocks urls pointing to localhost or the local network depending on +# ApplicationSetting.allow_local_requests_from_system_hooks +# +# Example: +# +# class SystemHook < WebHook +# validates :url, system_hook_url: { allow_localhost: true, allow_local_network: true } +# end +# +class SystemHookUrlValidator < AddressableUrlValidator + DEFAULT_OPTIONS = { + allow_localhost: true, + allow_local_network: true + }.freeze + + def initialize(options) + options.reverse_merge!(DEFAULT_OPTIONS) + + super(options) + end + + def self.allow_setting_local_requests? + ApplicationSetting.current&.allow_local_requests_from_system_hooks? + end +end diff --git a/app/views/admin/application_settings/_outbound.html.haml b/app/views/admin/application_settings/_outbound.html.haml index 4fecdb59e1d38c160e15c1a5ab029c138ce4a593..d39e192d6044a5b6b30d6b52b61f03dde08228a1 100644 --- a/app/views/admin/application_settings/_outbound.html.haml +++ b/app/views/admin/application_settings/_outbound.html.haml @@ -4,9 +4,13 @@ %fieldset .form-group .form-check - = f.check_box :allow_local_requests_from_hooks_and_services, class: 'form-check-input' - = f.label :allow_local_requests_from_hooks_and_services, class: 'form-check-label' do - Allow requests to the local network from hooks and services + = f.check_box :allow_local_requests_from_web_hooks_and_services, class: 'form-check-input' + = f.label :allow_local_requests_from_web_hooks_and_services, class: 'form-check-label' do + Allow requests to the local network from web hooks and services + .form-check + = f.check_box :allow_local_requests_from_system_hooks, class: 'form-check-input' + = f.label :allow_local_requests_from_system_hooks, class: 'form-check-label' do + Allow requests to the local network from system hooks .form-group = f.label :outbound_local_requests_whitelist_raw, class: 'label-bold' do diff --git a/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb new file mode 100644 index 0000000000000000000000000000000000000000..f1ba7da9fc7e55096991eb467b13cebb93ebe1da --- /dev/null +++ b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb @@ -0,0 +1,15 @@ +class RenameAllowLocalRequestsFromHooksAndServicesApplicationSetting < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + rename_column :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services + end + + def down + rename_column :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services + end +end diff --git a/db/migrate/20190726101133_add_allow_local_requests_from_system_hooks_to_application_settings.rb b/db/migrate/20190726101133_add_allow_local_requests_from_system_hooks_to_application_settings.rb new file mode 100644 index 0000000000000000000000000000000000000000..ed58d4e57fca5c0ee67a1d95ecac77e52dfe5c20 --- /dev/null +++ b/db/migrate/20190726101133_add_allow_local_requests_from_system_hooks_to_application_settings.rb @@ -0,0 +1,18 @@ +class AddAllowLocalRequestsFromSystemHooksToApplicationSettings < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:application_settings, :allow_local_requests_from_system_hooks, + :boolean, + default: true, + allow_null: false) + end + + def down + remove_column(:application_settings, :allow_local_requests_from_system_hooks) + end +end diff --git a/db/schema.rb b/db/schema.rb index 709f9ce2541f0a67ffd44ef77d8f2a1929d83b21..e2ae13174cd8f528766352315be4e8a97074f706 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -183,7 +183,7 @@ ActiveRecord::Schema.define(version: 2019_07_31_084415) do t.string "external_authorization_service_default_label" t.boolean "pages_domain_verification_enabled", default: true, null: false t.string "user_default_internal_regex" - t.boolean "allow_local_requests_from_hooks_and_services", default: false, null: false + t.boolean "allow_local_requests_from_web_hooks_and_services", default: false, null: false t.float "external_authorization_service_timeout", default: 0.5 t.text "external_auth_client_cert" t.text "encrypted_external_auth_client_key" @@ -230,6 +230,7 @@ ActiveRecord::Schema.define(version: 2019_07_31_084415) do t.string "grafana_url", default: "/-/grafana", null: false t.string "outbound_local_requests_whitelist", limit: 255, default: [], null: false, array: true t.integer "raw_blob_request_limit", default: 300, null: false + t.boolean "allow_local_requests_from_system_hooks", default: true, null: false t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id" t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id" t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id" diff --git a/doc/api/settings.md b/doc/api/settings.md index c3ac70f057929f911614dfc84eb1e5f1acb6b93b..f17a49cfc8904377dbf05ca1aecb4f2d6158b895 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -177,7 +177,8 @@ are listed in the descriptions of the relevant settings. | `akismet_api_key` | string | required by: `akismet_enabled` | API key for akismet spam protection. | | `akismet_enabled` | boolean | no | (**If enabled, requires:** `akismet_api_key`) Enable or disable akismet spam protection. | | `allow_group_owners_to_manage_ldap` | boolean | no | **(PREMIUM)** Set to `true` to allow group owners to manage LDAP | -| `allow_local_requests_from_hooks_and_services` | boolean | no | Allow requests to the local network from hooks and services. | +| `allow_local_requests_from_web_hooks_and_services` | boolean | no | Allow requests to the local network from web hooks and services. | +| `allow_local_requests_from_system_hooks` | boolean | no | Allow requests to the local network from system hooks. | | `authorized_keys_enabled` | boolean | no | By default, we write to the `authorized_keys` file to support Git over SSH without additional configuration. GitLab can be optimized to authenticate SSH keys via the database file. Only disable this if you have configured your OpenSSH server to use the AuthorizedKeysCommand. | | `auto_devops_domain` | string | no | Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages. | | `auto_devops_enabled` | boolean | no | Enable Auto DevOps for projects by default. It will automatically build, test, and deploy applications based on a predefined CI/CD configuration. | diff --git a/lib/gitlab/http_connection_adapter.rb b/lib/gitlab/http_connection_adapter.rb index 41eab3658bcb8cc08e971c3320c31456787718fe..84eb60f3a5db3a216b062820a85ecbd05ce3397e 100644 --- a/lib/gitlab/http_connection_adapter.rb +++ b/lib/gitlab/http_connection_adapter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # This class is part of the Gitlab::HTTP wrapper. Depending on the value -# of the global setting allow_local_requests_from_hooks_and_services this adapter +# of the global setting allow_local_requests_from_web_hooks_and_services this adapter # will allow/block connection to internal IPs and/or urls. # # This functionality can be overridden by providing the setting the option @@ -38,7 +38,7 @@ module Gitlab end def allow_settings_local_requests? - Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? end end end diff --git a/lib/gitlab/kubernetes/kube_client.rb b/lib/gitlab/kubernetes/kube_client.rb index 1350924cd76855639e6509dfaecc52b4bc767b0e..64317225ec6d1312baf83aa6c24e885ca5810ff6 100644 --- a/lib/gitlab/kubernetes/kube_client.rb +++ b/lib/gitlab/kubernetes/kube_client.rb @@ -128,7 +128,7 @@ module Gitlab private def validate_url! - return if Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + return if Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? Gitlab::UrlBlocker.validate!(api_prefix, allow_local_network: false) end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5e9e371a5fcf819e4bfbd6f0a30361ea1d4176a8..cca55aeb8bd01c5121c68fd55215ffaaaaa412fb 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -945,7 +945,10 @@ msgstr "" msgid "Allow rendering of PlantUML diagrams in Asciidoc documents." msgstr "" -msgid "Allow requests to the local network from hooks and services." +msgid "Allow requests to the local network from web hooks and services." +msgstr "" + +msgid "Allow requests to the local network from system hooks." msgstr "" msgid "Allow this key to push to repository as well? (Default only allows pull access.)" diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index c77605f38694030e446013454aa769e4ba5014d2..ddd87404003706af38adc755c0d84d62ea1e89fb 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -338,14 +338,17 @@ describe 'Admin updates settings' do visit network_admin_application_settings_path page.within('.as-outbound') do - check 'Allow requests to the local network from hooks and services' + check 'Allow requests to the local network from web hooks and services' + # Enabled by default + uncheck 'Allow requests to the local network from system hooks' # Enabled by default uncheck 'Enforce DNS rebinding attack protection' click_button 'Save changes' end expect(page).to have_content "Application settings saved successfully" - expect(current_settings.allow_local_requests_from_hooks_and_services).to be true + expect(current_settings.allow_local_requests_from_web_hooks_and_services).to be true + expect(current_settings.allow_local_requests_from_system_hooks).to be false expect(current_settings.dns_rebinding_protection_enabled).to be false end end diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb index 158f77cab2c751cdb7e51962ec0f5a75d770d5fa..d3f9be845dd127e8af5e72d0e08c57baa5a25b8c 100644 --- a/spec/lib/gitlab/http_spec.rb +++ b/spec/lib/gitlab/http_spec.rb @@ -23,14 +23,14 @@ describe Gitlab::HTTP do end end - describe 'allow_local_requests_from_hooks_and_services is' do + describe 'allow_local_requests_from_web_hooks_and_services is' do before do WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success') end context 'disabled' do before do - allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_web_hooks_and_services?).and_return(false) end it 'deny requests to localhost' do @@ -52,7 +52,7 @@ describe Gitlab::HTTP do context 'enabled' do before do - allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_web_hooks_and_services?).and_return(true) end it 'allow requests to localhost' do diff --git a/spec/lib/gitlab/kubernetes/kube_client_spec.rb b/spec/lib/gitlab/kubernetes/kube_client_spec.rb index 97ebb5f1554e5d364f69de5fbe3ebd3b927c11b4..f49d4e23e39736b33e1997ef5084d4221c018a7d 100644 --- a/spec/lib/gitlab/kubernetes/kube_client_spec.rb +++ b/spec/lib/gitlab/kubernetes/kube_client_spec.rb @@ -58,7 +58,7 @@ describe Gitlab::Kubernetes::KubeClient do context 'when local requests are allowed' do before do - stub_application_setting(allow_local_requests_from_hooks_and_services: true) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) end it 'allows local addresses' do diff --git a/spec/models/clusters/platforms/kubernetes_spec.rb b/spec/models/clusters/platforms/kubernetes_spec.rb index 471769e4aaba34cd5046775b638cdfca084301f1..5811016ea4da700376a956e7666c72bff3f48250 100644 --- a/spec/models/clusters/platforms/kubernetes_spec.rb +++ b/spec/models/clusters/platforms/kubernetes_spec.rb @@ -106,7 +106,7 @@ describe Clusters::Platforms::Kubernetes do before do allow(ApplicationSetting) .to receive(:current) - .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: true)) + .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: true)) end it { expect(kubernetes.save).to be_truthy } diff --git a/spec/models/lfs_download_object_spec.rb b/spec/models/lfs_download_object_spec.rb index effd8b0812496b046b68e63c115aac5dd58f0d2a..8b53effe98f98214f04d696aacb05a65cbc819ac 100644 --- a/spec/models/lfs_download_object_spec.rb +++ b/spec/models/lfs_download_object_spec.rb @@ -50,7 +50,7 @@ describe LfsDownloadObject do before do allow(ApplicationSetting) .to receive(:current) - .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: setting)) + .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: setting)) end context 'are allowed' do diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 75d534c59bfd83cff87d11619eafbe7dc2bd5d0f..970e82e71072bb0b001e8dbf09381dfd2add9d28 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -17,7 +17,7 @@ describe Projects::LfsPointers::LfsDownloadService do before do ApplicationSetting.create_from_defaults - stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: local_request_setting) allow(project).to receive(:lfs_enabled?).and_return(true) end diff --git a/spec/services/self_monitoring/project/create_service_spec.rb b/spec/services/self_monitoring/project/create_service_spec.rb index a1e7aaf45f291e60f7b75be4e2c61337d5e7d8b3..87d7d776a69e19c80150bc1d51975f5a999daa97 100644 --- a/spec/services/self_monitoring/project/create_service_spec.rb +++ b/spec/services/self_monitoring/project/create_service_spec.rb @@ -37,7 +37,7 @@ describe SelfMonitoring::Project::CreateService do allow(ApplicationSetting) .to receive(:current) .and_return( - ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: true) + ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: true) ) end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 37bafc0c002ff1ff076756aa2ba5981fe1b8e19d..8353fade5ea729eb713a752ea4b8257df9b2a53d 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -19,15 +19,43 @@ describe WebHookService do let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } describe '#initialize' do - it 'allow_local_requests is true if hook is a SystemHook' do - instance = described_class.new(build(:system_hook), data, :system_hook) - expect(instance.request_options[:allow_local_requests]).to be_truthy - end + context 'when SystemHook' do + context 'when allow_local_requests_from_system_hooks application setting is true' do + it 'allows local requests' do + stub_application_setting(allow_local_requests_from_system_hooks: true) + instance = described_class.new(build(:system_hook), data, :system_hook) + + expect(instance.request_options[:allow_local_requests]).to be_truthy + end + end - it 'allow_local_requests is false if hook is not a SystemHook' do - %i(project_hook service_hook web_hook_log).each do |hook| - instance = described_class.new(build(hook), data, hook) - expect(instance.request_options[:allow_local_requests]).to be_falsey + context 'when allow_local_requests_from_system_hooks application setting is false' do + it 'denies local requests' do + stub_application_setting(allow_local_requests_from_system_hooks: false) + instance = described_class.new(build(:system_hook), data, :system_hook) + + expect(instance.request_options[:allow_local_requests]).to be_falsey + end + end + + context 'when ProjectHook' do + context 'when allow_local_requests_from_web_hooks_and_services application setting is true' do + it 'allows local requests' do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) + instance = described_class.new(build(:project_hook), data, :project_hook) + + expect(instance.request_options[:allow_local_requests]).to be_truthy + end + end + + context 'when allow_local_requests_from_system_hooks application setting is false' do + it 'denies local requests' do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) + instance = described_class.new(build(:project_hook), data, :project_hook) + + expect(instance.request_options[:allow_local_requests]).to be_falsey + end + end end end end