Merge branch '44496-outbound_requests_whitelist' into 'master'

Add Outbound requests whitelist for local networks

See merge request gitlab-org/gitlab-ce!30350

app/helpers/application_settings_helper.rb

@@ -177,6 +177,7 @@ module ApplicationSettingsHelper
       :domain_blacklist_enabled,
       :domain_blacklist_raw,
       :domain_whitelist_raw,
+      :outbound_local_requests_whitelist_raw,
       :dsa_key_restriction,
       :ecdsa_key_restriction,
       :ed25519_key_restriction,

app/models/application_setting.rb

@@ -41,6 +41,11 @@ class ApplicationSetting < ApplicationRecord
 
   validates :uuid, presence: true
 
+  validates :outbound_local_requests_whitelist,
+            length: { maximum: 1_000, message: N_('is too long (maximum is 1000 entries)') }
+
+  validates :outbound_local_requests_whitelist, qualified_domain_array: true, allow_blank: true
+
   validates :session_expire_delay,
             presence: true,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }

app/models/application_setting_implementation.rb

@@ -2,6 +2,7 @@
 
 module ApplicationSettingImplementation
   extend ActiveSupport::Concern
+  include Gitlab::Utils::StrongMemoize
 
   DOMAIN_LIST_SEPARATOR = %r{\s*[,;]\s*     # comma or semicolon, optionally surrounded by whitespace
                             |               # or
@@ -96,7 +97,8 @@ module ApplicationSettingImplementation
         diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
         commit_email_hostname: default_commit_email_hostname,
         protected_ci_variables: false,
-        local_markdown_version: 0
+        local_markdown_version: 0,
+        outbound_local_requests_whitelist: []
       }
     end
 
@@ -131,31 +133,52 @@ module ApplicationSettingImplementation
   end
 
   def domain_whitelist_raw
-    self.domain_whitelist&.join("\n")
+    array_to_string(self.domain_whitelist)
   end
 
   def domain_blacklist_raw
-    self.domain_blacklist&.join("\n")
+    array_to_string(self.domain_blacklist)
   end
 
   def domain_whitelist_raw=(values)
-    self.domain_whitelist = []
-    self.domain_whitelist = values.split(DOMAIN_LIST_SEPARATOR)
-    self.domain_whitelist.reject! { |d| d.empty? }
-    self.domain_whitelist
+    self.domain_whitelist = domain_strings_to_array(values)
   end
 
   def domain_blacklist_raw=(values)
-    self.domain_blacklist = []
-    self.domain_blacklist = values.split(DOMAIN_LIST_SEPARATOR)
-    self.domain_blacklist.reject! { |d| d.empty? }
-    self.domain_blacklist
+    self.domain_blacklist = domain_strings_to_array(values)
   end
 
   def domain_blacklist_file=(file)
     self.domain_blacklist_raw = file.read
   end
 
+  def outbound_local_requests_whitelist_raw
+    array_to_string(self.outbound_local_requests_whitelist)
+  end
+
+  def outbound_local_requests_whitelist_raw=(values)
+    self.outbound_local_requests_whitelist = domain_strings_to_array(values)
+  end
+
+  def outbound_local_requests_whitelist_arrays
+    strong_memoize(:outbound_local_requests_whitelist_arrays) do
+      ip_whitelist = []
+      domain_whitelist = []
+
+      self.outbound_local_requests_whitelist.each do |str|
+        ip_obj = Gitlab::Utils.string_to_ip_object(str)
+
+        if ip_obj
+          ip_whitelist << ip_obj
+        else
+          domain_whitelist << str
+        end
+      end
+
+      [ip_whitelist, domain_whitelist]
+    end
+  end
+
   def repository_storages
     Array(read_attribute(:repository_storages))
   end
@@ -255,6 +278,17 @@ module ApplicationSettingImplementation
 
   private
 
+  def array_to_string(arr)
+    arr&.join("\n")
+  end
+
+  def domain_strings_to_array(values)
+    values
+      .split(DOMAIN_LIST_SEPARATOR)
+      .reject(&:empty?)
+      .uniq
+  end
+
   def ensure_uuid!
     return if uuid?
 

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

@@ -8,6 +8,13 @@
         = f.label :allow_local_requests_from_hooks_and_services, class: 'form-check-label' do
           Allow requests to the local network from hooks and services
 
+    .form-group
+      = f.label :outbound_local_requests_whitelist_raw, class: 'label-bold' do
+        = _('Whitelist to allow requests to the local network from hooks and services')
+      = f.text_area :outbound_local_requests_whitelist_raw, placeholder: "example.com, 192.168.1.1", class: 'form-control', rows: 8
+      %span.form-text.text-muted
+        = _('Requests to these domain(s)/address(es) on the local network will be allowed when local requests from hooks and services are disabled. IP ranges such as 1:0:0:0:0:0:0:0/124 or 127.0.0.0/28 are supported. Domain wildcards are not supported currently. Use comma, semicolon, or newline to separate multiple entries. The whitelist can hold a maximum of 4000 entries. Domains should use IDNA encoding. Ex: domain.com, 192.168.1.1, 127.0.0.0/28.')
+
     .form-group
       .form-check
         = f.check_box :dns_rebinding_protection_enabled, class: 'form-check-input'

changelogs/unreleased/add-outbound-requests-whitelist-for-local-networks.yml

+---
+title: Add Outbound requests whitelist for local networks
+merge_request: 30350
+author: Istvan Szalai
+type: added

db/migrate/20180824202952_add_outbound_requests_whitelist_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddOutboundRequestsWhitelistToApplicationSettings < ActiveRecord::Migration[5.1]
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :outbound_local_requests_whitelist, :string, array: true, limit: 255
+  end
+end

db/schema.rb

@@ -228,6 +228,7 @@ ActiveRecord::Schema.define(version: 2019_07_15_114644) do
     t.boolean "lock_memberships_to_ldap", default: false, null: false
     t.boolean "time_tracking_limit_to_hours", default: false, null: false
     t.string "grafana_url", default: "/-/grafana", null: false
+    t.string "outbound_local_requests_whitelist", limit: 255, array: true
     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

@@ -39,6 +39,7 @@ Example response:
    "session_expire_delay" : 10080,
    "home_page_url" : null,
    "default_snippet_visibility" : "private",
+   "outbound_local_requests_whitelist": [],
    "domain_whitelist" : [],
    "domain_blacklist_enabled" : false,
    "domain_blacklist" : [],
@@ -113,6 +114,7 @@ Example response:
   "default_project_visibility": "internal",
   "default_snippet_visibility": "private",
   "default_group_visibility": "private",
+  "outbound_local_requests_whitelist": [],
   "domain_whitelist": [],
   "domain_blacklist_enabled" : false,
   "domain_blacklist" : [],
@@ -193,6 +195,7 @@ are listed in the descriptions of the relevant settings.
 | `domain_blacklist`                       | array of strings | required by: `domain_blacklist_enabled` | Users with e-mail addresses that match these domain(s) will NOT be able to sign-up. Wildcards allowed. Use separate lines for multiple entries. Ex: `domain.com`, `*.domain.com`. |
 | `domain_blacklist_enabled`               | boolean          | no                                   | (**If enabled, requires:** `domain_blacklist`) Allows blocking sign-ups from emails from specific domains. |
 | `domain_whitelist`                       | array of strings | no                                   | Force people to use only corporate emails for sign-up. Default is `null`, meaning there is no restriction. |
+| `outbound_local_requests_whitelist`      | array of strings | no                                   | Define a list of trusted domains or ip addresses to which local requests are allowed when local requests for hooks and services are disabled.
 | `dsa_key_restriction`                    | integer          | no                                   | The minimum allowed bit length of an uploaded DSA key. Default is `0` (no restriction). `-1` disables DSA keys. |
 | `ecdsa_key_restriction`                  | integer          | no                                   | The minimum allowed curve size (in bits) of an uploaded ECDSA key. Default is `0` (no restriction). `-1` disables ECDSA keys. |
 | `ed25519_key_restriction`                | integer          | no                                   | The minimum allowed curve size (in bits) of an uploaded ED25519 key. Default is `0` (no restriction). `-1` disables ED25519 keys. |

lib/gitlab/url_blocker.rb

@@ -45,18 +45,21 @@ module Gitlab
           ascii_only: ascii_only
         )
 
+        normalized_hostname = uri.normalized_host
         hostname = uri.hostname
         port = get_port(uri)
 
         address_info = get_address_info(hostname, port)
         return [uri, nil] unless address_info
 
-        protected_uri_with_hostname = enforce_uri_hostname(address_info, uri, hostname, dns_rebind_protection)
+        ip_address = ip_address(address_info)
+        protected_uri_with_hostname = enforce_uri_hostname(ip_address, uri, hostname, dns_rebind_protection)
 
         # Allow url from the GitLab instance itself but only for the configured hostname and ports
         return protected_uri_with_hostname if internal?(uri)
 
         validate_local_request(
+          normalized_hostname: normalized_hostname,
           address_info: address_info,
           allow_localhost: allow_localhost,
           allow_local_network: allow_local_network
@@ -83,10 +86,7 @@ module Gitlab
       #
       # The original hostname is used to validate the SSL, given in that scenario
       # we'll be making the request to the IP address, instead of using the hostname.
-      def enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection)
-        address = addrs_info.first
-        ip_address = address&.ip_address
-
+      def enforce_uri_hostname(ip_address, uri, hostname, dns_rebind_protection)
         return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != hostname
 
         uri = uri.dup
@@ -94,6 +94,10 @@ module Gitlab
         [uri, hostname]
       end
 
+      def ip_address(address_info)
+        address_info.first&.ip_address
+      end
+
       def validate_uri(uri:, schemes:, ports:, enforce_sanitization:, enforce_user:, ascii_only:)
         validate_html_tags(uri) if enforce_sanitization
 
@@ -113,9 +117,19 @@ module Gitlab
       rescue SocketError
       end
 
-      def validate_local_request(address_info:, allow_localhost:, allow_local_network:)
+      def validate_local_request(
+        normalized_hostname:,
+        address_info:,
+        allow_localhost:,
+        allow_local_network:)
         return if allow_local_network && allow_localhost
 
+        ip_whitelist, domain_whitelist =
+          Gitlab::CurrentSettings.outbound_local_requests_whitelist_arrays
+
+        return if local_domain_whitelisted?(domain_whitelist, normalized_hostname) ||
+          local_ip_whitelisted?(ip_whitelist, ip_address(address_info))
+
         unless allow_localhost
           validate_localhost(address_info)
           validate_loopback(address_info)
@@ -231,6 +245,16 @@ module Gitlab
           (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
       end
 
+      def local_ip_whitelisted?(ip_whitelist, ip_string)
+        ip_obj = Gitlab::Utils.string_to_ip_object(ip_string)
+
+        ip_whitelist.any? { |ip| ip.include?(ip_obj) }
+      end
+
+      def local_domain_whitelisted?(domain_whitelist, domain_string)
+        domain_whitelist.include?(domain_string)
+      end
+
       def config
         Gitlab.config
       end

lib/gitlab/utils.rb

@@ -131,5 +131,12 @@ module Gitlab
         data
       end
     end
+
+    def string_to_ip_object(str)
+      return unless str
+
+      IPAddr.new(str)
+    rescue IPAddr::InvalidAddressError
+    end
   end
 end

locale/gitlab.pot

@@ -8998,6 +8998,9 @@ msgstr ""
 msgid "Requests Profiles"
 msgstr ""
 
+msgid "Requests to these domain(s)/address(es) on the local network will be allowed when local requests from hooks and services are disabled. IP ranges such as 1:0:0:0:0:0:0:0/124 or 127.0.0.0/28 are supported. Domain wildcards are not supported currently. Use comma, semicolon, or newline to separate multiple entries. The whitelist can hold a maximum of 4000 entries. Domains should use IDNA encoding. Ex: domain.com, 192.168.1.1, 127.0.0.0/28."
+msgstr ""
+
 msgid "Require all users in this group to setup Two-factor authentication"
 msgstr ""
 
@@ -12133,6 +12136,9 @@ msgstr[1] ""
 msgid "When:"
 msgstr ""
 
+msgid "Whitelist to allow requests to the local network from hooks and services"
+msgstr ""
+
 msgid "Who can see this group?"
 msgstr ""
 
@@ -12912,6 +12918,9 @@ msgstr ""
 msgid "is not an email you own"
 msgstr ""
 
+msgid "is too long (maximum is 1000 entries)"
+msgstr ""
+
 msgid "issue"
 msgstr ""
 

spec/lib/gitlab/utils_spec.rb

@@ -231,4 +231,23 @@ describe Gitlab::Utils do
       end
     end
   end
+
+  describe '.string_to_ip_object' do
+    it 'returns nil when string is nil' do
+      expect(described_class.string_to_ip_object(nil)).to eq(nil)
+    end
+
+    it 'returns nil when string is invalid IP' do
+      expect(described_class.string_to_ip_object('invalid ip')).to eq(nil)
+      expect(described_class.string_to_ip_object('')).to eq(nil)
+    end
+
+    it 'returns IP object when string is valid IP' do
+      expect(described_class.string_to_ip_object('192.168.1.1')).to eq(IPAddr.new('192.168.1.1'))
+      expect(described_class.string_to_ip_object('::ffff:a9fe:a864')).to eq(IPAddr.new('::ffff:a9fe:a864'))
+      expect(described_class.string_to_ip_object('[::ffff:a9fe:a864]')).to eq(IPAddr.new('::ffff:a9fe:a864'))
+      expect(described_class.string_to_ip_object('127.0.0.0/28')).to eq(IPAddr.new('127.0.0.0/28'))
+      expect(described_class.string_to_ip_object('1:0:0:0:0:0:0:0/124')).to eq(IPAddr.new('1:0:0:0:0:0:0:0/124'))
+    end
+  end
 end

spec/models/application_setting_spec.rb

@@ -37,6 +37,17 @@ describe ApplicationSetting do
     it { is_expected.not_to allow_value("myemail@example.com").for(:lets_encrypt_notification_email) }
     it { is_expected.to allow_value("myemail@test.example.com").for(:lets_encrypt_notification_email) }
 
+    it { is_expected.to allow_value(['192.168.1.1'] * 1_000).for(:outbound_local_requests_whitelist) }
+    it { is_expected.not_to allow_value(['192.168.1.1'] * 1_001).for(:outbound_local_requests_whitelist) }
+    it { is_expected.to allow_value(['1' * 255]).for(:outbound_local_requests_whitelist) }
+    it { is_expected.not_to allow_value(['1' * 256]).for(:outbound_local_requests_whitelist) }
+    it { is_expected.not_to allow_value(['ğitlab.com']).for(:outbound_local_requests_whitelist) }
+    it { is_expected.to allow_value(['xn--itlab-j1a.com']).for(:outbound_local_requests_whitelist) }
+    it { is_expected.not_to allow_value(['<h1></h1>']).for(:outbound_local_requests_whitelist) }
+    it { is_expected.to allow_value(['gitlab.com']).for(:outbound_local_requests_whitelist) }
+    it { is_expected.to allow_value(nil).for(:outbound_local_requests_whitelist) }
+    it { is_expected.to allow_value([]).for(:outbound_local_requests_whitelist) }
+
     context "when user accepted let's encrypt terms of service" do
       before do
         setting.update(lets_encrypt_terms_of_service_accepted: true)

spec/support/shared_examples/application_setting_examples.rb

 # frozen_string_literal: true
 
-RSpec.shared_examples 'application settings examples' do
-  context 'restricted signup domains' do
+RSpec.shared_examples 'string of domains' do |attribute|
   it 'sets single domain' do
-      setting.domain_whitelist_raw = 'example.com'
-      expect(setting.domain_whitelist).to eq(['example.com'])
+    setting.method("#{attribute}_raw=").call('example.com')
+    expect(setting.method(attribute).call).to eq(['example.com'])
   end
 
   it 'sets multiple domains with spaces' do
-      setting.domain_whitelist_raw = 'example.com *.example.com'
-      expect(setting.domain_whitelist).to eq(['example.com', '*.example.com'])
+    setting.method("#{attribute}_raw=").call('example.com *.example.com')
+    expect(setting.method(attribute).call).to eq(['example.com', '*.example.com'])
   end
 
   it 'sets multiple domains with newlines and a space' do
-      setting.domain_whitelist_raw = "example.com\n *.example.com"
-      expect(setting.domain_whitelist).to eq(['example.com', '*.example.com'])
+    setting.method("#{attribute}_raw=").call("example.com\n *.example.com")
+    expect(setting.method(attribute).call).to eq(['example.com', '*.example.com'])
   end
 
   it 'sets multiple domains with commas' do
-      setting.domain_whitelist_raw = "example.com, *.example.com"
-      expect(setting.domain_whitelist).to eq(['example.com', '*.example.com'])
-    end
+    setting.method("#{attribute}_raw=").call("example.com, *.example.com")
+    expect(setting.method(attribute).call).to eq(['example.com', '*.example.com'])
   end
 
-  context 'blacklisted signup domains' do
-    it 'sets single domain' do
-      setting.domain_blacklist_raw = 'example.com'
-      expect(setting.domain_blacklist).to contain_exactly('example.com')
+  it 'sets multiple domains with semicolon' do
+    setting.method("#{attribute}_raw=").call("example.com; *.example.com")
+    expect(setting.method(attribute).call).to contain_exactly('example.com', '*.example.com')
   end
 
-    it 'sets multiple domains with spaces' do
-      setting.domain_blacklist_raw = 'example.com *.example.com'
-      expect(setting.domain_blacklist).to contain_exactly('example.com', '*.example.com')
+  it 'sets multiple domains with mixture of everything' do
+    setting.method("#{attribute}_raw=").call("example.com; *.example.com\n test.com\sblock.com   yes.com")
+    expect(setting.method(attribute).call).to contain_exactly('example.com', '*.example.com', 'test.com', 'block.com', 'yes.com')
   end
 
-    it 'sets multiple domains with newlines and a space' do
-      setting.domain_blacklist_raw = "example.com\n *.example.com"
-      expect(setting.domain_blacklist).to contain_exactly('example.com', '*.example.com')
+  it 'removes duplicates' do
+    setting.method("#{attribute}_raw=").call("example.com; example.com; 127.0.0.1; 127.0.0.1")
+    expect(setting.method(attribute).call).to contain_exactly('example.com', '127.0.0.1')
   end
 
-    it 'sets multiple domains with commas' do
-      setting.domain_blacklist_raw = "example.com, *.example.com"
-      expect(setting.domain_blacklist).to contain_exactly('example.com', '*.example.com')
+  it 'does not fail with garbage values' do
+    setting.method("#{attribute}_raw=").call("example;34543:garbage:fdh5654;")
+    expect(setting.method(attribute).call).to contain_exactly('example', '34543:garbage:fdh5654')
   end
+end
 
-    it 'sets multiple domains with semicolon' do
-      setting.domain_blacklist_raw = "example.com; *.example.com"
-      expect(setting.domain_blacklist).to contain_exactly('example.com', '*.example.com')
+RSpec.shared_examples 'application settings examples' do
+  context 'restricted signup domains' do
+    it_behaves_like 'string of domains', :domain_whitelist
   end
 
-    it 'sets multiple domains with mixture of everything' do
-      setting.domain_blacklist_raw = "example.com; *.example.com\n test.com\sblock.com   yes.com"
-      expect(setting.domain_blacklist).to contain_exactly('example.com', '*.example.com', 'test.com', 'block.com', 'yes.com')
-    end
+  context 'blacklisted signup domains' do
+    it_behaves_like 'string of domains', :domain_blacklist
 
     it 'sets multiple domain with file' do
       setting.domain_blacklist_file = File.open(Rails.root.join('spec/fixtures/', 'domain_blacklist.txt'))
@@ -60,6 +56,27 @@ RSpec.shared_examples 'application settings examples' do
     end
   end
 
+  context 'outbound_local_requests_whitelist' do
+    it_behaves_like 'string of domains', :outbound_local_requests_whitelist
+  end
+
+  context 'outbound_local_requests_whitelist_arrays' do
+    it 'separates the IPs and domains' do
+      setting.outbound_local_requests_whitelist = [
+        '192.168.1.1', '127.0.0.0/28', 'www.example.com', 'example.com',
+        '::ffff:a00:2', '1:0:0:0:0:0:0:0/124', 'subdomain.example.com'
+      ]
+
+      ip_whitelist = [
+        IPAddr.new('192.168.1.1'), IPAddr.new('127.0.0.0/8'),
+        IPAddr.new('::ffff:a00:2'), IPAddr.new('1:0:0:0:0:0:0:0/124')
+      ]
+      domain_whitelist = ['www.example.com', 'example.com', 'subdomain.example.com']
+
+      expect(setting.outbound_local_requests_whitelist_arrays).to contain_exactly(ip_whitelist, domain_whitelist)
+    end
+  end
+
   describe 'usage ping settings' do
     context 'when the usage ping is disabled in gitlab.yml' do
       before do