Remove the fallback path from gitlab-ce

lib/gitlab/shell.rb

@@ -165,17 +165,8 @@ 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
-    end
 
     # Batch-add keys to authorized_keys
     #
@@ -184,20 +175,8 @@ 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
-    end
 
     # Remove ssh key from authorized_keys
     #
@@ -207,12 +186,8 @@ 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
-    end
 
     # Remove all ssh keys from gitlab shell
     #
@@ -222,12 +197,8 @@ 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
-    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,18 +341,10 @@ 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
     end
-    end
 
     def strip_key(key)
       key.split(/[ ]+/)[0, 2].join(' ')

spec/lib/gitlab/shell_spec.rb

@@ -52,29 +52,6 @@ describe Gitlab::Shell do
 
   describe '#add_key' do
     context 'when authorized_keys_enabled is true' do
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          allow(gitlab_shell)
-            .to receive(:gitlab_shell_keys_path)
-            .and_return(:gitlab_shell_keys_path)
-        end
-
-        it 'calls #gitlab_shell_fast_execute with add-key command' do
-          expect(gitlab_shell)
-            .to receive(:gitlab_shell_fast_execute)
-            .with([
-              :gitlab_shell_keys_path,
-              'add-key',
-              'key-123',
-              'ssh-rsa foobar'
-            ])
-
-          gitlab_shell.add_key('key-123', 'ssh-rsa foobar trailing garbage')
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'calls Gitlab::AuthorizedKeys#add_key with id and key' do
         expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
 
@@ -85,62 +62,24 @@ describe Gitlab::Shell do
         gitlab_shell.add_key('key-123', 'ssh-rsa foobar')
       end
     end
-    end
 
     context 'when authorized_keys_enabled is false' do
       before do
         stub_application_setting(authorized_keys_enabled: false)
       end
 
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-        end
-
-        it 'does nothing' do
-          expect(gitlab_shell).not_to receive(:gitlab_shell_fast_execute)
-
-          gitlab_shell.add_key('key-123', 'ssh-rsa foobar trailing garbage')
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'does nothing' do
         expect(Gitlab::AuthorizedKeys).not_to receive(:new)
 
         gitlab_shell.add_key('key-123', 'ssh-rsa foobar trailing garbage')
       end
     end
-    end
 
     context 'when authorized_keys_enabled is nil' do
       before do
         stub_application_setting(authorized_keys_enabled: nil)
       end
 
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          allow(gitlab_shell)
-            .to receive(:gitlab_shell_keys_path)
-            .and_return(:gitlab_shell_keys_path)
-        end
-
-        it 'calls #gitlab_shell_fast_execute with add-key command' do
-          expect(gitlab_shell)
-            .to receive(:gitlab_shell_fast_execute)
-            .with([
-              :gitlab_shell_keys_path,
-              'add-key',
-              'key-123',
-              'ssh-rsa foobar'
-            ])
-
-          gitlab_shell.add_key('key-123', 'ssh-rsa foobar trailing garbage')
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'calls Gitlab::AuthorizedKeys#add_key with id and key' do
         expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
 
@@ -152,47 +91,11 @@ describe Gitlab::Shell do
       end
     end
   end
-  end
 
   describe '#batch_add_keys' do
     let(:keys) { [double(shell_id: 'key-123', key: 'ssh-rsa foobar')] }
 
     context 'when authorized_keys_enabled is true' do
-      context 'authorized_keys_file not set' do
-        let(:io) { double }
-
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-        end
-
-        context 'valid keys' do
-          before do
-            allow(gitlab_shell)
-              .to receive(:gitlab_shell_keys_path)
-              .and_return(:gitlab_shell_keys_path)
-          end
-
-          it 'calls gitlab-keys with batch-add-keys command' do
-            expect(IO)
-              .to receive(:popen)
-              .with("gitlab_shell_keys_path batch-add-keys", 'w')
-              .and_yield(io)
-
-            expect(io).to receive(:puts).with("key-123\tssh-rsa foobar")
-            expect(gitlab_shell.batch_add_keys(keys)).to be_truthy
-          end
-        end
-
-        context 'invalid keys' do
-          let(:keys) { [double(shell_id: 'key-123', key: "ssh-rsa A\tSDFA\nSGADG")] }
-
-          it 'catches failure and returns false' do
-            expect(gitlab_shell.batch_add_keys(keys)).to be_falsey
-          end
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'calls Gitlab::AuthorizedKeys#batch_add_keys with keys to be added' do
         expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
 
@@ -203,62 +106,24 @@ describe Gitlab::Shell do
         gitlab_shell.batch_add_keys(keys)
       end
     end
-    end
 
     context 'when authorized_keys_enabled is false' do
       before do
         stub_application_setting(authorized_keys_enabled: false)
       end
 
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-        end
-
-        it 'does nothing' do
-          expect(IO).not_to receive(:popen)
-
-          gitlab_shell.batch_add_keys(keys)
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'does nothing' do
         expect(Gitlab::AuthorizedKeys).not_to receive(:new)
 
         gitlab_shell.batch_add_keys(keys)
       end
     end
-    end
 
     context 'when authorized_keys_enabled is nil' do
       before do
         stub_application_setting(authorized_keys_enabled: nil)
       end
 
-      context 'authorized_keys_file not set' do
-        let(:io) { double }
-
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          allow(gitlab_shell)
-            .to receive(:gitlab_shell_keys_path)
-            .and_return(:gitlab_shell_keys_path)
-        end
-
-        it 'calls gitlab-keys with batch-add-keys command' do
-          expect(IO)
-            .to receive(:popen)
-            .with("gitlab_shell_keys_path batch-add-keys", 'w')
-            .and_yield(io)
-
-          expect(io).to receive(:puts).with("key-123\tssh-rsa foobar")
-
-          gitlab_shell.batch_add_keys(keys)
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'calls Gitlab::AuthorizedKeys#batch_add_keys with keys to be added' do
         expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
 
@@ -270,32 +135,9 @@ describe Gitlab::Shell do
       end
     end
   end
-  end
 
   describe '#remove_key' do
     context 'when authorized_keys_enabled is true' do
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          allow(gitlab_shell)
-            .to receive(:gitlab_shell_keys_path)
-            .and_return(:gitlab_shell_keys_path)
-        end
-
-        it 'calls #gitlab_shell_fast_execute with rm-key command' do
-          expect(gitlab_shell)
-            .to receive(:gitlab_shell_fast_execute)
-            .with([
-              :gitlab_shell_keys_path,
-              'rm-key',
-              'key-123'
-            ])
-
-          gitlab_shell.remove_key('key-123')
-        end
-      end
-
-      context 'authorized_keys_file not set' do
       it 'calls Gitlab::AuthorizedKeys#rm_key with the key to be removed' do
         expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
         expect(gitlab_authorized_keys).to receive(:rm_key).with('key-123')
@@ -303,61 +145,24 @@ describe Gitlab::Shell do
         gitlab_shell.remove_key('key-123')
       end
     end
-    end
 
     context 'when authorized_keys_enabled is false' do
       before do
         stub_application_setting(authorized_keys_enabled: false)
       end
 
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-        end
-
-        it 'does nothing' do
-          expect(gitlab_shell).not_to receive(:gitlab_shell_fast_execute)
-
-          gitlab_shell.remove_key('key-123')
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'does nothing' do
         expect(Gitlab::AuthorizedKeys).not_to receive(:new)
 
         gitlab_shell.remove_key('key-123')
       end
     end
-    end
 
     context 'when authorized_keys_enabled is nil' do
       before do
         stub_application_setting(authorized_keys_enabled: nil)
       end
 
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          allow(gitlab_shell)
-            .to receive(:gitlab_shell_keys_path)
-            .and_return(:gitlab_shell_keys_path)
-        end
-
-        it 'calls #gitlab_shell_fast_execute with rm-key command' do
-          expect(gitlab_shell)
-            .to receive(:gitlab_shell_fast_execute)
-            .with([
-              :gitlab_shell_keys_path,
-              'rm-key',
-              'key-123'
-            ])
-
-          gitlab_shell.remove_key('key-123')
-        end
-      end
-
-      context 'authorized_keys_file not set' do
       it 'calls Gitlab::AuthorizedKeys#rm_key with the key to be removed' do
         expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
         expect(gitlab_authorized_keys).to receive(:rm_key).with('key-123')
@@ -366,28 +171,9 @@ describe Gitlab::Shell do
       end
     end
   end
-  end
 
   describe '#remove_all_keys' do
     context 'when authorized_keys_enabled is true' do
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          allow(gitlab_shell)
-            .to receive(:gitlab_shell_keys_path)
-            .and_return(:gitlab_shell_keys_path)
-        end
-
-        it 'calls #gitlab_shell_fast_execute with clear command' do
-          expect(gitlab_shell)
-            .to receive(:gitlab_shell_fast_execute)
-            .with([:gitlab_shell_keys_path, 'clear'])
-
-          gitlab_shell.remove_all_keys
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'calls Gitlab::AuthorizedKeys#clear' do
         expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
         expect(gitlab_authorized_keys).to receive(:clear)
@@ -395,57 +181,24 @@ describe Gitlab::Shell do
         gitlab_shell.remove_all_keys
       end
     end
-    end
 
     context 'when authorized_keys_enabled is false' do
       before do
         stub_application_setting(authorized_keys_enabled: false)
       end
 
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-        end
-
-        it 'does nothing' do
-          expect(gitlab_shell).not_to receive(:gitlab_shell_fast_execute)
-
-          gitlab_shell.remove_all_keys
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'does nothing' do
         expect(Gitlab::AuthorizedKeys).not_to receive(:new)
 
         gitlab_shell.remove_all_keys
       end
     end
-    end
 
     context 'when authorized_keys_enabled is nil' do
       before do
         stub_application_setting(authorized_keys_enabled: nil)
       end
 
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          allow(gitlab_shell)
-            .to receive(:gitlab_shell_keys_path)
-            .and_return(:gitlab_shell_keys_path)
-        end
-
-        it 'calls #gitlab_shell_fast_execute with clear command' do
-          expect(gitlab_shell)
-            .to receive(:gitlab_shell_fast_execute)
-            .with([:gitlab_shell_keys_path, 'clear'])
-
-          gitlab_shell.remove_all_keys
-        end
-      end
-
-      context 'authorized_keys_file set' do
       it 'calls Gitlab::AuthorizedKeys#clear' do
         expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
         expect(gitlab_authorized_keys).to receive(:clear)
@@ -454,29 +207,9 @@ describe Gitlab::Shell do
       end
     end
   end
-  end
 
   describe '#remove_keys_not_found_in_db' do
     context 'when keys are in the file that are not in the DB' do
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          gitlab_shell.remove_all_keys
-          gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
-          gitlab_shell.add_key('key-9876', 'ssh-rsa ASDFASDF')
-          @another_key = create(:key) # this one IS in the DB
-        end
-
-        it 'removes the keys' do
-          expect(gitlab_shell).to receive(:remove_key).with('key-1234')
-          expect(gitlab_shell).to receive(:remove_key).with('key-9876')
-          expect(gitlab_shell).not_to receive(:remove_key).with("key-#{@another_key.id}")
-
-          gitlab_shell.remove_keys_not_found_in_db
-        end
-      end
-
-      context 'authorized_keys_file set' do
       before do
         gitlab_shell.remove_all_keys
         gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
@@ -492,25 +225,8 @@ describe Gitlab::Shell do
         gitlab_shell.remove_keys_not_found_in_db
       end
     end
-    end
 
     context 'when keys there are duplicate keys in the file that are not in the DB' do
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          gitlab_shell.remove_all_keys
-          gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
-          gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
-        end
-
-        it 'removes the keys' do
-          expect(gitlab_shell).to receive(:remove_key).with('key-1234')
-
-          gitlab_shell.remove_keys_not_found_in_db
-        end
-      end
-
-      context 'authorized_keys_file set' do
       before do
         gitlab_shell.remove_all_keys
         gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
@@ -523,25 +239,8 @@ describe Gitlab::Shell do
         gitlab_shell.remove_keys_not_found_in_db
       end
     end
-    end
 
     context 'when keys there are duplicate keys in the file that ARE in the DB' do
-      context 'authorized_keys_file not set' do
-        before do
-          stub_gitlab_shell_setting(authorized_keys_file: nil)
-          gitlab_shell.remove_all_keys
-          @key = create(:key)
-          gitlab_shell.add_key(@key.shell_id, @key.key)
-        end
-
-        it 'does not remove the key' do
-          expect(gitlab_shell).not_to receive(:remove_key).with("key-#{@key.id}")
-
-          gitlab_shell.remove_keys_not_found_in_db
-        end
-      end
-
-      context 'authorized_keys_file set' do
       before do
         gitlab_shell.remove_all_keys
         @key = create(:key)
@@ -554,26 +253,9 @@ describe Gitlab::Shell do
         gitlab_shell.remove_keys_not_found_in_db
       end
     end
-    end
 
     unless ENV['CI'] # Skip in CI, it takes 1 minute
       context 'when the first batch can be skipped, but the next batch has keys that are not in the DB' do
-        context 'authorized_keys_file not set' do
-          before do
-            stub_gitlab_shell_setting(authorized_keys_file: nil)
-            gitlab_shell.remove_all_keys
-            100.times { |i| create(:key) } # first batch is all in the DB
-            gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
-          end
-
-          it 'removes the keys not in the DB' do
-            expect(gitlab_shell).to receive(:remove_key).with('key-1234')
-
-            gitlab_shell.remove_keys_not_found_in_db
-          end
-        end
-
-        context 'authorized_keys_file set' do
         before do
           gitlab_shell.remove_all_keys
           100.times { |i| create(:key) } # first batch is all in the DB
@@ -588,7 +270,6 @@ describe Gitlab::Shell do
       end
     end
   end
-  end
 
   describe 'projects commands' do
     let(:gitlab_shell_path) { File.expand_path('tmp/tests/gitlab-shell') }