Merge branch 'jm-fix-special-character-passwords' into 'master'

Pass hash instead of URI to Elasticsearch client

See merge request gitlab-org/gitlab!85122

ee/app/models/ee/application_setting.rb

@@ -317,12 +317,8 @@ module EE
     end
 
     def elasticsearch_url_with_credentials
-      return elasticsearch_url if elasticsearch_username.blank?
-
       elasticsearch_url.map do |uri|
-        uri.user = URI.encode_www_form_component(elasticsearch_username)
-        uri.password = URI.encode_www_form_component(elasticsearch_password)
-        uri
+        ::Gitlab::Elastic::Helper.connection_settings(uri: uri, user: elasticsearch_username, password: elasticsearch_password)
       end
     end
 

ee/lib/gem_extensions/elasticsearch/model/client.rb

@@ -25,7 +25,7 @@ module GemExtensions
             adapter = ::Gitlab::Elastic::Client.adapter
 
             if store.cached_client.nil? || config != store.cached_config || adapter != store.cached_adapter
-              store.cached_client = ::Gitlab::Elastic::Client.build(config)
+              store.cached_client = ::Gitlab::Elastic::Client.build(config.deep_dup)
               store.cached_config = config
               store.cached_adapter = adapter
             end

ee/lib/gitlab/elastic/helper.rb

@@ -42,6 +42,28 @@ module Gitlab
         def default
           self.new
         end
+
+        def connection_settings(uri:, user: nil, password: nil)
+          # Returns a hash that is compatible with elasticsearch-transport client settings.
+          #
+          # See:
+          # https://github.com/elastic/elasticsearch-ruby/blob/v7.3.0/elasticsearch-transport/lib/elasticsearch/transport/client.rb#L196
+          uri = Addressable::URI.parse(uri) if uri.is_a? String
+
+          {
+            scheme:   uri.scheme,
+            user:     user.presence || Addressable::URI.unencode(uri.user),
+            password: password.presence || Addressable::URI.unencode(uri.password),
+            host:     uri.host,
+            path:     uri.path,
+            port:     uri.port
+          }.compact
+        end
+
+        def url_string(url_settings)
+          # Converts the hash from connection_settings into a percent encoded URL string.
+          Addressable::URI.new(url_settings).normalize.to_s
+        end
       end
 
       def default_settings

ee/lib/gitlab/elastic/indexer.rb

@@ -183,9 +183,18 @@ module Gitlab
       end
 
       def elasticsearch_config(target)
-        Gitlab::CurrentSettings.elasticsearch_config.merge(
+        config = Gitlab::CurrentSettings.elasticsearch_config.merge(
           index_name: target.index_name
-        ).to_json
+        )
+
+        # We need to pass a percent encoded URL string instead of a hash
+        # to the go indexer because it passes authentication credentials
+        # embedded in the url.
+        #
+        # See:
+        # https://gitlab.com/gitlab-org/gitlab-elasticsearch-indexer/blob/main/elastic/elastic.go#L22
+        config[:url] = config[:url].map { |u| ::Gitlab::Elastic::Helper.url_string(u) }
+        config.to_json
       end
 
       def gitaly_config

ee/spec/lib/ee/gitlab/elastic/helper_spec.rb

@@ -45,6 +45,71 @@ RSpec.describe Gitlab::Elastic::Helper, :request_store do
     end
   end
 
+  describe '.connection_settings' do
+    it 'returns a hash compatible with elasticsearcht-transport client settings' do
+      settings = described_class.connection_settings(uri: "http://localhost:9200")
+
+      expect(settings).to eq({
+        scheme:   "http",
+        host:     "localhost",
+        path:     "",
+        port:     9200
+      })
+    end
+
+    it 'works when given a URI' do
+      settings = described_class.connection_settings(uri: Addressable::URI.parse("http://localhost:9200"))
+
+      expect(settings).to eq({
+        scheme:   "http",
+        host:     "localhost",
+        path:     "",
+        port:     9200
+      })
+    end
+
+    it 'parses credentials out of the uri' do
+      settings = described_class.connection_settings(uri: "http://elastic:myp%40ssword@localhost:9200")
+
+      expect(settings).to eq({
+        scheme:   "http",
+        host:     "localhost",
+        user:     "elastic",
+        password: "myp@ssword",
+        path:     "",
+        port:     9200
+      })
+    end
+
+    it 'prioritizes creds in arguments over those in url' do
+      settings = described_class.connection_settings(uri: "http://elastic:password@localhost:9200", user: "myuser", password: "p@ssword")
+
+      expect(settings).to eq({
+        scheme:   "http",
+        host:     "localhost",
+        user:     "myuser",
+        password: "p@ssword",
+        path:     "",
+        port:     9200
+      })
+    end
+  end
+
+  describe '.`url_string`' do
+    it 'returns a percent encoded url string' do
+      settings = {
+        scheme:   "http",
+        host:     "localhost",
+        user:     "myuser",
+        password: "p@ssword",
+        path:     "/foo",
+        port:     9200
+      }
+
+      expect(described_class.url_string(settings)).to eq("http://myuser:p%40ssword@localhost:9200/foo")
+    end
+  end
+
   describe '#default_mappings' do
     it 'has only one type' do
       expect(helper.default_mappings.keys).to match_array %i(doc)

ee/spec/lib/gitlab/elastic/indexer_spec.rb

@@ -456,7 +456,9 @@ RSpec.describe Gitlab::Elastic::Indexer do
   def elasticsearch_config
     Gitlab::CurrentSettings.elasticsearch_config.merge(
       index_name: 'gitlab-test'
-    )
+    ).tap do |config|
+      config[:url] = config[:url].map { |u| ::Gitlab::Elastic::Helper.url_string(u) }
+    end
   end
 
   def envvars

ee/spec/models/application_setting_spec.rb

@@ -334,54 +334,78 @@ RSpec.describe ApplicationSetting do
   end
 
   describe '#elasticsearch_url_with_credentials' do
-    it 'embeds credentials in the result' do
-      setting.elasticsearch_url = 'http://example.com,https://example.org:9200'
-      setting.elasticsearch_username = 'foo'
-      setting.elasticsearch_password = 'bar'
+    let(:elasticsearch_url) { "#{host1},#{host2}" }
+    let(:host1) { 'http://example.com' }
+    let(:host2) { 'https://example.org:9200' }
+    let(:elasticsearch_username) { 'elastic' }
+    let(:elasticsearch_password) { 'password' }
 
-      expect(setting.elasticsearch_url_with_credentials).to match_array([URI.parse('http://foo:bar@example.com'), URI.parse('https://foo:bar@example.org:9200')])
+    before do
+      setting.elasticsearch_url = elasticsearch_url
+      setting.elasticsearch_username = elasticsearch_username
+      setting.elasticsearch_password = elasticsearch_password
     end
 
-    it 'embeds username only' do
-      setting.elasticsearch_url = 'http://example.com,https://example.org:9200'
-      setting.elasticsearch_username = 'foo'
-      setting.elasticsearch_password = ''
+    context 'when credentials are embedded in url' do
+      let(:elasticsearch_url) { 'http://username:password@example.com,https://test:test@example.org:9200' }
 
-      expect(setting.elasticsearch_url_with_credentials).to match_array([URI.parse('http://foo:@example.com'), URI.parse('https://foo:@example.org:9200')])
+      it 'ignores them and uses elasticsearch_username and elasticsearch_password settings' do
+        expect(setting.elasticsearch_url_with_credentials).to match_array([
+          { scheme: 'http', user: elasticsearch_username, password: elasticsearch_password, host: 'example.com', path: '', port: 80 },
+          { scheme: 'https', user: elasticsearch_username, password: elasticsearch_password, host: 'example.org', path: '', port: 9200 }
+        ])
+      end
     end
 
-    it 'overrides existing embedded credentials' do
-      setting.elasticsearch_url = 'http://username:password@example.com,https://test:test@example.org:9200'
-      setting.elasticsearch_username = 'foo'
-      setting.elasticsearch_password = 'bar'
+    context 'when credential settings are blank' do
+      let(:elasticsearch_username) { nil }
+      let(:elasticsearch_password) { nil }
 
-      expect(setting.elasticsearch_url_with_credentials).to match_array([URI.parse('http://foo:bar@example.com'), URI.parse('https://foo:bar@example.org:9200')])
-    end
+      it 'does not return credential info' do
+        expect(setting.elasticsearch_url_with_credentials).to match_array([
+          { scheme: 'http', host: 'example.com', path: '', port: 80 },
+          { scheme: 'https', host: 'example.org', path: '', port: 9200 }
+        ])
+      end
+
+      context 'and url contains credentials' do
+        let(:elasticsearch_url) { 'http://username:password@example.com,https://test:test@example.org:9200' }
+
+        it 'returns credentials from url' do
+          expect(setting.elasticsearch_url_with_credentials).to match_array([
+            { scheme: 'http', user: 'username', password: 'password', host: 'example.com', path: '', port: 80 },
+            { scheme: 'https', user: 'test', password: 'test', host: 'example.org', path: '', port: 9200 }
+          ])
+        end
+      end
 
-    it 'returns original url if credentials blank' do
-      setting.elasticsearch_url = 'http://username:password@example.com,https://test:test@example.org:9200'
-      setting.elasticsearch_username = ''
-      setting.elasticsearch_password = ''
+      context 'and url contains credentials with special characters' do
+        let(:elasticsearch_url) { 'http://admin:p%40ssword@localhost:9200/' }
 
-      expect(setting.elasticsearch_url_with_credentials).to match_array([URI.parse('http://username:password@example.com'), URI.parse('https://test:test@example.org:9200')])
+        it 'returns decoded credentials from url' do
+          expect(setting.elasticsearch_url_with_credentials).to match_array([
+            { scheme: 'http', user: 'admin', password: 'p@ssword', host: 'localhost', path: '', port: 9200 }
+          ])
+        end
+      end
     end
 
-    it 'encodes the credentials' do
-      setting.elasticsearch_url = 'http://username:password@example.com,https://test:test@example.org:9200'
-      setting.elasticsearch_username = 'foo/admin'
-      setting.elasticsearch_password = 'b@r'
+    context 'when credentials settings have special characters' do
+      let(:elasticsearch_username) { 'foo/admin' }
+      let(:elasticsearch_password) { 'b@r+baz!$' }
 
-      expect(setting.elasticsearch_url_with_credentials).to match_array([
-        URI.parse('http://foo%2Fadmin:b%40r@example.com'),
-        URI.parse('https://foo%2Fadmin:b%40r@example.org:9200')
-      ])
+      it 'returns the correct values' do
+        expect(setting.elasticsearch_url_with_credentials).to match_array([
+          { scheme: 'http', user: elasticsearch_username, password: elasticsearch_password, host: 'example.com', path: '', port: 80 },
+          { scheme: 'https', user: elasticsearch_username, password: elasticsearch_password, host: 'example.org', path: '', port: 9200 }
+        ])
+      end
     end
   end
 
   describe '#elasticsearch_password' do
     it 'does not modify password if it is unchanged in the form' do
       setting.elasticsearch_password = 'foo'
-
       setting.elasticsearch_password = ApplicationSetting::MASK_PASSWORD
 
       expect(setting.elasticsearch_password).to eq('foo')
@@ -404,7 +428,7 @@ RSpec.describe ApplicationSetting do
       )
 
       expect(setting.elasticsearch_config).to eq(
-        url: [URI.parse('http://foo:bar@example.com:9200')],
+        url: [Gitlab::Elastic::Helper.connection_settings(uri: URI.parse('http://foo:bar@example.com:9200'))],
         aws: false,
         aws_region:     'test-region',
         aws_access_key: 'test-access-key',