Merge branch 'user-argument-2/upstream' into 'master'

Add support for ssh certificates

Closes gitlab-ce#34572

See merge request gitlab-org/gitlab-shell!207

bin/gitlab-shell

@@ -5,19 +5,19 @@ unless ENV['SSH_CONNECTION']
   exit
 end
 
-key_id = /key-[0-9]+/.match(ARGV.join).to_s
 original_cmd = ENV.delete('SSH_ORIGINAL_COMMAND')
 
 require_relative '../lib/gitlab_init'
 
 #
 #
-# GitLab shell, invoked from ~/.ssh/authorized_keys
+# GitLab shell, invoked from ~/.ssh/authorized_keys or from an
+# AuthorizedPrincipalsCommand in the key-less SSH CERT mode.
 #
 #
 require File.join(ROOT_PATH, 'lib', 'gitlab_shell')
 
-if GitlabShell.new(key_id).exec(original_cmd)
+if GitlabShell.new(ARGV.join).exec(original_cmd)
   exit 0
 else
   exit 1

bin/gitlab-shell-authorized-principals-check

+#!/usr/bin/env ruby
+
+#
+# GitLab shell authorized principals helper. Emits the same sort of
+# command="..." line as gitlab-shell-authorized-principals-check, with
+# the right options.
+#
+# Ex.
+#   bin/gitlab-shell-authorized-keys-check <key-id> <principal1> [<principal2>...]
+#
+# Returns one line per principal passed in, e.g.:
+#   command="/bin/gitlab-shell username-{KEY_ID}",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty {PRINCIPAL}
+#   [command="/bin/gitlab-shell username-{KEY_ID}",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty {PRINCIPAL2}]
+#
+# Expects to be called by the SSH daemon, via configuration like:
+#     AuthorizedPrincipalsCommandUser root
+#     AuthorizedPrincipalsCommand /bin/gitlab-shell-authorized-principals-check git %i sshUsers
+
+abort "# Wrong number of arguments. #{ARGV.size}. Usage:
+#     gitlab-shell-authorized-principals-check <key-id> <principal1> [<principal2>...]" unless ARGV.size >= 2
+
+key_id = ARGV[0]
+abort '# No key_id provided' if key_id.nil? || key_id == ''
+
+principals = ARGV[1..-1]
+principals.each { |principal|
+  abort '# An invalid principal was provided' if principal.nil? || principal == ''  
+}
+
+require_relative '../lib/gitlab_init'
+require_relative '../lib/gitlab_net'
+require_relative '../lib/gitlab_keys'
+
+principals.each { |principal|
+  puts GitlabKeys.principal_line("username-#{key_id}", principal.dup)
+}

lib/gitlab_access_status.rb

 require 'json'
 
 class GitAccessStatus
-  attr_reader :message, :gl_repository, :gl_username, :repository_path, :gitaly
+  attr_reader :message, :gl_repository, :gl_id, :gl_username, :repository_path, :gitaly
 
-  def initialize(status, message, gl_repository:, gl_username:, repository_path:, gitaly:)
+  def initialize(status, message, gl_repository:, gl_id:, gl_username:, repository_path:, gitaly:)
     @status = status
     @message = message
     @gl_repository = gl_repository
+    @gl_id = gl_id
     @gl_username = gl_username
     @repository_path = repository_path
     @gitaly = gitaly
@@ -17,6 +18,7 @@ class GitAccessStatus
     new(values["status"],
         values["message"],
         gl_repository: values["gl_repository"],
+        gl_id: values["gl_id"],
         gl_username: values["gl_username"],
         repository_path: values["repository_path"],
         gitaly: values["gitaly"])

lib/gitlab_keys.rb

@@ -9,12 +9,20 @@ class GitlabKeys # rubocop:disable Metrics/ClassLength
 
   attr_accessor :auth_file, :key
 
-  def self.command(key_id)
+  def self.command(whatever)
+    "#{ROOT_PATH}/bin/gitlab-shell #{whatever}"
+  end
+
+  def self.command_key(key_id)
     unless /\A[a-z0-9-]+\z/ =~ key_id
       raise KeyError, "Invalid key_id: #{key_id.inspect}"
     end
 
-    "#{ROOT_PATH}/bin/gitlab-shell #{key_id}"
+    command(key_id)
+  end
+
+  def self.whatever_line(command, trailer)
+    "command=\"#{command}\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty #{trailer}"
   end
 
   def self.key_line(key_id, public_key)
@@ -24,7 +32,17 @@ class GitlabKeys # rubocop:disable Metrics/ClassLength
       raise KeyError, "Invalid public_key: #{public_key.inspect}"
     end
 
-    "command=\"#{command(key_id)}\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty #{public_key}"
+    whatever_line(command_key(key_id), public_key)
+  end
+
+  def self.principal_line(username_key_id, principal)
+    principal.chomp!
+
+    if principal.include?("\n")
+      raise KeyError, "Invalid principal: #{principal.inspect}"
+    end
+
+    whatever_line(command_key(username_key_id), principal)
   end
 
   def initialize
@@ -119,7 +137,7 @@ class GitlabKeys # rubocop:disable Metrics/ClassLength
       $logger.info('Removing key', key_id: @key_id)
       open_auth_file('r+') do |f|
         while line = f.gets # rubocop:disable Style/AssignmentInCondition
-          next unless line.start_with?("command=\"#{self.class.command(@key_id)}\"")
+          next unless line.start_with?("command=\"#{self.class.command_key(@key_id)}\"")
           f.seek(-line.length, IO::SEEK_CUR)
           # Overwrite the line with #'s. Because the 'line' variable contains
           # a terminating '\n', we write line.length - 1 '#' characters.

lib/gitlab_net.rb

@@ -17,7 +17,7 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
 
   CHECK_TIMEOUT = 5
 
-  def check_access(cmd, gl_repository, repo, actor, changes, protocol, env: {})
+  def check_access(cmd, gl_repository, repo, who, changes, protocol, env: {})
     changes = changes.join("\n") unless changes.is_a?(String)
 
     params = {
@@ -29,11 +29,8 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
       env: env
     }
 
-    if actor =~ /\Akey\-\d+\Z/
-      params[:key_id] = actor.gsub("key-", "")
-    elsif actor =~ /\Auser\-\d+\Z/
-      params[:user_id] = actor.gsub("user-", "")
-    end
+    who_sym, _, who_v = self.class.parse_who(who)
+    params[who_sym] = who_v
 
     url = "#{internal_api_endpoint}/allowed"
     resp = post(url, params)
@@ -44,23 +41,37 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
       GitAccessStatus.new(false,
                           'API is not accessible',
                           gl_repository: nil,
+                          gl_id: nil,
                           gl_username: nil,
                           repository_path: nil,
                           gitaly: nil)
     end
   end
 
-  def discover(key)
-    key_id = key.gsub("key-", "")
-    resp = get("#{internal_api_endpoint}/discover?key_id=#{key_id}")
+  def discover(who)
+    _, who_k, who_v = self.class.parse_who(who)
+
+    resp = get("#{internal_api_endpoint}/discover?#{who_k}=#{who_v}")
+
     JSON.parse(resp.body) rescue nil
   end
 
-  def lfs_authenticate(key, repo)
-    params = {
-      project: sanitize_path(repo),
-      key_id: key.gsub('key-', '')
-    }
+  def lfs_authenticate(gl_id, repo)
+    id_sym, _, id = self.class.parse_who(gl_id)
+
+    if id_sym == :key_id
+      params = {
+        project: sanitize_path(repo),
+        key_id: id
+      }
+    elsif id_sym == :user_id
+      params = {
+        project: sanitize_path(repo),
+        user_id: id
+      }
+    else
+      raise ArgumentError, "lfs_authenticate() got unsupported GL_ID='#{gl_id}'!"
+    end
 
     resp = post("#{internal_api_endpoint}/lfs_authenticate", params)
 
@@ -101,9 +112,10 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
     nil
   end
 
-  def two_factor_recovery_codes(key)
-    key_id = key.gsub('key-', '')
-    resp = post("#{internal_api_endpoint}/two_factor_recovery_codes", key_id: key_id)
+  def two_factor_recovery_codes(gl_id)
+    id_sym, _, id = self.class.parse_who(gl_id)
+
+    resp = post("#{internal_api_endpoint}/two_factor_recovery_codes", id_sym => id)
 
     JSON.parse(resp.body) if resp.code == '200'
   rescue
@@ -140,6 +152,22 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
     JSON.parse(resp.body) if resp.code == '200'
   end
 
+  def self.parse_who(who)
+    if who.start_with?("key-")
+      value = who.gsub("key-", "")
+      raise ArgumentError, "who='#{who}' is invalid!" unless value =~ /\A[0-9]+\z/
+      [:key_id, 'key_id', value]
+    elsif who.start_with?("user-")
+      value = who.gsub("user-", "")
+      raise ArgumentError, "who='#{who}' is invalid!" unless value =~ /\A[0-9]+\z/
+      [:user_id, 'user_id', value]
+    elsif who.start_with?("username-")
+      [:username, 'username', who.gsub("username-", "")]
+    else
+      raise ArgumentError, "who='#{who}' is invalid!"
+    end
+  end
+
   protected
 
   def sanitize_path(repo)

lib/gitlab_shell.rb

@@ -18,11 +18,16 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
   API_COMMANDS = %w(2fa_recovery_codes).freeze
   GL_PROTOCOL = 'ssh'.freeze
 
-  attr_accessor :key_id, :gl_repository, :repo_name, :command, :git_access
+  attr_accessor :gl_id, :gl_repository, :repo_name, :command, :git_access
   attr_reader :repo_path
 
-  def initialize(key_id)
-    @key_id = key_id
+  def initialize(who)
+    who_sym, = GitlabNet.parse_who(who)
+    if who_sym == :username
+      @who = who
+    else
+      @gl_id = who
+    end
     @config = GitlabConfig.new
   end
 
@@ -40,6 +45,12 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
 
     if GIT_COMMANDS.include?(args.first)
       GitlabMetrics.measure('verify-access') { verify_access }
+    elsif !defined?(@gl_id)
+      # We're processing an API command like 2fa_recovery_codes, but
+      # don't have a @gl_id yet, that means we're in the "username"
+      # mode and need to materialize it, calling the "user" method
+      # will do that and call the /discover method.
+      user
     end
 
     process_cmd(args)
@@ -101,7 +112,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
   end
 
   def verify_access
-    status = api.check_access(@git_access, nil, @repo_name, @key_id, '_any', GL_PROTOCOL)
+    status = api.check_access(@git_access, nil, @repo_name, @who || @gl_id, '_any', GL_PROTOCOL)
 
     raise AccessDeniedError, status.message unless status.allowed?
 
@@ -109,6 +120,9 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     @gl_repository = status.gl_repository
     @gitaly = status.gitaly
     @username = status.gl_username
+    if defined?(@who)
+      @gl_id = status.gl_id
+    end
   end
 
   def process_cmd(args)
@@ -135,7 +149,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
       gitaly_request = {
         'repository' => @gitaly['repository'],
         'gl_repository' => @gl_repository,
-        'gl_id' => @key_id,
+        'gl_id' => @gl_id,
         'gl_username' => @username
       }
 
@@ -161,7 +175,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
       'PATH' => ENV['PATH'],
       'LD_LIBRARY_PATH' => ENV['LD_LIBRARY_PATH'],
       'LANG' => ENV['LANG'],
-      'GL_ID' => @key_id,
+      'GL_ID' => @gl_id,
       'GL_PROTOCOL' => GL_PROTOCOL,
       'GL_REPOSITORY' => @gl_repository,
       'GL_USERNAME' => @username
@@ -190,7 +204,12 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     return @user if defined?(@user)
 
     begin
-      @user = api.discover(@key_id)
+      if defined?(@who)
+        @user = api.discover(@who)
+        @gl_id = "user-#{@user['id']}"
+      else
+        @user = api.discover(@gl_id)
+      end
     rescue GitlabNet::ApiUnreachableError
       @user = nil
     end
@@ -208,11 +227,11 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
 
   # User identifier to be used in log messages.
   def log_username
-    @config.audit_usernames ? username : "user with key #{@key_id}"
+    @config.audit_usernames ? username : "user with id #{@gl_id}"
   end
 
   def lfs_authenticate
-    lfs_access = api.lfs_authenticate(@key_id, @repo_name)
+    lfs_access = api.lfs_authenticate(@gl_id, @repo_name)
 
     return unless lfs_access
 
@@ -240,7 +259,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
       return
     end
 
-    resp = api.two_factor_recovery_codes(key_id)
+    resp = api.two_factor_recovery_codes(@gl_id)
     if resp['success']
       codes = resp['recovery_codes'].join("\n")
       puts "Your two-factor authentication recovery codes are:\n\n" \

spec/gitlab_access_spec.rb

@@ -10,6 +10,7 @@ describe GitlabAccess do
       api.stub(check_access: GitAccessStatus.new(true,
                                                  'ok',
                                                  gl_repository: 'project-1',
+                                                 gl_id: 'user-123',
                                                  gl_username: 'testuser',
                                                  repository_path: '/home/git/repositories',
                                                  gitaly: nil))
@@ -47,6 +48,7 @@ describe GitlabAccess do
                   false,
                   'denied',
                   gl_repository: nil,
+                  gl_id: nil,
                   gl_username: nil,
                   repository_path: nil,
                   gitaly: nil

spec/gitlab_keys_spec.rb

@@ -8,13 +8,25 @@ describe GitlabKeys do
   end
 
   describe '.command' do
+    it 'the internal "command" utility function' do
+      command = "#{ROOT_PATH}/bin/gitlab-shell does-not-validate"
+      expect(described_class.command('does-not-validate')).to eq(command)
+    end
+
+    it 'does not raise a KeyError on invalid input' do
+      command = "#{ROOT_PATH}/bin/gitlab-shell foo\nbar\nbaz\n"
+      expect(described_class.command("foo\nbar\nbaz\n")).to eq(command)
+    end
+  end
+
+  describe '.command_key' 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)
+      expect(described_class.command_key('key-123')).to eq(command)
     end
 
     it 'raises KeyError on invalid input' do
-      expect { described_class.command("\nssh-rsa AAA") }.to raise_error(described_class::KeyError)
+      expect { described_class.command_key("\nssh-rsa AAA") }.to raise_error(described_class::KeyError)
     end
   end
 
@@ -34,6 +46,22 @@ describe GitlabKeys do
     end
   end
 
+  describe '.principal_line' do
+    let(:line) { %(command="#{ROOT_PATH}/bin/gitlab-shell username-someuser",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty sshUsers) }
+
+    it 'returns the key line' do
+      expect(described_class.principal_line('username-someuser', 'sshUsers')).to eq(line)
+    end
+
+    it 'silently removes a trailing newline' do
+      expect(described_class.principal_line('username-someuser', "sshUsers\n")).to eq(line)
+    end
+
+    it 'raises KeyError on invalid input' do
+      expect { described_class.principal_line('username-someuser', "sshUsers\nloginUsers") }.to raise_error(described_class::KeyError)
+    end
+  end
+
   describe :initialize do
     let(:gitlab_keys) { build_gitlab_keys('add-key', 'key-741', 'ssh-rsa AAAAB3NzaDAxx2E') }
 

spec/gitlab_shell_spec.rb

@@ -13,8 +13,8 @@ describe GitlabShell do
   end
 
   subject do
-    ARGV[0] = key_id
-    GitlabShell.new(key_id).tap do |shell|
+    ARGV[0] = gl_id
+    GitlabShell.new(gl_id).tap do |shell|
       shell.stub(exec_cmd: :exec_called)
       shell.stub(api: api)
     end
@@ -24,6 +24,7 @@ describe GitlabShell do
     true,
     'ok',
     gl_repository: gl_repository,
+    gl_id: gl_id,
     gl_username: gl_username,
     repository_path: repo_path,
     gitaly: { 'repository' => { 'relative_path' => repo_name, 'storage_name' => 'default'} , 'address' => 'unix:gitaly.socket' }
@@ -37,6 +38,7 @@ describe GitlabShell do
                 true,
                 'ok',
                 gl_repository: gl_repository,
+                gl_id: gl_id,
                 gl_username: gl_username,
                 repository_path: repo_path,
                 gitaly: nil))
@@ -47,13 +49,14 @@ describe GitlabShell do
     end
   end
 
-  let(:key_id) { "key-#{rand(100) + 100}" }
+  let(:gl_id) { "key-#{rand(100) + 100}" }
   let(:ssh_cmd) { nil }
   let(:tmp_repos_path) { File.join(ROOT_PATH, 'tmp', 'repositories') }
 
   let(:repo_name) { 'gitlab-ci.git' }
   let(:repo_path) { File.join(tmp_repos_path, repo_name) }
   let(:gl_repository) { 'project-1' }
+  let(:gl_id) { 'user-1' }
   let(:gl_username) { 'testuser' }
 
   before do
@@ -63,7 +66,7 @@ describe GitlabShell do
   describe :initialize do
     let(:ssh_cmd) { 'git-receive-pack' }
 
-    its(:key_id) { should == key_id }
+    its(:gl_id) { should == gl_id }
   end
 
   describe :parse_cmd do
@@ -146,7 +149,7 @@ describe GitlabShell do
   end
 
   describe :exec do
-    let(:gitaly_message) { JSON.dump({ 'repository' => { 'relative_path' => repo_name, 'storage_name' => 'default' }, 'gl_repository' => gl_repository, 'gl_id' => key_id, 'gl_username' => gl_username}) }
+    let(:gitaly_message) { JSON.dump({ 'repository' => { 'relative_path' => repo_name, 'storage_name' => 'default' }, 'gl_repository' => gl_repository, 'gl_id' => gl_id, 'gl_username' => gl_username}) }
 
     shared_examples_for 'upload-pack' do |command|
       let(:ssh_cmd) { "#{command} gitlab-ci.git" }
@@ -162,7 +165,7 @@ describe GitlabShell do
 
       it "should log the command execution" do
         message = "executing git command"
-        user_string = "user with key #{key_id}"
+        user_string = "user with id #{gl_id}"
         $logger.should_receive(:info).with(message, command: "git-upload-pack #{repo_path}", user: user_string)
       end
 
@@ -197,7 +200,7 @@ describe GitlabShell do
 
       it "should log the command execution" do
         message = "executing git command"
-        user_string = "user with key #{key_id}"
+        user_string = "user with id #{gl_id}"
         $logger.should_receive(:info).with(message, command: "gitaly-upload-pack unix:gitaly.socket #{gitaly_message}", user: user_string)
       end
 
@@ -221,7 +224,7 @@ describe GitlabShell do
 
       it "should log the command execution" do
         message = "executing git command"
-        user_string = "user with key #{key_id}"
+        user_string = "user with id #{gl_id}"
         $logger.should_receive(:info).with(message, command: "git-receive-pack #{repo_path}", user: user_string)
       end
     end
@@ -243,7 +246,7 @@ describe GitlabShell do
 
       it "should log the command execution" do
         message = "executing git command"
-        user_string = "user with key #{key_id}"
+        user_string = "user with id #{gl_id}"
         $logger.should_receive(:info).with(message, command: "gitaly-receive-pack unix:gitaly.socket #{gitaly_message}", user: user_string)
       end
 
@@ -270,7 +273,7 @@ describe GitlabShell do
 
       it "should log the command execution" do
         message = "executing git command"
-        user_string = "user with key #{key_id}"
+        user_string = "user with id #{gl_id}"
         $logger.should_receive(:info).with(message, command: exec_cmd_log_params.join(' '), user: user_string)
       end
 
@@ -322,7 +325,7 @@ describe GitlabShell do
 
       it "should log the attempt" do
         message = 'Denied disallowed command'
-        user_string = "user with key #{key_id}"
+        user_string = "user with id #{gl_id}"
         $logger.should_receive(:warn).with(message, command: 'arbitrary command', user: user_string)
       end
     end
@@ -331,7 +334,7 @@ describe GitlabShell do
       after { subject.exec(nil) }
 
       it "should call api.discover" do
-        api.should_receive(:discover).with(key_id)
+        api.should_receive(:discover).with(gl_id)
       end
     end
 
@@ -398,7 +401,7 @@ describe GitlabShell do
       after { subject.exec(ssh_cmd) }
 
       it "should call api.check_access" do
-        api.should_receive(:check_access).with('git-upload-pack', nil, 'gitlab-ci.git', key_id, '_any', 'ssh')
+        api.should_receive(:check_access).with('git-upload-pack', nil, 'gitlab-ci.git', gl_id, '_any', 'ssh')
       end
 
       it "should disallow access and log the attempt if check_access returns false status" do
@@ -406,11 +409,12 @@ describe GitlabShell do
                   false,
                   'denied',
                   gl_repository: nil,
+                  gl_id: nil,
                   gl_username: nil,
                   repository_path: nil,
                   gitaly: nil))
         message = 'Access denied'
-        user_string = "user with key #{key_id}"
+        user_string = "user with id #{gl_id}"
         $logger.should_receive(:warn).with(message, command: 'git-upload-pack gitlab-ci.git', user: user_string)
       end
     end
@@ -436,14 +440,14 @@ describe GitlabShell do
   end
 
   describe :exec_cmd do
-    let(:shell) { GitlabShell.new(key_id) }
+    let(:shell) { GitlabShell.new(gl_id) }
     let(:env) do
       {
         'HOME' => ENV['HOME'],
         'PATH' => ENV['PATH'],
         'LD_LIBRARY_PATH' => ENV['LD_LIBRARY_PATH'],
         'LANG' => ENV['LANG'],
-        'GL_ID' => key_id,
+        'GL_ID' => gl_id,
         'GL_PROTOCOL' => 'ssh',
         'GL_REPOSITORY' => gl_repository,
         'GL_USERNAME' => 'testuser'
@@ -543,7 +547,7 @@ describe GitlabShell do
   end
 
   describe :api do
-    let(:shell) { GitlabShell.new(key_id) }
+    let(:shell) { GitlabShell.new(gl_id) }
     subject { shell.send :api }
 
     it { should be_a(GitlabNet) }