Merge remote-tracking branch 'origin/master' into ash.mckenzie/srp-refactor

CHANGELOG

+v8.0.0
+  - SSH certificate support (!207)
+
 v7.2.0
   - Update gitaly-proto to 0.109.0 (!216)
 

VERSION

-7.2.0
+8.0.0

bin/gitlab-shell

@@ -5,19 +5,19 @@ unless ENV['SSH_CONNECTION']
   exit
 end
 
-key_str = /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_str).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/action/api_2fa_recovery.rb

@@ -3,8 +3,8 @@ require_relative '../gitlab_logger'
 
 module Action
   class API2FARecovery < Base
-    def initialize(key)
-      @key = key
+    def initialize(actor)
+      @actor = actor
     end
 
     def execute(_, _)
@@ -13,7 +13,7 @@ module Action
 
     private
 
-    attr_reader :key
+    attr_reader :actor
 
     def continue?(question)
       puts "#{question} (yes/no)"
@@ -34,10 +34,10 @@ module Action
         return
       end
 
-      resp = api.two_factor_recovery_codes(key.key_id)
+      resp = api.two_factor_recovery_codes(self)
       if resp['success']
         codes = resp['recovery_codes'].join("\n")
-        $logger.info('API 2FA recovery success', user: key.log_username)
+        $logger.info('API 2FA recovery success', user: actor.log_username)
         puts "Your two-factor authentication recovery codes are:\n\n" \
             "#{codes}\n\n" \
             "During sign in, use one of the codes above when prompted for\n" \
@@ -45,7 +45,7 @@ module Action
             "a new device so you do not lose access to your account again."
         true
       else
-        $logger.info('API 2FA recovery error', user: key.log_username)
+        $logger.info('API 2FA recovery error', user: actor.log_username)
         puts "An error occurred while trying to generate new recovery codes.\n" \
             "#{resp['message']}"
       end

lib/action/git_lfs_authenticate.rb

@@ -3,15 +3,15 @@ require_relative '../gitlab_logger'
 
 module Action
   class GitLFSAuthenticate < Base
-    def initialize(key, repo_name)
-      @key = key
+    def initialize(actor, repo_name)
+      @actor = actor
       @repo_name = repo_name
     end
 
     def execute(_, _)
       GitlabMetrics.measure('lfs-authenticate') do
-        $logger.info('Processing LFS authentication', user: key.log_username)
-        lfs_access = api.lfs_authenticate(key.key_id, repo_name)
+        $logger.info('Processing LFS authentication', user: actor.log_username)
+        lfs_access = api.lfs_authenticate(self, repo_name)
         return unless lfs_access
 
         puts lfs_access.authentication_payload
@@ -21,6 +21,6 @@ module Action
 
     private
 
-    attr_reader :key, :repo_name
+    attr_reader :actor, :repo_name
   end
 end

lib/actor.rb

 require_relative 'actor/base'
 require_relative 'actor/key'
 require_relative 'actor/user'
+require_relative 'actor/username'
 
 module Actor
   class UnsupportedActorError < StandardError; end
@@ -11,6 +12,8 @@ module Actor
       Key.from(str, audit_usernames: audit_usernames)
     when User.id_regex
       User.from(str, audit_usernames: audit_usernames)
+    when Username.id_regex
+      Username.from(str, audit_usernames: audit_usernames)
     else
       raise UnsupportedActorError
     end

lib/actor/base.rb

@@ -31,6 +31,10 @@ module Actor
       "#{self.class.identifier_prefix}-#{id}"
     end
 
+    def identifier_key
+      self.class.identifier_key
+    end
+
     def log_username
       audit_usernames? ? username : "#{klass_name.downcase} with identifier #{identifier}"
     end

lib/actor/key.rb

@@ -21,7 +21,7 @@ module Actor
 
     def username
       @username ||= begin
-        user = GitlabNet.new.discover(key_id)
+        user = GitlabNet.new.discover(self)
         user ? "@#{user['username']}" : ANONYMOUS_USER
       end
     end

lib/actor/username.rb

+require_relative 'base'
+
+module Actor
+  class Username < Base
+    alias username identifier
+
+    def self.identifier_prefix
+      'username'.freeze
+    end
+
+    def self.identifier_key
+      'username'.freeze
+    end
+
+    def self.id_regex
+      /\Ausername\-\d+\Z/
+    end
+  end
+end

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

@@ -26,22 +26,24 @@ class GitlabNet
       env: env
     }
 
-    params[actor.class.identifier_key.to_sym] = actor.id
+    params[actor.identifier_key.to_sym] = actor.id
 
     resp = post("#{internal_api_endpoint}/allowed", params)
 
     determine_action(actor, resp)
   end
 
-  def discover(key_id)
-    resp = get("#{internal_api_endpoint}/discover?key_id=#{key_id}")
+  def discover(actor)
+    resp = get("#{internal_api_endpoint}/discover?#{actor.identifier_key}=#{actor.id}")
     JSON.parse(resp.body)
   rescue JSON::ParserError, ApiUnreachableError
     nil
   end
 
-  def lfs_authenticate(key_id, repo)
-    params = { project: sanitize_path(repo), key_id: key_id }
+  def lfs_authenticate(actor, repo)
+    params = { project: sanitize_path(repo) }
+    params[actor.identifier_key.to_sym] = actor.id
+
     resp = post("#{internal_api_endpoint}/lfs_authenticate", params)
 
     GitlabLfsAuthentication.build_from_json(resp.body) if resp.code == HTTP_SUCCESS
@@ -75,8 +77,9 @@ class GitlabNet
     nil
   end
 
-  def two_factor_recovery_codes(key_id)
-    resp = post("#{internal_api_endpoint}/two_factor_recovery_codes", key_id: key_id)
+  def two_factor_recovery_codes(actor)
+    params = { actor.identifier_key.to_sym => actor.id }
+    resp = post("#{internal_api_endpoint}/two_factor_recovery_codes", params)
     JSON.parse(resp.body) if resp.code == HTTP_SUCCESS
   rescue
     {}

lib/gitlab_shell.rb

@@ -3,7 +3,7 @@ require 'pathname'
 
 require_relative 'gitlab_net'
 require_relative 'gitlab_metrics'
-require_relative 'actor/key'
+require_relative 'actor'
 
 class GitlabShell
   API_2FA_RECOVERY_CODES_COMMAND = '2fa_recovery_codes'.freeze
@@ -18,9 +18,9 @@ class GitlabShell
 
   Struct.new('ParsedCommand', :command, :git_access_command, :repo_name, :args)
 
-  def initialize(key_str)
-    @key_str = key_str
+  def initialize(who)
     @config = GitlabConfig.new
+    @actor = Actor.new_from(who, audit_usernames: @config.audit_usernames)
   end
 
   # The origin_cmd variable contains UNTRUSTED input. If the user ran
@@ -28,22 +28,22 @@ class GitlabShell
   # 'evil command'.
   def exec(origin_cmd)
     if !origin_cmd || origin_cmd.empty?
-      puts "Welcome to GitLab, #{key.username}!"
+      puts "Welcome to GitLab, #{actor.username}!"
       return true
     end
 
     parsed_command = parse_cmd(origin_cmd)
-    action = determine_action(parsed_command)
+    action = determine_action(parsed_command)  # FIXME: watch out
     action.execute(parsed_command.command, parsed_command.args)
   rescue GitlabNet::ApiUnreachableError
     $stderr.puts "GitLab: Failed to authorize your Git request: internal API unreachable"
     false
   rescue AccessDeniedError, UnknownError => ex
-    $logger.warn('Access denied', command: origin_cmd, user: key.log_username)
+    $logger.warn('Access denied', command: origin_cmd, user: actor.log_username)
     $stderr.puts "GitLab: #{ex.message}"
     false
   rescue DisallowedCommandError
-    $logger.warn('Denied disallowed command', command: origin_cmd, user: key.log_username)
+    $logger.warn('Denied disallowed command', command: origin_cmd, user: actor.log_username)
     $stderr.puts 'GitLab: Disallowed command'
     false
   rescue InvalidRepositoryPathError
@@ -53,11 +53,7 @@ class GitlabShell
 
   private
 
-  attr_reader :config, :key_str
-
-  def key
-    @key ||= Actor::Key.from(key_str, audit_usernames: config.audit_usernames)
-  end
+  attr_reader :config, :actor
 
   def parse_cmd(cmd)
     args = Shellwords.shellwords(cmd)
@@ -99,7 +95,7 @@ class GitlabShell
   end
 
   def determine_action(parsed_command)
-    return Action::API2FARecovery.new(key) if parsed_command.command == API_2FA_RECOVERY_CODES_COMMAND
+    return Action::API2FARecovery.new(actor) if parsed_command.command == API_2FA_RECOVERY_CODES_COMMAND
 
     GitlabMetrics.measure('verify-access') do
       # GitlatNet#check_access will raise exception in the event of a problem
@@ -107,13 +103,13 @@ class GitlabShell
         parsed_command.git_access_command,
         nil,
         parsed_command.repo_name,
-        key,
+        actor,
         '_any'
       )
 
       case parsed_command.command
       when GIT_LFS_AUTHENTICATE_COMMAND
-        Action::GitLFSAuthenticate.new(key, parsed_command.repo_name)
+        Action::GitLFSAuthenticate.new(actor, parsed_command.repo_name)
       else
         initial_action
       end

spec/action/api_2fa_recovery.rb_spec.rb

@@ -3,18 +3,18 @@ require_relative '../../lib/action/api_2fa_recovery'
 
 describe Action::API2FARecovery do
   let(:key_id) { '1' }
-  let(:key) { Actor::Key.new(key_id) }
+  let(:actor) { Actor::Key.new(key_id) }
   let(:username) { 'testuser' }
   let(:discover_payload) { { 'username' => username } }
   let(:api) { double(GitlabNet) }
 
   before do
     allow(GitlabNet).to receive(:new).and_return(api)
-    allow(api).to receive(:discover).with(key_id).and_return(discover_payload)
+    allow(api).to receive(:discover).with(actor).and_return(discover_payload)
   end
 
   subject do
-    described_class.new(key)
+    described_class.new(actor)
   end
 
   describe '#execute' do
@@ -46,7 +46,7 @@ describe Action::API2FARecovery do
 
       before do
         expect(subject).to receive(:continue?).and_return(true)
-        expect(api).to receive(:two_factor_recovery_codes).with(key_id).and_return(response)
+        expect(api).to receive(:two_factor_recovery_codes).with(subject).and_return(response)
       end
 
       context 'with a unsuccessful response' do

spec/action/git_lfs_authenticate_spec.rb

@@ -4,24 +4,24 @@ require_relative '../../lib/action/git_lfs_authenticate'
 describe Action::GitLFSAuthenticate do
   let(:key_id) { '1' }
   let(:repo_name) { 'gitlab-ci.git' }
-  let(:key) { Actor::Key.new(key_id) }
+  let(:actor) { Actor::Key.new(key_id) }
   let(:username) { 'testuser' }
   let(:discover_payload) { { 'username' => username } }
   let(:api) { double(GitlabNet) }
 
   before do
     allow(GitlabNet).to receive(:new).and_return(api)
-    allow(api).to receive(:discover).with(key_id).and_return(discover_payload)
+    allow(api).to receive(:discover).with(actor).and_return(discover_payload)
   end
 
   subject do
-    described_class.new(key, repo_name)
+    described_class.new(actor, repo_name)
   end
 
   describe '#execute' do
     context 'when response from API is not a success' do
       before do
-        expect(api).to receive(:lfs_authenticate).with(key_id, repo_name).and_return(nil)
+        expect(api).to receive(:lfs_authenticate).with(subject, repo_name).and_return(nil)
       end
 
       it 'returns nil' do
@@ -36,7 +36,7 @@ describe Action::GitLFSAuthenticate do
       let(:gitlab_lfs_authentication) { GitlabLfsAuthentication.new(username, lfs_token, repository_http_path) }
 
       before do
-        expect(api).to receive(:lfs_authenticate).with(key_id, repo_name).and_return(gitlab_lfs_authentication)
+        expect(api).to receive(:lfs_authenticate).with(subject, repo_name).and_return(gitlab_lfs_authentication)
       end
 
       it 'puts payload to stdout' do

spec/actor/key_spec.rb

@@ -11,7 +11,7 @@ describe Actor::Key do
 
   before do
     allow(GitlabNet).to receive(:new).and_return(api)
-    allow(api).to receive(:discover).with(key_id).and_return(discover_payload)
+    allow(api).to receive(:discover).with(subject).and_return(discover_payload)
   end
 
   describe '.from' do

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 do described_class.command("\nssh-rsa AAA") end.to raise_error(described_class::KeyError)
+      expect { described_class.command_key("\nssh-rsa AAA") }.to raise_error(described_class::KeyError)
     end
   end
 
@@ -30,7 +42,23 @@ describe GitlabKeys do
     end
 
     it 'raises KeyError on invalid input' do
-      expect do described_class.key_line('key-741', "ssh-rsa AAA\nssh-rsa AAA") end.to raise_error(described_class::KeyError)
+      expect { described_class.key_line('key-741', "ssh-rsa AAA\nssh-rsa AAA") }.to raise_error(described_class::KeyError)
+    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
 

spec/gitlab_shell_spec.rb

@@ -10,44 +10,44 @@ describe GitlabShell do
 
   after { FileUtils.rm_rf(tmp_repos_path) }
 
-  subject { described_class.new(key_id) }
+  subject { described_class.new(who) }
 
-  let(:key_id) { '1' }
-  let(:key) { Actor::Key.new(key_id) }
+  let(:who) { 'key-1' }
+  let(:audit_usernames) { true }
+  let(:actor) { Actor.new_from(who, audit_usernames: audit_usernames) }
   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_username) { 'testuser' }
-  let(:audit_usernames) { true }
 
   let(:api) { double(GitlabNet) }
   let(:config) { double(GitlabConfig) }
 
   let(:gitaly_action) { Action::Gitaly.new(
-                          key_id,
+                          actor,
                           gl_repository,
                           gl_username,
                           repo_path,
                           { 'repository' => { 'relative_path' => repo_name, 'storage_name' => 'default' } , 'address' => 'unix:gitaly.socket' })
                       }
-  let(:api_2fa_recovery_action) { Action::API2FARecovery.new(key_id) }
-  let(:git_lfs_authenticate_action) { Action::GitLFSAuthenticate.new(key_id, repo_name) }
+  let(:api_2fa_recovery_action) { Action::API2FARecovery.new(actor) }
+  let(:git_lfs_authenticate_action) { Action::GitLFSAuthenticate.new(actor, repo_name) }
 
   before do
     allow(GitlabConfig).to receive(:new).and_return(config)
     allow(config).to receive(:audit_usernames).and_return(audit_usernames)
 
-    allow(Actor::Key).to receive(:from).with(key_id, audit_usernames: audit_usernames).and_return(key)
+    allow(Actor).to receive(:new_from).with(who, audit_usernames: audit_usernames).and_return(actor)
 
     allow(GitlabNet).to receive(:new).and_return(api)
-    allow(api).to receive(:discover).with(key_id).and_return('username' => gl_username)
+    allow(api).to receive(:discover).with(actor).and_return('username' => gl_username)
   end
 
   describe '#exec' do
     context "when we don't have a valid user" do
       before do
-        allow(api).to receive(:discover).with(key_id).and_return(nil)
+        allow(api).to receive(:discover).with(actor).and_return(nil)
       end
 
       it 'prints Welcome.. and returns true' do
@@ -114,7 +114,7 @@ describe GitlabShell do
       let(:git_access) { '2fa_recovery_codes' }
 
       before do
-        expect(Action::API2FARecovery).to receive(:new).with(key).and_return(api_2fa_recovery_action)
+        expect(Action::API2FARecovery).to receive(:new).with(actor).and_return(api_2fa_recovery_action)
       end
 
       it 'returns true' do
@@ -125,7 +125,7 @@ describe GitlabShell do
 
     context 'when access to the repo is denied' do
       before do
-        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, key, '_any').and_raise(AccessDeniedError, 'Sorry, access denied')
+        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, actor, '_any').and_raise(AccessDeniedError, 'Sorry, access denied')
       end
 
       it 'prints a message to stderr and returns false' do
@@ -136,7 +136,7 @@ describe GitlabShell do
 
     context 'when the API is unavailable' do
       before do
-        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, key, '_any').and_raise(GitlabNet::ApiUnreachableError)
+        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, actor, '_any').and_raise(GitlabNet::ApiUnreachableError)
       end
 
       it 'prints a message to stderr and returns false' do
@@ -147,7 +147,7 @@ describe GitlabShell do
 
     context 'when access has been verified OK' do
       before do
-        expect(api).to receive(:check_access).with(git_access, nil, repo_name, key, '_any').and_return(gitaly_action)
+        expect(api).to receive(:check_access).with(git_access, nil, repo_name, actor, '_any').and_return(gitaly_action)
       end
 
       context 'when origin_cmd is git-upload-pack' do
@@ -180,7 +180,7 @@ describe GitlabShell do
         let(:lfs_access) { double(GitlabLfsAuthentication, authentication_payload: fake_payload)}
 
         before do
-          expect(Action::GitLFSAuthenticate).to receive(:new).with(key, repo_name).and_return(git_lfs_authenticate_action)
+          expect(Action::GitLFSAuthenticate).to receive(:new).with(actor, repo_name).and_return(git_lfs_authenticate_action)
         end
 
         context 'upload' do