diff options
author | Nick Thomas <nick@gitlab.com> | 2018-08-24 08:58:07 +0000 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2018-08-24 08:58:07 +0000 |
commit | 306b6b5bf0e3dff50af8e37af4a28a36338a0dc6 (patch) | |
tree | 708c07c236619ab3153bb96556169b8d1a4409b8 | |
parent | abb55c839ceedeb8f32f9af4ad791d349ab03f8c (diff) | |
parent | 911e49e1894060404d4400efe06203e9e747a9d8 (diff) | |
download | gitlab-shell-306b6b5bf0e3dff50af8e37af4a28a36338a0dc6.tar.gz |
Merge branch 'zj-cleanup-exec' into 'master'
Cleanup `git-upload-*` and `git-receive-*` related code
Closes gitaly#1300
See merge request gitlab-org/gitlab-shell!232
-rw-r--r-- | config.yml.example | 7 | ||||
-rw-r--r-- | lib/gitlab_config.rb | 4 | ||||
-rw-r--r-- | lib/gitlab_shell.rb | 97 | ||||
-rw-r--r-- | spec/gitlab_shell_spec.rb | 167 |
4 files changed, 52 insertions, 223 deletions
diff --git a/config.yml.example b/config.yml.example index c616254..23743fd 100644 --- a/config.yml.example +++ b/config.yml.example @@ -49,10 +49,3 @@ log_level: INFO # Set to true to see real usernames in the logs instead of key ids, which is easier to follow, but # incurs an extra API call on every gitlab-shell command. audit_usernames: false - -# Git trace log file. -# If set, git commands receive GIT_TRACE* environment variables -# See https://git-scm.com/book/es/v2/Git-Internals-Environment-Variables#Debugging for documentation -# An absolute path starting with / – the trace output will be appended to that file. -# It needs to exist so we can check permissions and avoid to throwing warnings to the users. -git_trace_log_file: diff --git a/lib/gitlab_config.rb b/lib/gitlab_config.rb index 7645989..85aa889 100644 --- a/lib/gitlab_config.rb +++ b/lib/gitlab_config.rb @@ -50,10 +50,6 @@ class GitlabConfig @config['audit_usernames'] ||= false end - def git_trace_log_file - @config['git_trace_log_file'] - end - def metrics_log_file @config['metrics_log_file'] ||= File.join(ROOT_PATH, 'gitlab-shell-metrics.log') end diff --git a/lib/gitlab_shell.rb b/lib/gitlab_shell.rb index 4cabd37..4d0b26f 100644 --- a/lib/gitlab_shell.rb +++ b/lib/gitlab_shell.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'shellwords' require 'pathname' @@ -9,14 +11,15 @@ class GitlabShell # rubocop:disable Metrics/ClassLength class DisallowedCommandError < StandardError; end class InvalidRepositoryPathError < StandardError; end - GIT_COMMANDS = %w(git-upload-pack git-receive-pack git-upload-archive git-lfs-authenticate).freeze - GITALY_MIGRATED_COMMANDS = { + GITALY_COMMANDS = { 'git-upload-pack' => File.join(ROOT_PATH, 'bin', 'gitaly-upload-pack'), 'git-upload-archive' => File.join(ROOT_PATH, 'bin', 'gitaly-upload-archive'), 'git-receive-pack' => File.join(ROOT_PATH, 'bin', 'gitaly-receive-pack') }.freeze + + GIT_COMMANDS = (GITALY_COMMANDS.keys + ['git-lfs-authenticate']).freeze API_COMMANDS = %w(2fa_recovery_codes).freeze - GL_PROTOCOL = 'ssh'.freeze + GL_PROTOCOL = 'ssh' attr_accessor :gl_id, :gl_repository, :repo_name, :command, :git_access, :git_protocol @@ -136,64 +139,31 @@ class GitlabShell # rubocop:disable Metrics/ClassLength return end - executable = @command - args = [] - - if GITALY_MIGRATED_COMMANDS.key?(executable) && @gitaly - executable = GITALY_MIGRATED_COMMANDS[executable] - - gitaly_address = @gitaly['address'] - - # The entire gitaly_request hash should be built in gitlab-ce and passed - # on as-is. For now we build a fake one on the spot. - gitaly_request = { - 'repository' => @gitaly['repository'], - 'gl_repository' => @gl_repository, - 'gl_id' => @gl_id, - 'gl_username' => @username, - 'git_config_options' => @git_config_options, - 'git_protocol' => @git_protocol - } - - args = [gitaly_address, JSON.dump(gitaly_request)] - end + # TODO: instead of building from pieces here in gitlab-shell, build the + # entire gitaly_request in gitlab-ce and pass on as-is here. + args = JSON.dump( + 'repository' => @gitaly['repository'], + 'gl_repository' => @gl_repository, + 'gl_id' => @gl_id, + 'gl_username' => @username, + 'git_config_options' => @git_config_options, + 'git_protocol' => @git_protocol + ) - args_string = [File.basename(executable), *args].join(' ') + gitaly_address = @gitaly['address'] + executable = GITALY_COMMANDS.fetch(@command) + gitaly_bin = File.basename(executable) + args_string = [gitaly_bin, gitaly_address, args].join(' ') $logger.info('executing git command', command: args_string, user: log_username) - exec_cmd(executable, *args) + + exec_cmd(executable, gitaly_address: gitaly_address, token: @gitaly['token'], json_args: args) end # This method is not covered by Rspec because it ends the current Ruby process. - def exec_cmd(*args) - # If you want to call a command without arguments, use - # exec_cmd(['my_command', 'my_command']) . Otherwise use - # exec_cmd('my_command', 'my_argument', ...). - if args.count == 1 && !args.first.is_a?(Array) - raise DisallowedCommandError - end - - env = { - 'HOME' => ENV['HOME'], - 'PATH' => ENV['PATH'], - 'LD_LIBRARY_PATH' => ENV['LD_LIBRARY_PATH'], - 'LANG' => ENV['LANG'], - 'GL_ID' => @gl_id, - 'GL_PROTOCOL' => GL_PROTOCOL, - 'GL_REPOSITORY' => @gl_repository, - 'GL_USERNAME' => @username - } - if @gitaly && @gitaly.include?('token') - env['GITALY_TOKEN'] = @gitaly['token'] - end - - if git_trace_available? - env.merge!( - 'GIT_TRACE' => @config.git_trace_log_file, - 'GIT_TRACE_PACKET' => @config.git_trace_log_file, - 'GIT_TRACE_PERFORMANCE' => @config.git_trace_log_file - ) - end + def exec_cmd(executable, gitaly_address:, token:, json_args:) + env = { 'GITALY_TOKEN' => token } + args = [executable, gitaly_address, json_args] # We use 'chdir: ROOT_PATH' to let the next executable know where config.yml is. Kernel.exec(env, *args, unsetenv_others: true, chdir: ROOT_PATH) end @@ -274,21 +244,4 @@ class GitlabShell # rubocop:disable Metrics/ClassLength "#{resp['message']}" end end - - def git_trace_available? - return false unless @config.git_trace_log_file - - if Pathname(@config.git_trace_log_file).relative? - $logger.warn('git trace log path must be absolute, ignoring', git_trace_log_file: @config.git_trace_log_file) - return false - end - - begin - File.open(@config.git_trace_log_file, 'a') { nil } - return true - rescue => ex - $logger.warn('Failed to open git trace log file', git_trace_log_file: @config.git_trace_log_file, error: ex.to_s) - return false - end - end end diff --git a/spec/gitlab_shell_spec.rb b/spec/gitlab_shell_spec.rb index 96c4878..135a474 100644 --- a/spec/gitlab_shell_spec.rb +++ b/spec/gitlab_shell_spec.rb @@ -201,19 +201,13 @@ describe GitlabShell do end end - context 'git-upload-pack' do - it_behaves_like 'upload-pack', 'git-upload-pack' - end - - context 'git upload-pack' do - it_behaves_like 'upload-pack', 'git upload-pack' - end - context 'gitaly-upload-pack' do let(:ssh_cmd) { "git-upload-pack gitlab-ci.git" } + before do allow(api).to receive(:check_access).and_return(gitaly_check_access) end + after { subject.exec(ssh_cmd) } it "should process the command" do @@ -221,12 +215,13 @@ describe GitlabShell do end it "should execute the command" do - expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-upload-pack"), 'unix:gitaly.socket', gitaly_message) + expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-upload-pack"), gitaly_address: 'unix:gitaly.socket', json_args: gitaly_message, token: nil) end it "should log the command execution" do message = "executing git command" user_string = "user with id #{gl_id}" + expect($logger).to receive(:info).with(message, command: "gitaly-upload-pack unix:gitaly.socket #{gitaly_message}", user: user_string) end @@ -238,6 +233,11 @@ describe GitlabShell do context 'git-receive-pack' do let(:ssh_cmd) { "git-receive-pack gitlab-ci.git" } + + before do + allow(api).to receive(:check_access).and_return(gitaly_check_access) + end + after { subject.exec(ssh_cmd) } it "should process the command" do @@ -245,13 +245,13 @@ describe GitlabShell do end it "should execute the command" do - expect(subject).to receive(:exec_cmd).with('git-receive-pack') + expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-receive-pack"), gitaly_address: 'unix:gitaly.socket', json_args: gitaly_message, token: nil) end it "should log the command execution" do message = "executing git command" user_string = "user with id #{gl_id}" - expect($logger).to receive(:info).with(message, command: "git-receive-pack", user: user_string) + expect($logger).to receive(:info).with(message, command: "gitaly-receive-pack unix:gitaly.socket #{gitaly_message}", user: user_string) end end @@ -267,7 +267,7 @@ describe GitlabShell do end it "should execute the command" do - expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-receive-pack"), 'unix:gitaly.socket', gitaly_message) + expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-receive-pack"), gitaly_address: 'unix:gitaly.socket', json_args: gitaly_message, token: nil) end it "should log the command execution" do @@ -284,7 +284,6 @@ describe GitlabShell do shared_examples_for 'upload-archive' do |command| let(:ssh_cmd) { "#{command} gitlab-ci.git" } - let(:exec_cmd_params) { ['git-upload-archive'] } let(:exec_cmd_log_params) { exec_cmd_params } after { subject.exec(ssh_cmd) } @@ -309,14 +308,6 @@ describe GitlabShell do end end - context 'git-upload-archive' do - it_behaves_like 'upload-archive', 'git-upload-archive' - end - - context 'git upload-archive' do - it_behaves_like 'upload-archive', 'git upload-archive' - end - context 'gitaly-upload-archive' do before do allow(api).to receive(:check_access).and_return(gitaly_check_access) @@ -327,8 +318,7 @@ describe GitlabShell do let(:exec_cmd_params) do [ File.join(ROOT_PATH, "bin", gitaly_executable), - 'unix:gitaly.socket', - gitaly_message + { gitaly_address: 'unix:gitaly.socket', json_args: gitaly_message, token: nil } ] end let(:exec_cmd_log_params) do @@ -424,6 +414,19 @@ describe GitlabShell do let(:ssh_cmd) { "git-upload-pack gitlab-ci.git" } describe 'check access with api' do + before do + allow(api).to receive(:check_access).and_return( + GitAccessStatus.new( + false, + 'denied', + gl_repository: nil, + gl_id: nil, + gl_username: nil, + git_config_options: nil, + gitaly: nil, + git_protocol: nil)) + end + after { subject.exec(ssh_cmd) } it "should call api.check_access" do @@ -431,126 +434,10 @@ describe GitlabShell do end it "should disallow access and log the attempt if check_access returns false status" do - allow(api).to receive(:check_access).and_return(GitAccessStatus.new( - false, - 'denied', - gl_repository: nil, - gl_id: nil, - gl_username: nil, - git_config_options: nil, - gitaly: nil, - git_protocol: nil)) message = 'Access denied' user_string = "user with id #{gl_id}" - expect($logger).to receive(:warn).with(message, command: 'git-upload-pack gitlab-ci.git', user: user_string) - end - end - end - - describe :exec_cmd do - 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' => gl_id, - 'GL_PROTOCOL' => 'ssh', - 'GL_REPOSITORY' => gl_repository, - 'GL_USERNAME' => 'testuser' - } - end - let(:exec_options) { { unsetenv_others: true, chdir: ROOT_PATH } } - before do - allow(Kernel).to receive(:exec) - shell.gl_repository = gl_repository - shell.git_protocol = git_protocol - shell.instance_variable_set(:@username, gl_username) - end - - it "uses Kernel::exec method" do - expect(Kernel).to receive(:exec).with(env, 1, 2, exec_options).once - shell.send :exec_cmd, 1, 2 - end - it "refuses to execute a lone non-array argument" do - expect { shell.send :exec_cmd, 1 }.to raise_error(GitlabShell::DisallowedCommandError) - end - - it "allows one argument if it is an array" do - expect(Kernel).to receive(:exec).with(env, [1, 2], exec_options).once - shell.send :exec_cmd, [1, 2] - end - - context "when specifying a git_tracing log file" do - let(:git_trace_log_file) { '/tmp/git_trace_performance.log' } - - before do - allow_any_instance_of(GitlabConfig).to receive(:git_trace_log_file).and_return(git_trace_log_file) - shell - end - - it "uses GIT_TRACE_PERFORMANCE" do - expected_hash = hash_including( - 'GIT_TRACE' => git_trace_log_file, - 'GIT_TRACE_PACKET' => git_trace_log_file, - 'GIT_TRACE_PERFORMANCE' => git_trace_log_file - ) - expect(Kernel).to receive(:exec).with(expected_hash, [1, 2], exec_options).once - - shell.send :exec_cmd, [1, 2] - end - - context "when provides a relative path" do - let(:git_trace_log_file) { 'git_trace_performance.log' } - - it "does not uses GIT_TRACE*" do - # If we try to use it we'll show a warning to the users - expected_hash = hash_excluding( - 'GIT_TRACE', 'GIT_TRACE_PACKET', 'GIT_TRACE_PERFORMANCE' - ) - expect(Kernel).to receive(:exec).with(expected_hash, [1, 2], exec_options).once - - shell.send :exec_cmd, [1, 2] - end - - it "writes an entry on the log" do - message = 'git trace log path must be absolute, ignoring' - - expect($logger).to receive(:warn). - with(message, git_trace_log_file: git_trace_log_file) - - expect(Kernel).to receive(:exec).with(env, [1, 2], exec_options).once - shell.send :exec_cmd, [1, 2] - end - end - - context "when provides a file not writable" do - before do - expect(File).to receive(:open).with(git_trace_log_file, 'a').and_raise(Errno::EACCES) - end - - it "does not uses GIT_TRACE*" do - # If we try to use it we'll show a warning to the users - expected_hash = hash_excluding( - 'GIT_TRACE', 'GIT_TRACE_PACKET', 'GIT_TRACE_PERFORMANCE' - ) - expect(Kernel).to receive(:exec).with(expected_hash, [1, 2], exec_options).once - - shell.send :exec_cmd, [1, 2] - end - - it "writes an entry on the log" do - message = 'Failed to open git trace log file' - error = 'Permission denied' - - expect($logger).to receive(:warn). - with(message, git_trace_log_file: git_trace_log_file, error: error) - - expect(Kernel).to receive(:exec).with(env, [1, 2], exec_options).once - shell.send :exec_cmd, [1, 2] - end + expect($logger).to receive(:warn).with(message, command: 'git-upload-pack gitlab-ci.git', user: user_string) end end end |