Merge branch 'ee-49565-ssh-push-mirroring' into 'master'

EE port of "backport SSH host key detection code to CE"

See merge request gitlab-org/gitlab-ee!8065

app/controllers/projects/mirrors_controller.rb

@@ -46,6 +46,22 @@ class Projects::MirrorsController < Projects::ApplicationController
     redirect_to_repository_settings(project, anchor: 'js-push-remote-settings')
   end
 
+  def ssh_host_keys
+    lookup = SshHostKey.new(project: project, url: params[:ssh_url], compare_host_keys: params[:compare_host_keys])
+
+    if lookup.error.present?
+      # Failed to read keys
+      render json: { message: lookup.error }, status: :bad_request
+    elsif lookup.known_hosts.nil?
+      # Still working, come back later
+      render body: nil, status: :no_content
+    else
+      render json: lookup
+    end
+  rescue ArgumentError => err
+    render json: { message: err.message }, status: :bad_request
+  end
+
   private
 
   def remote_mirror

ee/app/models/ssh_host_key.rb → app/models/ssh_host_key.rb

+# frozen_string_literal: true
+
 # Detected SSH host keys are transiently stored in Redis
 class SshHostKey
   class Fingerprint < Gitlab::SSHPublicKey
@@ -18,15 +20,16 @@ class SshHostKey
 
   self.reactive_cache_key = ->(key) { [key.class.to_s, key.id] }
 
-  # Do not refresh the data in the background - it is not expected to change
+  # Do not refresh the data in the background - it is not expected to change.
+  # This is achieved by making the lifetime shorter than the refresh interval.
   self.reactive_cache_refresh_interval = 15.minutes
   self.reactive_cache_lifetime = 10.minutes
 
   def self.find_by(opts = {})
-    id = opts.fetch(:id, "")
-    project_id, url = id.split(':', 2)
+    return nil unless opts.key?(:id)
 
-    project = Project.where(id: project_id).includes(:import_data).first
+    project_id, url = opts[:id].split(':', 2)
+    project = Project.find_by(id: project_id)
 
     project.presence && new(project: project, url: url)
   end
@@ -41,11 +44,12 @@ class SshHostKey
       .select(&:valid?)
   end
 
-  attr_reader :project, :url
+  attr_reader :project, :url, :compare_host_keys
 
-  def initialize(project:, url:)
+  def initialize(project:, url:, compare_host_keys: nil)
     @project = project
     @url = normalize_url(url)
+    @compare_host_keys = compare_host_keys
   end
 
   def id
@@ -54,7 +58,7 @@ class SshHostKey
 
   def as_json(*)
     {
-      changes_project_import_data: changes_project_import_data?,
+      host_keys_changed: host_keys_changed?,
       fingerprints: fingerprints,
       known_hosts: known_hosts
     }
@@ -68,15 +72,10 @@ class SshHostKey
     @fingerprints ||= self.class.fingerprint_host_keys(known_hosts)
   end
 
-  # Returns true if the known_hosts data differs from that currently set for
-  # `project.import_data.ssh_known_hosts`. Ordering is ignored.
-  #
-  # Ordering is ignored
-  def changes_project_import_data?
-    our_known_hosts = known_hosts
-    project_known_hosts = project.import_data&.ssh_known_hosts
-
-    cleanup(our_known_hosts.to_s) != cleanup(project_known_hosts.to_s)
+  # Returns true if the known_hosts data differs from the version passed in at
+  # initialization as `compare_host_keys`. Comments, ordering, etc, is ignored
+  def host_keys_changed?
+    cleanup(known_hosts) != cleanup(compare_host_keys)
   end
 
   def error
@@ -98,13 +97,13 @@ class SshHostKey
 
     # ssh-keyscan returns an exit code 0 in several error conditions, such as an
     # unknown hostname, so check both STDERR and the exit code
-    if !status.success? || errors.present?
+    if status.success? && !errors.present?
+      { known_hosts: known_hosts }
+    else
       Rails.logger.debug("Failed to detect SSH host keys for #{id}: #{errors}")
 
-      return { error: 'Failed to detect SSH host keys' }
+      { error: 'Failed to detect SSH host keys' }
     end
-
-    { known_hosts: known_hosts }
   end
 
   private
@@ -112,9 +111,9 @@ class SshHostKey
   # Remove comments and duplicate entries
   def cleanup(data)
     data
+      .to_s
       .each_line
-      .map { |line| line unless line.start_with?('#') || line.chomp.empty? }
-      .compact
+      .reject { |line| line.start_with?('#') || line.chomp.empty? }
       .uniq
       .sort
       .join

ee/app/assets/javascripts/mirrors/mirror_pull.js

@@ -13,6 +13,7 @@ export default class MirrorPull {
     this.$form = $(formSelector);
 
     this.$repositoryUrl = this.$form.find('.js-repo-url');
+    this.$knownHosts = this.$form.find('.js-known-hosts');
 
     this.$sectionSSHHostKeys = this.$form.find('.js-ssh-host-keys-section');
     this.$hostKeysInformation = this.$form.find('.js-fingerprint-ssh-info');
@@ -34,7 +35,7 @@ export default class MirrorPull {
     this.handleRepositoryUrlInput(true);
 
     this.$repositoryUrl.on('keyup', () => this.handleRepositoryUrlInput());
-    this.$form.find('.js-known-hosts').on('keyup', e => this.handleSSHKnownHostsInput(e));
+    this.$knownHosts.on('keyup', e => this.handleSSHKnownHostsInput(e));
     this.$dropdownAuthType.on('change', e => this.handleAuthTypeChange(e));
     this.$btnDetectHostKeys.on('click', e => this.handleDetectHostKeys(e));
     this.$btnSSHHostsShowAdvanced.on('click', e => this.handleSSHHostsAdvanced(e));
@@ -85,6 +86,7 @@ export default class MirrorPull {
   handleDetectHostKeys() {
     const projectMirrorSSHEndpoint = this.$form.data('project-mirror-ssh-endpoint');
     const repositoryUrl = this.$repositoryUrl.val();
+    const currentKnownHosts = this.$knownHosts.val();
     const $btnLoadSpinner = this.$btnDetectHostKeys.find('.js-spinner');
 
     // Disable button while we make request
@@ -94,7 +96,7 @@ export default class MirrorPull {
     // Make backOff polling to get data
     backOff((next, stop) => {
       axios
-        .get(`${projectMirrorSSHEndpoint}?ssh_url=${repositoryUrl}`)
+        .get(`${projectMirrorSSHEndpoint}?ssh_url=${repositoryUrl}&compare_host_keys=${encodeURIComponent(currentKnownHosts)}`)
         .then(({ data, status }) => {
           if (status === 204) {
             this.backOffRequestCounter += 1;
@@ -114,7 +116,7 @@ export default class MirrorPull {
         // Once data is received, we show verification info along with Host keys and fingerprints
         this.$hostKeysInformation
           .find('.js-fingerprint-verification')
-          .collapse(res.changes_project_import_data ? 'hide' : 'show');
+          .collapse(res.host_keys_changed ? 'hide' : 'show');
         if (res.known_hosts && res.fingerprints) {
           this.showSSHInformation(res);
         }

ee/app/controllers/ee/projects/mirrors_controller.rb

@@ -4,22 +4,6 @@ module EE
       extend ::Gitlab::Utils::Override
       extend ActiveSupport::Concern
 
-      def ssh_host_keys
-        lookup = SshHostKey.new(project: project, url: params[:ssh_url])
-
-        if lookup.error.present?
-          # Failed to read keys
-          render json: { message: lookup.error }, status: :bad_request
-        elsif lookup.known_hosts.nil?
-          # Still working, come back later
-          render body: nil, status: :no_content
-        else
-          render json: lookup
-        end
-      rescue ArgumentError => err
-        render json: { message: err.message }, status: :bad_request
-      end
-
       override :update
       def update
         result = ::Projects::UpdateService.new(project, current_user, safe_mirror_params).execute

ee/spec/controllers/projects/mirrors_controller_spec.rb

@@ -214,67 +214,6 @@ describe Projects::MirrorsController do
     end
   end
 
-  describe '#ssh_host_keys', :use_clean_rails_memory_store_caching do
-    let(:project) { create(:project) }
-    let(:cache) { SshHostKey.new(project: project, url: "ssh://example.com:22") }
-
-    before do
-      sign_in(project.owner)
-    end
-
-    context 'invalid URLs' do
-      where(url: %w[INVALID git@example.com:foo/bar.git ssh://git@example.com:foo/bar.git])
-
-      with_them do
-        it 'returns an error with a 400 response' do
-          do_get(project, url)
-
-          expect(response).to have_gitlab_http_status(400)
-          expect(json_response).to eq('message' => 'Invalid URL')
-        end
-      end
-    end
-
-    context 'no data in cache' do
-      it 'requests the cache to be filled and returns a 204 response' do
-        expect(ReactiveCachingWorker).to receive(:perform_async).with(cache.class, cache.id).at_least(:once)
-
-        do_get(project)
-
-        expect(response).to have_gitlab_http_status(204)
-      end
-    end
-
-    context 'error in the cache' do
-      it 'returns the error with a 400 response' do
-        stub_reactive_cache(cache, error: 'An error')
-
-        do_get(project)
-
-        expect(response).to have_gitlab_http_status(400)
-        expect(json_response).to eq('message' => 'An error')
-      end
-    end
-
-    context 'data in the cache' do
-      let(:ssh_key) { 'ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAfuCHKVTjquxvt6CM6tdG4SLp1Btn/nOeHHE5UOzRdf' }
-      let(:ssh_fp) { { type: 'ed25519', bits: 256, fingerprint: '2e:65:6a:c8:cf:bf:b2:8b:9a:bd:6d:9f:11:5c:12:16', index: 0 } }
-
-      it 'returns the data with a 200 response' do
-        stub_reactive_cache(cache, known_hosts: ssh_key)
-
-        do_get(project)
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response).to eq('known_hosts' => ssh_key, 'fingerprints' => [ssh_fp.stringify_keys], 'changes_project_import_data' => true)
-      end
-    end
-
-    def do_get(project, url = 'ssh://example.com')
-      get :ssh_host_keys, namespace_id: project.namespace, project_id: project, ssh_url: url
-    end
-  end
-
   def do_put(project, options, extra_attrs = {})
     attrs = extra_attrs.merge(namespace_id: project.namespace.to_param, project_id: project.to_param)
     attrs[:project] = options

spec/controllers/projects/mirrors_controller_spec.rb

@@ -63,6 +63,69 @@ describe Projects::MirrorsController do
     end
   end
 
+  describe '#ssh_host_keys', :use_clean_rails_memory_store_caching do
+    let(:project) { create(:project) }
+    let(:cache) { SshHostKey.new(project: project, url: "ssh://example.com:22") }
+
+    before do
+      sign_in(project.owner)
+    end
+
+    context 'invalid URLs' do
+      %w[
+        INVALID
+        git@example.com:foo/bar.git
+        ssh://git@example.com:foo/bar.git
+      ].each do |url|
+        it "returns an error with a 400 response for URL #{url.inspect}" do
+          do_get(project, url)
+
+          expect(response).to have_gitlab_http_status(400)
+          expect(json_response).to eq('message' => 'Invalid URL')
+        end
+      end
+    end
+
+    context 'no data in cache' do
+      it 'requests the cache to be filled and returns a 204 response' do
+        expect(ReactiveCachingWorker).to receive(:perform_async).with(cache.class, cache.id).at_least(:once)
+
+        do_get(project)
+
+        expect(response).to have_gitlab_http_status(204)
+      end
+    end
+
+    context 'error in the cache' do
+      it 'returns the error with a 400 response' do
+        stub_reactive_cache(cache, error: 'An error')
+
+        do_get(project)
+
+        expect(response).to have_gitlab_http_status(400)
+        expect(json_response).to eq('message' => 'An error')
+      end
+    end
+
+    context 'data in the cache' do
+      let(:ssh_key) { 'ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAfuCHKVTjquxvt6CM6tdG4SLp1Btn/nOeHHE5UOzRdf' }
+      let(:ssh_fp) { { type: 'ed25519', bits: 256, fingerprint: '2e:65:6a:c8:cf:bf:b2:8b:9a:bd:6d:9f:11:5c:12:16', index: 0 } }
+
+      it 'returns the data with a 200 response' do
+        stub_reactive_cache(cache, known_hosts: ssh_key)
+
+        do_get(project)
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response).to eq('known_hosts' => ssh_key, 'fingerprints' => [ssh_fp.stringify_keys], 'host_keys_changed' => true)
+      end
+    end
+
+    def do_get(project, url = 'ssh://example.com')
+      get :ssh_host_keys, namespace_id: project.namespace, project_id: project, ssh_url: url
+    end
+  end
+
   def do_put(project, options, extra_attrs = {})
     attrs = extra_attrs.merge(namespace_id: project.namespace.to_param, project_id: project.to_param)
     attrs[:project] = options

ee/spec/models/ssh_host_key_spec.rb → spec/models/ssh_host_key_spec.rb

@@ -33,6 +33,8 @@ describe SshHostKey do
   let(:extra) { known_hosts + "foo\nbar\n" }
   let(:reversed) { known_hosts.lines.reverse.join }
 
+  let(:compare_host_keys) { nil }
+
   def stub_ssh_keyscan(args, status: true, stdout: "", stderr: "")
     stdin = StringIO.new
     stdout = double(:stdout, read: stdout)
@@ -44,9 +46,9 @@ describe SshHostKey do
     stdin
   end
 
-  let(:project) { build(:project, :mirror) }
+  let(:project) { build(:project) }
 
-  subject(:ssh_host_key) { described_class.new(project: project, url: 'ssh://example.com:2222') }
+  subject(:ssh_host_key) { described_class.new(project: project, url: 'ssh://example.com:2222', compare_host_keys: compare_host_keys) }
 
   describe '#fingerprints', :use_clean_rails_memory_store_caching do
     it 'returns an array of indexed fingerprints when the cache is filled' do
@@ -82,8 +84,8 @@ describe SshHostKey do
     end
   end
 
-  describe '#changes_project_import_data?' do
-    where(:a, :b, :result) do
+  describe '#host_keys_changed?' do
+    where(:known_hosts_a, :known_hosts_b, :result) do
       known_hosts | extra       | true
       known_hosts | "foo\n"     | true
       known_hosts | ''          | true
@@ -97,21 +99,29 @@ describe SshHostKey do
     end
 
     with_them do
-      subject { ssh_host_key.changes_project_import_data? }
+      let(:compare_host_keys) { known_hosts_b }
+
+      subject { ssh_host_key.host_keys_changed? }
 
-      it "(normal)" do
-        expect(ssh_host_key).to receive(:known_hosts).and_return(a)
-        project.import_data.ssh_known_hosts = b
+      context '(normal)' do
+        let(:compare_host_keys) { known_hosts_b }
 
-        is_expected.to eq(result)
+        before do
+          expect(ssh_host_key).to receive(:known_hosts).and_return(known_hosts_a)
+        end
+
+        it { is_expected.to eq(result) }
       end
 
       # Comparisons should be symmetrical, so test the reverse too
-      it "(reversed)" do
-        expect(ssh_host_key).to receive(:known_hosts).and_return(b)
-        project.import_data.ssh_known_hosts = a
+      context '(reversed)' do
+        let(:compare_host_keys) { known_hosts_a }
+
+        before do
+          expect(ssh_host_key).to receive(:known_hosts).and_return(known_hosts_b)
+        end
 
-        is_expected.to eq(result)
+        it { is_expected.to eq(result) }
       end
     end
   end