Merge branch '60071-remove-gitlab-keys-usage' into 'master'

Remove usage of gitlab-shell gitlab-keys script

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

config/initializers/1_settings.rb

@@ -515,7 +515,7 @@ Settings['sidekiq']['log_format'] ||= 'default'
 Settings['gitlab_shell'] ||= Settingslogic.new({})
 Settings.gitlab_shell['path']           = Settings.absolute(Settings.gitlab_shell['path'] || Settings.gitlab['user_home'] + '/gitlab-shell/')
 Settings.gitlab_shell['hooks_path']     = :deprecated_use_gitlab_shell_path_instead
-Settings.gitlab_shell['authorized_keys_file'] ||= nil
+Settings.gitlab_shell['authorized_keys_file'] ||= File.join(Dir.home, '.ssh', 'authorized_keys')
 Settings.gitlab_shell['secret_file'] ||= Rails.root.join('.gitlab_shell_secret')
 Settings.gitlab_shell['receive_pack']   = true if Settings.gitlab_shell['receive_pack'].nil?
 Settings.gitlab_shell['upload_pack']    = true if Settings.gitlab_shell['upload_pack'].nil?

lib/gitlab/authorized_keys.rb

@@ -13,6 +13,24 @@ module Gitlab
       @logger = logger
     end
 
+    # Checks if the file is accessible or not
+    #
+    # @return [Boolean]
+    def accessible?
+      open_authorized_keys_file('r') { true }
+    rescue Errno::ENOENT, Errno::EACCES
+      false
+    end
+
+    # Creates the authorized_keys file if it doesn't exist
+    #
+    # @return [Boolean]
+    def create
+      open_authorized_keys_file(File::CREAT) { true }
+    rescue Errno::EACCES
+      false
+    end
+
     # Add id and its key to the authorized_keys file
     #
     # @param [String] id identifier of key prefixed by `key-`
@@ -102,10 +120,14 @@ module Gitlab
       []
     end
 
+    def file
+      @file ||= Gitlab.config.gitlab_shell.authorized_keys_file
+    end
+
     private
 
     def lock(timeout = 10)
-      File.open("#{authorized_keys_file}.lock", "w+") do |f|
+      File.open("#{file}.lock", "w+") do |f|
         f.flock File::LOCK_EX
         Timeout.timeout(timeout) { yield }
       ensure
@@ -114,7 +136,7 @@ module Gitlab
     end
 
     def open_authorized_keys_file(mode)
-      File.open(authorized_keys_file, mode, 0o600) do |file|
+      File.open(file, mode, 0o600) do |file|
         file.chmod(0o600)
         yield file
       end
@@ -141,9 +163,5 @@ module Gitlab
     def strip(key)
       key.split(/[ ]+/)[0, 2].join(' ')
     end
-
-    def authorized_keys_file
-      Gitlab.config.gitlab_shell.authorized_keys_file
-    end
   end
 end

lib/gitlab/shell.rb

@@ -165,16 +165,7 @@ module Gitlab
     def add_key(key_id, key_content)
       return unless self.authorized_keys_enabled?
 
-      if shell_out_for_gitlab_keys?
-        gitlab_shell_fast_execute([
-          gitlab_shell_keys_path,
-          'add-key',
-          key_id,
-          strip_key(key_content)
-        ])
-      else
-        gitlab_authorized_keys.add_key(key_id, key_content)
-      end
+      gitlab_authorized_keys.add_key(key_id, key_content)
     end
 
     # Batch-add keys to authorized_keys
@@ -184,19 +175,7 @@ module Gitlab
     def batch_add_keys(keys)
       return unless self.authorized_keys_enabled?
 
-      if shell_out_for_gitlab_keys?
-        begin
-          IO.popen("#{gitlab_shell_keys_path} batch-add-keys", 'w') do |io|
-            add_keys_to_io(keys, io)
-          end
-
-          $?.success?
-        rescue Error
-          false
-        end
-      else
-        gitlab_authorized_keys.batch_add_keys(keys)
-      end
+      gitlab_authorized_keys.batch_add_keys(keys)
     end
 
     # Remove ssh key from authorized_keys
@@ -207,11 +186,7 @@ module Gitlab
     def remove_key(id, _ = nil)
       return unless self.authorized_keys_enabled?
 
-      if shell_out_for_gitlab_keys?
-        gitlab_shell_fast_execute([gitlab_shell_keys_path, 'rm-key', id])
-      else
-        gitlab_authorized_keys.rm_key(id)
-      end
+      gitlab_authorized_keys.rm_key(id)
     end
 
     # Remove all ssh keys from gitlab shell
@@ -222,11 +197,7 @@ module Gitlab
     def remove_all_keys
       return unless self.authorized_keys_enabled?
 
-      if shell_out_for_gitlab_keys?
-        gitlab_shell_fast_execute([gitlab_shell_keys_path, 'clear'])
-      else
-        gitlab_authorized_keys.clear
-      end
+      gitlab_authorized_keys.clear
     end
 
     # Remove ssh keys from gitlab shell that are not in the DB
@@ -341,14 +312,6 @@ module Gitlab
       File.join(Gitlab.config.repositories.storages[storage].legacy_disk_path, dir_name)
     end
 
-    def gitlab_shell_projects_path
-      File.join(gitlab_shell_path, 'bin', 'gitlab-projects')
-    end
-
-    def gitlab_shell_keys_path
-      File.join(gitlab_shell_path, 'bin', 'gitlab-keys')
-    end
-
     def authorized_keys_enabled?
       # Return true if nil to ensure the authorized_keys methods work while
       # fixing the authorized_keys file during migration.
@@ -359,35 +322,6 @@ module Gitlab
 
     private
 
-    def shell_out_for_gitlab_keys?
-      Gitlab.config.gitlab_shell.authorized_keys_file.blank?
-    end
-
-    def gitlab_shell_fast_execute(cmd)
-      output, status = gitlab_shell_fast_execute_helper(cmd)
-
-      return true if status.zero?
-
-      Rails.logger.error("gitlab-shell failed with error #{status}: #{output}") # rubocop:disable Gitlab/RailsLogger
-      false
-    end
-
-    def gitlab_shell_fast_execute_raise_error(cmd, vars = {})
-      output, status = gitlab_shell_fast_execute_helper(cmd, vars)
-
-      raise Error, output unless status.zero?
-
-      true
-    end
-
-    def gitlab_shell_fast_execute_helper(cmd, vars = {})
-      vars.merge!(ENV.to_h.slice(*GITLAB_SHELL_ENV_VARS))
-
-      # Don't pass along the entire parent environment to prevent gitlab-shell
-      # from wasting I/O by searching through GEM_PATH
-      Bundler.with_original_env { Popen.popen(cmd, nil, vars) }
-    end
-
     def git_timeout
       Gitlab.config.gitlab_shell.git_timeout
     end
@@ -407,16 +341,8 @@ module Gitlab
     def batch_read_key_ids(batch_size: 100, &block)
       return unless self.authorized_keys_enabled?
 
-      if shell_out_for_gitlab_keys?
-        IO.popen("#{gitlab_shell_keys_path} list-key-ids") do |key_id_stream|
-          key_id_stream.lazy.each_slice(batch_size) do |lines|
-            yield(lines.map { |l| l.chomp.to_i })
-          end
-        end
-      else
-        gitlab_authorized_keys.list_key_ids.lazy.each_slice(batch_size) do |key_ids|
-          yield(key_ids)
-        end
+      gitlab_authorized_keys.list_key_ids.lazy.each_slice(batch_size) do |key_ids|
+        yield(key_ids)
       end
     end
 

lib/system_check/app/authorized_keys_permission_check.rb

+# frozen_string_literal: true
+
+module SystemCheck
+  module App
+    class AuthorizedKeysPermissionCheck < SystemCheck::BaseCheck
+      set_name 'Is authorized keys file accessible?'
+      set_skip_reason 'skipped (authorized keys not enabled)'
+
+      def skip?
+        !authorized_keys_enabled?
+      end
+
+      def check?
+        authorized_keys.accessible?
+      end
+
+      def repair!
+        authorized_keys.create
+      end
+
+      def show_error
+        try_fixing_it([
+          "sudo chmod 700 #{File.dirname(authorized_keys.file)}",
+          "touch #{authorized_keys.file}",
+          "sudo chmod 600 #{authorized_keys.file}"
+        ])
+        fix_and_rerun
+      end
+
+      private
+
+      def authorized_keys_enabled?
+        Gitlab::CurrentSettings.current_application_settings.authorized_keys_enabled
+      end
+
+      def authorized_keys
+        @authorized_keys ||= Gitlab::AuthorizedKeys.new
+      end
+    end
+  end
+end

lib/system_check/rake_task/app_task.rb

@@ -30,7 +30,8 @@ module SystemCheck
           SystemCheck::App::RubyVersionCheck,
           SystemCheck::App::GitVersionCheck,
           SystemCheck::App::GitUserDefaultSSHConfigCheck,
-          SystemCheck::App::ActiveUsersCheck
+          SystemCheck::App::ActiveUsersCheck,
+          SystemCheck::App::AuthorizedKeysPermissionCheck
         ]
       end
     end

spec/lib/gitlab/authorized_keys_spec.rb

@@ -5,10 +5,81 @@ require 'spec_helper'
 describe Gitlab::AuthorizedKeys do
   let(:logger) { double('logger').as_null_object }
 
-  subject { described_class.new(logger) }
+  subject(:authorized_keys) { described_class.new(logger) }
+
+  describe '#accessible?' do
+    subject { authorized_keys.accessible? }
+
+    context 'authorized_keys file exists' do
+      before do
+        create_authorized_keys_fixture
+      end
+
+      after do
+        delete_authorized_keys_file
+      end
+
+      context 'can open file' do
+        it { is_expected.to be_truthy }
+      end
+
+      context 'cannot open file' do
+        before do
+          allow(File).to receive(:open).and_raise(Errno::EACCES)
+        end
+
+        it { is_expected.to be_falsey }
+      end
+    end
+
+    context 'authorized_keys file does not exist' do
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#create' do
+    subject { authorized_keys.create }
+
+    context 'authorized_keys file exists' do
+      before do
+        create_authorized_keys_fixture
+      end
+
+      after do
+        delete_authorized_keys_file
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'authorized_keys file does not exist' do
+      after do
+        delete_authorized_keys_file
+      end
+
+      it 'creates authorized_keys file' do
+        expect(subject).to be_truthy
+        expect(File.exist?(tmp_authorized_keys_path)).to be_truthy
+      end
+    end
+
+    context 'cannot create file' do
+      before do
+        allow(File).to receive(:open).and_raise(Errno::EACCES)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
 
   describe '#add_key' do
+    let(:id) { 'key-741' }
+
+    subject { authorized_keys.add_key(id, key) }
+
     context 'authorized_keys file exists' do
+      let(:key) { 'ssh-rsa AAAAB3NzaDAxx2E trailing garbage' }
+
       before do
         create_authorized_keys_fixture
       end
@@ -21,19 +92,20 @@ describe Gitlab::AuthorizedKeys do
         auth_line = "command=\"#{Gitlab.config.gitlab_shell.path}/bin/gitlab-shell key-741\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa AAAAB3NzaDAxx2E"
 
         expect(logger).to receive(:info).with('Adding key (key-741): ssh-rsa AAAAB3NzaDAxx2E')
-        expect(subject.add_key('key-741', 'ssh-rsa AAAAB3NzaDAxx2E trailing garbage'))
-          .to be_truthy
+        expect(subject).to be_truthy
         expect(File.read(tmp_authorized_keys_path)).to eq("existing content\n#{auth_line}\n")
       end
     end
 
     context 'authorized_keys file does not exist' do
+      let(:key) { 'ssh-rsa AAAAB3NzaDAxx2E' }
+
       before do
         delete_authorized_keys_file
       end
 
       it 'creates the file' do
-        expect(subject.add_key('key-741', 'ssh-rsa AAAAB3NzaDAxx2E')).to be_truthy
+        expect(subject).to be_truthy
         expect(File.exist?(tmp_authorized_keys_path)).to be_truthy
       end
     end
@@ -47,6 +119,8 @@ describe Gitlab::AuthorizedKeys do
       ]
     end
 
+    subject { authorized_keys.batch_add_keys(keys) }
+
     context 'authorized_keys file exists' do
       before do
         create_authorized_keys_fixture
@@ -62,7 +136,7 @@ describe Gitlab::AuthorizedKeys do
 
         expect(logger).to receive(:info).with('Adding key (key-12): ssh-dsa ASDFASGADG')
         expect(logger).to receive(:info).with('Adding key (key-123): ssh-rsa GFDGDFSGSDFG')
-        expect(subject.batch_add_keys(keys)).to be_truthy
+        expect(subject).to be_truthy
         expect(File.read(tmp_authorized_keys_path)).to eq("existing content\n#{auth_line1}\n#{auth_line2}\n")
       end
 
@@ -70,7 +144,7 @@ describe Gitlab::AuthorizedKeys do
         let(:keys) { [double(shell_id: 'key-123', key: "ssh-rsa A\tSDFA\nSGADG")] }
 
         it "doesn't add keys" do
-          expect(subject.batch_add_keys(keys)).to be_falsey
+          expect(subject).to be_falsey
           expect(File.read(tmp_authorized_keys_path)).to eq("existing content\n")
         end
       end
@@ -82,16 +156,28 @@ describe Gitlab::AuthorizedKeys do
       end
 
       it 'creates the file' do
-        expect(subject.batch_add_keys(keys)).to be_truthy
+        expect(subject).to be_truthy
         expect(File.exist?(tmp_authorized_keys_path)).to be_truthy
       end
     end
   end
 
   describe '#rm_key' do
+    let(:key) { 'key-741' }
+
+    subject { authorized_keys.rm_key(key) }
+
     context 'authorized_keys file exists' do
+      let(:other_line) { "command=\"#{Gitlab.config.gitlab_shell.path}/bin/gitlab-shell key-742\",options ssh-rsa AAAAB3NzaDAxx2E" }
+      let(:delete_line) { "command=\"#{Gitlab.config.gitlab_shell.path}/bin/gitlab-shell key-741\",options ssh-rsa AAAAB3NzaDAxx2E" }
+
       before do
         create_authorized_keys_fixture
+
+        File.open(tmp_authorized_keys_path, 'a') do |auth_file|
+          auth_file.puts delete_line
+          auth_file.puts other_line
+        end
       end
 
       after do
@@ -99,16 +185,10 @@ describe Gitlab::AuthorizedKeys do
       end
 
       it "removes the right line" do
-        other_line = "command=\"#{Gitlab.config.gitlab_shell.path}/bin/gitlab-shell key-742\",options ssh-rsa AAAAB3NzaDAxx2E"
-        delete_line = "command=\"#{Gitlab.config.gitlab_shell.path}/bin/gitlab-shell key-741\",options ssh-rsa AAAAB3NzaDAxx2E"
         erased_line = delete_line.gsub(/./, '#')
-        File.open(tmp_authorized_keys_path, 'a') do |auth_file|
-          auth_file.puts delete_line
-          auth_file.puts other_line
-        end
 
         expect(logger).to receive(:info).with('Removing key (key-741)')
-        expect(subject.rm_key('key-741')).to be_truthy
+        expect(subject).to be_truthy
         expect(File.read(tmp_authorized_keys_path)).to eq("existing content\n#{erased_line}\n#{other_line}\n")
       end
     end
@@ -118,13 +198,13 @@ describe Gitlab::AuthorizedKeys do
         delete_authorized_keys_file
       end
 
-      it 'returns false' do
-        expect(subject.rm_key('key-741')).to be_falsey
-      end
+      it { is_expected.to be_falsey }
     end
   end
 
   describe '#clear' do
+    subject { authorized_keys.clear }
+
     context 'authorized_keys file exists' do
       before do
         create_authorized_keys_fixture
@@ -134,9 +214,7 @@ describe Gitlab::AuthorizedKeys do
         delete_authorized_keys_file
       end
 
-      it "returns true" do
-        expect(subject.clear).to be_truthy
-      end
+      it { is_expected.to be_truthy }
     end
 
     context 'authorized_keys file does not exist' do
@@ -144,13 +222,13 @@ describe Gitlab::AuthorizedKeys do
         delete_authorized_keys_file
       end
 
-      it "still returns true" do
-        expect(subject.clear).to be_truthy
-      end
+      it { is_expected.to be_truthy }
     end
   end
 
   describe '#list_key_ids' do
+    subject { authorized_keys.list_key_ids }
+
     context 'authorized_keys file exists' do
       before do
         create_authorized_keys_fixture(
@@ -163,9 +241,7 @@ describe Gitlab::AuthorizedKeys do
         delete_authorized_keys_file
       end
 
-      it 'returns array of key IDs' do
-        expect(subject.list_key_ids).to eq([1, 2, 3, 9000])
-      end
+      it { is_expected.to eq([1, 2, 3, 9000]) }
     end
 
     context 'authorized_keys file does not exist' do
@@ -173,9 +249,7 @@ describe Gitlab::AuthorizedKeys do
         delete_authorized_keys_file
       end
 
-      it 'returns an empty array' do
-        expect(subject.list_key_ids).to be_empty
-      end
+      it { is_expected.to be_empty }
     end
   end
 

spec/lib/system_check/app/authorized_keys_permission_check_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe SystemCheck::App::AuthorizedKeysPermissionCheck do
+  subject(:system_check) { described_class.new }
+
+  describe '#skip?' do
+    subject { system_check.skip? }
+
+    context 'authorized keys enabled' do
+      it { is_expected.to eq(false) }
+    end
+
+    context 'authorized keys not enabled' do
+      before do
+        stub_application_setting(authorized_keys_enabled: false)
+      end
+
+      it { is_expected.to eq(true) }
+    end
+  end
+
+  describe '#check?' do
+    subject { system_check.check? }
+
+    before do
+      expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
+        allow(instance).to receive(:accessible?) { accessible? }
+      end
+    end
+
+    context 'authorized keys is accessible' do
+      let(:accessible?) { true }
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'authorized keys is not accessible' do
+      let(:accessible?) { false }
+
+      it { is_expected.to eq(false) }
+    end
+  end
+
+  describe '#repair!' do
+    subject { system_check.repair! }
+
+    before do
+      expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
+        allow(instance).to receive(:create) { created }
+      end
+    end
+
+    context 'authorized_keys file created' do
+      let(:created) { true }
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'authorized_keys file is not created' do
+      let(:created) { false }
+
+      it { is_expected.to eq(false) }
+    end
+  end
+end