Merge branch 'strip-key-comments-for-gitlab-shell' into 'master'

Strip comments before sending keys to gitlab-shell

## Why was this MR needed?

https://gitlab.com/gitlab-org/gitlab-ce/issues/22167 encoding issues in comment text.

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/22167

See merge request !6381

CHANGELOG

@@ -58,6 +58,7 @@ v 8.12.0 (unreleased)
   - Add hover color to emoji icon (ClemMakesApps)
   - Increase ci_builds artifacts_size column to 8-byte integer to allow larger files
   - Add textarea autoresize after comment (ClemMakesApps)
+  - Do not write SSH public key 'comments' to authorized_keys !6381
   - Refresh todos count cache when an Issue/MR is deleted
   - Fix branches page dropdown sort alignment (ClemMakesApps)
   - Hides merge request button on branches page is user doesn't have permissions

lib/gitlab/backend/shell.rb

@@ -6,7 +6,12 @@ module Gitlab
 
     KeyAdder = Struct.new(:io) do
       def add_key(id, key)
-        key.gsub!(/[[:space:]]+/, ' ').strip!
+        key = Gitlab::Shell.strip_key(key)
+        # Newline and tab are part of the 'protocol' used to transmit id+key to the other end
+        if key.include?("\t") || key.include?("\n")
+          raise Error.new("Invalid key: #{key.inspect}")
+        end
+
         io.puts("#{id}\t#{key}")
       end
     end
@@ -16,6 +21,10 @@ module Gitlab
         @version_required ||= File.read(Rails.root.
                                         join('GITLAB_SHELL_VERSION')).strip
       end
+
+      def strip_key(key)
+        key.split(/ /)[0, 2].join(' ')
+      end
     end
 
     # Init new repository
@@ -107,7 +116,7 @@ module Gitlab
     #
     def add_key(key_id, key_content)
       Gitlab::Utils.system_silent([gitlab_shell_keys_path,
-                                   'add-key', key_id, key_content])
+                                   'add-key', key_id, self.class.strip_key(key_content)])
     end
 
     # Batch-add keys to authorized_keys

spec/lib/gitlab/backend/shell_spec.rb

 require 'spec_helper'
+require 'stringio'
 
 describe Gitlab::Shell, lib: true do
   let(:project) { double('Project', id: 7, path: 'diaspora') }
@@ -44,15 +45,38 @@ describe Gitlab::Shell, lib: true do
     end
   end
 
+  describe '#add_key' do
+    it 'removes trailing garbage' do
+      allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path)
+      expect(Gitlab::Utils).to receive(:system_silent).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
+
   describe Gitlab::Shell::KeyAdder, lib: true do
     describe '#add_key' do
-      it 'normalizes space characters in the key' do
-        io = spy
+      it 'removes trailing garbage' do
+        io = spy(:io)
         adder = described_class.new(io)
 
-        adder.add_key('key-42', "sha-rsa foo\tbar\tbaz")
+        adder.add_key('key-42', "ssh-rsa foo bar\tbaz")
+
+        expect(io).to have_received(:puts).with("key-42\tssh-rsa foo")
+      end
+
+      it 'raises an exception if the key contains a tab' do
+        expect do
+          described_class.new(StringIO.new).add_key('key-42', "ssh-rsa\tfoobar")
+        end.to raise_error(Gitlab::Shell::Error)
+      end
 
-        expect(io).to have_received(:puts).with("key-42\tsha-rsa foo bar baz")
+      it 'raises an exception if the key contains a newline' do
+        expect do
+          described_class.new(StringIO.new).add_key('key-42', "ssh-rsa foobar\nssh-rsa pawned")
+        end.to raise_error(Gitlab::Shell::Error)
       end
     end
   end