Defense in depth for authorized_keys lines

lib/gitlab_keys.rb

@@ -4,13 +4,18 @@ require_relative 'gitlab_config'
 require_relative 'gitlab_logger'
 
 class GitlabKeys
+  class KeyError < StandardError ; end
+
   attr_accessor :auth_file, :key
 
   def self.command(key_id)
+    raise KeyError.new("Invalid key_id: #{key_id.inspect}") unless /\A[a-z0-9-]+\z/.match(key_id)
     "#{ROOT_PATH}/bin/gitlab-shell #{key_id}"
   end
 
   def self.key_line(key_id, public_key)
+    public_key.chomp!
+    raise KeyError.new("Invalid public_key: #{public_key.inspect}") if public_key.include?("\n")
     "command=\"#{command(key_id)}\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty #{public_key}"
   end
 

spec/gitlab_keys_spec.rb

@@ -7,6 +7,32 @@ describe GitlabKeys do
     $logger = double('logger').as_null_object
   end
 
+  describe '.command' do
+    it 'returns the "command" part of the key line' do
+      command = "#{ROOT_PATH}/bin/gitlab-shell key-123"
+      expect(described_class.command('key-123')).to eq(command)
+    end
+
+    it 'raises KeyError on invalid input' do
+      expect { described_class.command("\nssh-rsa AAA") }.to raise_error(GitlabKeys::KeyError)
+    end
+  end
+
+  describe '.key_line' do
+    let(:line) { %(command="#{ROOT_PATH}/bin/gitlab-shell key-741",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa AAAAB3NzaDAxx2E) }
+    it 'returns the key line' do
+      expect(described_class.key_line('key-741', 'ssh-rsa AAAAB3NzaDAxx2E')).to eq(line)
+    end
+
+    it 'silently removes a trailing newline' do
+      expect(described_class.key_line('key-741', "ssh-rsa AAAAB3NzaDAxx2E\n")).to eq(line)
+    end
+
+    it 'raises KeyError on invalid input' do
+      expect { described_class.key_line('key-741', "ssh-rsa AAA\nssh-rsa AAA") }.to raise_error(GitlabKeys::KeyError)
+    end
+  end
+
   describe :initialize do
     let(:gitlab_keys) { build_gitlab_keys('add-key', 'key-741', 'ssh-rsa AAAAB3NzaDAxx2E') }