Add outbound requests setting for system hooks

This MR adds new application setting to network section
`allow_local_requests_from_system_hooks`. Prior to this change
system hooks were allowed to do local network requests by default
and we are adding an ability for admins to control it.

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,

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,

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

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

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

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

app/validators/system_hook_url_validator.rb

+# 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

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

db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb

+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

db/migrate/20190726101133_add_allow_local_requests_from_system_hooks_to_application_settings.rb

+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

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"

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. |

lib/gitlab/http_connection_adapter.rb

 # 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

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

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.)"

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

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

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

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 }

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

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
 

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
 

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