Merge branch 'sh-fix-no-proxy-webhooks' into 'master'

Fix no_proxy not working when DNS rebinding protection enabled

See merge request !120412



Merged-by: Dylan Griffith <dyl.griffith@gmail.com>
Approved-by: Nick Malcolm <nmalcolm@gitlab.com>
Approved-by: Luke Duncalfe <lduncalfe@gitlab.com>
Approved-by: Dylan Griffith <dyl.griffith@gmail.com>
Reviewed-by: Luke Duncalfe <lduncalfe@gitlab.com>
Reviewed-by: Nick Malcolm <nmalcolm@gitlab.com>
Co-authored-by: Stan Hu <stanhu@gmail.com>

lib/gitlab/http_connection_adapter.rb

@@ -24,11 +24,18 @@ class HTTPConnectionAdapter < HTTParty::ConnectionAdapter
 
     override :connection
     def connection
-      @uri, hostname = validate_url!(uri)
+      result = validate_url_with_proxy!(uri)
+      @uri = result.uri
+      hostname = result.hostname
 
       http = super
       http.hostname_override = hostname if hostname
 
+      unless result.use_proxy
+        http.proxy_from_env = false
+        http.proxy_address = nil
+      end
+
       gitlab_http = Gitlab::NetHttpAdapter.new(http.address, http.port)
 
       http.instance_variables.each do |variable|
@@ -40,12 +47,13 @@ def connection
 
     private
 
-    def validate_url!(url)
-      Gitlab::UrlBlocker.validate!(url, allow_local_network: allow_local_requests?,
-                                        allow_localhost: allow_local_requests?,
-                                        allow_object_storage: allow_object_storage?,
-                                        dns_rebind_protection: dns_rebind_protection?,
-                                        schemes: %w[http https])
+    def validate_url_with_proxy!(url)
+      Gitlab::UrlBlocker.validate_url_with_proxy!(
+        url, allow_local_network: allow_local_requests?,
+        allow_localhost: allow_local_requests?,
+        allow_object_storage: allow_object_storage?,
+        dns_rebind_protection: dns_rebind_protection?,
+        schemes: %w[http https])
     rescue Gitlab::UrlBlocker::BlockedUrlError => e
       raise Gitlab::HTTP::BlockedUrlError, "URL is blocked: #{e.message}"
     end

lib/gitlab/url_blocker.rb

@@ -9,6 +9,23 @@ class UrlBlocker
 
     DENY_ALL_REQUESTS_EXCEPT_ALLOWED_DEFAULT = proc { deny_all_requests_except_allowed_app_setting }.freeze
 
+    # Result stores the validation result:
+    # uri - The original URI requested
+    # hostname - The hostname that should be used to connect. For DNS
+    #   rebinding protection, this will be the resolved IP address of
+    #   the hostname.
+    # use_proxy -
+    #   If true, this means that the proxy server specified in the
+    #   http_proxy/https_proxy environment variables should be used.
+    #
+    #   If false, this either means that no proxy server was specified
+    #   or that the hostname in the URL is exempt via the no_proxy
+    #   environment variable. This allows the caller to disable usage
+    #   of a proxy since the IP address may be used to
+    #   connect. Otherwise, Net::HTTP may erroneously compare the IP
+    #   address against the no_proxy list.
+    Result = Struct.new(:uri, :hostname, :use_proxy)
+
     class << self
       # Validates the given url according to the constraints specified by arguments.
       #
@@ -21,9 +38,9 @@ class << self
       # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true.
       # deny_all_requests_except_allowed - Raises error if URL is not in the allow list and argument is true. Can be Boolean or Proc. Defaults to instance app setting.
       #
-      # Returns an array with [<uri>, <original-hostname>].
+      # Returns a Result object.
       # rubocop:disable Metrics/ParameterLists
-      def validate!(
+      def validate_url_with_proxy!(
         url,
         schemes:,
         ports: [],
@@ -37,7 +54,7 @@ def validate!(
         dns_rebind_protection: true)
         # rubocop:enable Metrics/ParameterLists
 
-        return [nil, nil] if url.nil?
+        return Result.new(nil, nil, true) if url.nil?
 
         raise ArgumentError, 'The schemes is a required argument' if schemes.blank?
 
@@ -56,19 +73,22 @@ def validate!(
         begin
           address_info = get_address_info(uri)
         rescue SocketError
-          return [uri, nil] unless enforce_address_info_retrievable?(uri, dns_rebind_protection, deny_all_requests_except_allowed)
+          proxy_in_use = uri_under_proxy_setting?(uri, nil)
+
+          return Result.new(uri, nil, proxy_in_use) unless enforce_address_info_retrievable?(uri, dns_rebind_protection, deny_all_requests_except_allowed)
 
           raise BlockedUrlError, 'Host cannot be resolved or invalid'
         end
 
         ip_address = ip_address(address_info)
+        proxy_in_use = uri_under_proxy_setting?(uri, ip_address)
 
         # Ignore DNS rebind protection when a proxy is being used, as DNS
         # rebinding is expected behavior.
-        dns_rebind_protection &= !uri_under_proxy_setting?(uri, ip_address)
-        return [uri, nil] if domain_in_allow_list?(uri)
+        dns_rebind_protection &&= !proxy_in_use
+        return Result.new(uri, nil, proxy_in_use) if domain_in_allow_list?(uri)
 
-        protected_uri_with_hostname = enforce_uri_hostname(ip_address, uri, dns_rebind_protection)
+        protected_uri_with_hostname = enforce_uri_hostname(ip_address, uri, dns_rebind_protection, proxy_in_use)
 
         return protected_uri_with_hostname if ip_in_allow_list?(ip_address, port: get_port(uri))
 
@@ -96,6 +116,13 @@ def blocked_url?(url, **kwargs)
         true
       end
 
+      # For backwards compatibility, Returns an array with [<uri>, <original-hostname>].
+      # Issue for refactoring: https://gitlab.com/gitlab-org/gitlab/-/issues/410890
+      def validate!(...)
+        result = validate_url_with_proxy!(...)
+        [result.uri, result.hostname]
+      end
+
       private
 
       # Returns the given URI with IP address as hostname and the original hostname respectively
@@ -106,12 +133,12 @@ def blocked_url?(url, **kwargs)
       #
       # 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(ip_address, uri, dns_rebind_protection)
-        return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != uri.hostname
+      def enforce_uri_hostname(ip_address, uri, dns_rebind_protection, proxy_in_use)
+        return Result.new(uri, nil, proxy_in_use) unless dns_rebind_protection && ip_address && ip_address != uri.hostname
 
         new_uri = uri.dup
         new_uri.hostname = ip_address
-        [new_uri, uri.hostname]
+        Result.new(new_uri, uri.hostname, proxy_in_use)
       end
 
       def ip_address(address_info)

spec/lib/gitlab/http_connection_adapter_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::HTTPConnectionAdapter do
+RSpec.describe Gitlab::HTTPConnectionAdapter, feature_category: :shared do
   include StubRequests
 
   let(:uri) { URI('https://example.org') }
@@ -111,6 +111,42 @@
       end
     end
 
+    context 'when proxy is enabled' do
+      before do
+        stub_env('http_proxy', 'http://proxy.example.com')
+      end
+
+      it 'proxy stays configured' do
+        expect(connection.proxy?).to be true
+        expect(connection.proxy_from_env?).to be true
+        expect(connection.proxy_address).to eq('proxy.example.com')
+      end
+
+      context 'when no_proxy matches the request' do
+        before do
+          stub_env('no_proxy', 'example.org')
+        end
+
+        it 'proxy is disabled' do
+          expect(connection.proxy?).to be false
+          expect(connection.proxy_from_env?).to be false
+          expect(connection.proxy_address).to be nil
+        end
+      end
+
+      context 'when no_proxy does not match the request' do
+        before do
+          stub_env('no_proxy', 'example.com')
+        end
+
+        it 'proxy stays configured' do
+          expect(connection.proxy?).to be true
+          expect(connection.proxy_from_env?).to be true
+          expect(connection.proxy_address).to eq('proxy.example.com')
+        end
+      end
+    end
+
     context 'when URL scheme is not HTTP/HTTPS' do
       let(:uri) { URI('ssh://example.org') }
 

spec/lib/gitlab/url_blocker_spec.rb

@@ -7,6 +7,9 @@
 
   let(:schemes) { %w[http https] }
 
+  # This test ensures backward compatibliity for the validate! method.
+  # We shoud refactor all callers of validate! to handle a Result object:
+  # https://gitlab.com/gitlab-org/gitlab/-/issues/410890
   describe '#validate!' do
     let(:options) { { schemes: schemes } }
 
@@ -21,6 +24,36 @@
       end
     end
 
+    context 'when the URL hostname is a domain' do
+      context 'when domain can be resolved' do
+        let(:import_url) { 'https://example.org' }
+
+        before do
+          stub_dns(import_url, ip_address: '93.184.216.34')
+        end
+
+        it_behaves_like 'validates URI and hostname' do
+          let(:expected_uri) { 'https://93.184.216.34' }
+          let(:expected_hostname) { 'example.org' }
+          let(:expected_use_proxy) { false }
+        end
+      end
+    end
+  end
+
+  describe '#validate_url_with_proxy!' do
+    let(:options) { { schemes: schemes } }
+
+    subject { described_class.validate_url_with_proxy!(import_url, **options) }
+
+    shared_examples 'validates URI and hostname' do
+      it 'runs the url validations' do
+        expect(subject.uri).to eq(Addressable::URI.parse(expected_uri))
+        expect(subject.hostname).to eq(expected_hostname)
+        expect(subject.use_proxy).to eq(expected_use_proxy)
+      end
+    end
+
     shared_context 'when instance configured to deny all requests' do
       before do
         allow(Gitlab::CurrentSettings).to receive(:current_application_settings?).and_return(true)
@@ -94,6 +127,7 @@
       it_behaves_like 'validates URI and hostname' do
         let(:expected_uri) { nil }
         let(:expected_hostname) { nil }
+        let(:expected_use_proxy) { true }
       end
 
       it_behaves_like 'a URI exempt from `deny_all_requests_except_allowed`'
@@ -109,6 +143,7 @@
       it_behaves_like 'validates URI and hostname' do
         let(:expected_uri) { 'http://127.0.0.1' }
         let(:expected_hostname) { 'localhost' }
+        let(:expected_use_proxy) { false }
       end
 
       it_behaves_like 'a URI exempt from `deny_all_requests_except_allowed`'
@@ -146,6 +181,7 @@
           it_behaves_like 'validates URI and hostname' do
             let(:expected_uri) { 'http://127.0.0.1:9000/external-diffs/merge_request_diffs/mr-1/diff-1' }
             let(:expected_hostname) { 'review-minio-svc.svc' }
+            let(:expected_use_proxy) { false }
           end
 
           it_behaves_like 'a URI exempt from `deny_all_requests_except_allowed`'
@@ -157,6 +193,7 @@
           it_behaves_like 'validates URI and hostname' do
             let(:expected_uri) { 'http://127.0.0.1:9000/external-diffs/merge_request_diffs/mr-1/diff-1' }
             let(:expected_hostname) { nil }
+            let(:expected_use_proxy) { false }
           end
 
           it_behaves_like 'a URI exempt from `deny_all_requests_except_allowed`'
@@ -239,6 +276,7 @@
         it_behaves_like 'validates URI and hostname' do
           let(:expected_uri) { 'https://93.184.216.34' }
           let(:expected_hostname) { 'example.org' }
+          let(:expected_use_proxy) { false }
         end
 
         it_behaves_like 'a URI denied by `deny_all_requests_except_allowed`'
@@ -259,12 +297,25 @@
           let(:import_url) { 'http://foobar.x' }
 
           before do
-            allow(Gitlab).to receive(:http_proxy_env?).and_return(true)
+            stub_env('http_proxy', 'http://proxy.example.com')
           end
 
           it_behaves_like 'validates URI and hostname' do
             let(:expected_uri) { import_url }
             let(:expected_hostname) { nil }
+            let(:expected_use_proxy) { true }
+          end
+
+          context 'with no_proxy' do
+            before do
+              stub_env('no_proxy', 'foobar.x')
+            end
+
+            it_behaves_like 'validates URI and hostname' do
+              let(:expected_uri) { import_url }
+              let(:expected_hostname) { nil }
+              let(:expected_use_proxy) { false }
+            end
           end
         end
       end
@@ -284,6 +335,7 @@
       it_behaves_like 'validates URI and hostname' do
         let(:expected_uri) { import_url }
         let(:expected_hostname) { nil }
+        let(:expected_use_proxy) { false }
       end
 
       it_behaves_like 'a URI denied by `deny_all_requests_except_allowed`'
@@ -311,18 +363,20 @@
       it_behaves_like 'validates URI and hostname' do
         let(:expected_uri) { 'http://192.168.0.120:9121/scrape?target=unix:///var/opt/gitlab/redis/redis.socket&check-keys=*' }
         let(:expected_hostname) { 'a.192.168.0.120.3times.127.0.0.1.1time.repeat.rebind.network' }
+        let(:expected_use_proxy) { false }
       end
 
       it_behaves_like 'a URI exempt from `deny_all_requests_except_allowed`'
 
       context 'with HTTP_PROXY' do
         before do
-          allow(Gitlab).to receive(:http_proxy_env?).and_return(true)
+          stub_env('http_proxy', 'http://proxy.example.com')
         end
 
         it_behaves_like 'validates URI and hostname' do
           let(:expected_uri) { import_url }
           let(:expected_hostname) { nil }
+          let(:expected_use_proxy) { true }
         end
 
         context 'when domain is in no_proxy env' do
@@ -333,6 +387,7 @@
           it_behaves_like 'validates URI and hostname' do
             let(:expected_uri) { 'http://192.168.0.120:9121/scrape?target=unix:///var/opt/gitlab/redis/redis.socket&check-keys=*' }
             let(:expected_hostname) { 'a.192.168.0.120.3times.127.0.0.1.1time.repeat.rebind.network' }
+            let(:expected_use_proxy) { false }
           end
         end
       end
@@ -347,6 +402,7 @@
         it_behaves_like 'validates URI and hostname' do
           let(:expected_uri) { import_url }
           let(:expected_hostname) { nil }
+          let(:expected_use_proxy) { false }
         end
 
         it_behaves_like 'a URI exempt from `deny_all_requests_except_allowed`'
@@ -363,6 +419,7 @@
           it_behaves_like 'validates URI and hostname' do
             let(:expected_uri) { import_url }
             let(:expected_hostname) { nil }
+            let(:expected_use_proxy) { false }
           end
 
           it_behaves_like 'a URI denied by `deny_all_requests_except_allowed`'
@@ -374,6 +431,7 @@
           it_behaves_like 'validates URI and hostname' do
             let(:expected_uri) { import_url }
             let(:expected_hostname) { nil }
+            let(:expected_use_proxy) { false }
           end
 
           it_behaves_like 'a URI denied by `deny_all_requests_except_allowed`'
@@ -386,6 +444,7 @@
         it_behaves_like 'validates URI and hostname' do
           let(:expected_uri) { import_url }
           let(:expected_hostname) { nil }
+          let(:expected_use_proxy) { false }
         end
 
         it_behaves_like 'a URI denied by `deny_all_requests_except_allowed`'
@@ -396,6 +455,7 @@
           it_behaves_like 'validates URI and hostname' do
             let(:expected_uri) { import_url }
             let(:expected_hostname) { nil }
+            let(:expected_use_proxy) { false }
           end
 
           it_behaves_like 'a URI denied by `deny_all_requests_except_allowed`'