From 17cfff2c246b479e4ec51c11cad4628049a5a9b3 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 17 May 2023 04:35:54 +0000 Subject: Add latest changes from gitlab-org/gitlab@15-11-stable-ee --- lib/gitlab/http_connection_adapter.rb | 22 ++++++--- lib/gitlab/url_blocker.rb | 47 ++++++++++++++---- spec/lib/gitlab/http_connection_adapter_spec.rb | 38 ++++++++++++++- spec/lib/gitlab/url_blocker_spec.rb | 64 ++++++++++++++++++++++++- 4 files changed, 151 insertions(+), 20 deletions(-) diff --git a/lib/gitlab/http_connection_adapter.rb b/lib/gitlab/http_connection_adapter.rb index 2152f619228..afb740a902b 100644 --- a/lib/gitlab/http_connection_adapter.rb +++ b/lib/gitlab/http_connection_adapter.rb @@ -24,11 +24,18 @@ module Gitlab 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 @@ module Gitlab 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 diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 2c02874876a..67178af08c4 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -9,6 +9,23 @@ module Gitlab 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 @@ module Gitlab # 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 [, ]. + # Returns a Result object. # rubocop:disable Metrics/ParameterLists - def validate!( + def validate_url_with_proxy!( url, schemes:, ports: [], @@ -37,7 +54,7 @@ module Gitlab 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 @@ module Gitlab 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 @@ module Gitlab true end + # For backwards compatibility, Returns an array with [, ]. + # 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 @@ 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(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) diff --git a/spec/lib/gitlab/http_connection_adapter_spec.rb b/spec/lib/gitlab/http_connection_adapter_spec.rb index 8b8097f4885..fac0c1a2a9f 100644 --- a/spec/lib/gitlab/http_connection_adapter_spec.rb +++ b/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 @@ RSpec.describe Gitlab::HTTPConnectionAdapter do 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') } diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 7b6c89b5dd3..7d3060b005e 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -7,6 +7,9 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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`' @@ -240,6 +277,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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`' @@ -260,12 +298,25 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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 @@ -285,6 +336,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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`' @@ -312,18 +364,20 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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 @@ -334,6 +388,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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 @@ -348,6 +403,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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`' @@ -364,6 +420,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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`' @@ -375,6 +432,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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`' @@ -387,6 +445,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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`' @@ -397,6 +456,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only, feature_category: :sh 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`' -- cgit v1.2.1