From 9e7df846855d44302b516b6ee31b89fbb0477596 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 16 Aug 2018 15:22:50 +0200 Subject: Clean up cmd_exec execution environment Given the gitaly-* now proxy the data from the client to the Gitaly server, the environment variables aren't used. Therefor we don't have to set them either. Only exception to the rule, is the GITALY_TOKEN. These changes also remove the `GIT_TRACE` options, introduced by 192e2bd367494bf66746c8971896a2d9cb84fc92. Part of: https://gitlab.com/gitlab-org/gitaly/issues/1300 --- config.yml.example | 7 --- lib/gitlab_config.rb | 4 -- lib/gitlab_shell.rb | 75 +++++------------------ spec/gitlab_shell_spec.rb | 151 +++++++++------------------------------------- 4 files changed, 41 insertions(+), 196 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 286a3d1..c7663e1 100644 --- a/lib/gitlab_shell.rb +++ b/lib/gitlab_shell.rb @@ -19,7 +19,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength GIT_COMMANDS = GITALY_COMMAND.keys + ['git-lfs-authenticate'] 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 @@ -139,61 +139,31 @@ class GitlabShell # rubocop:disable Metrics/ClassLength return end - # TODO happens only in testing as of right now - return unless @gitaly - - # 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 = { + # 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 = [@gitaly['address'], JSON.dump(gitaly_request)] + ) - executable = GITALY_COMMAND[@command] - args_string = [File.basename(executable), *args].join(' ') + gitaly_address = @gitaly['address'] + executable = GITALY_COMMAND.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 - } - - # @gitaly is a thing, unless another code path exists that doesn't go through process_cmd - if @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 1841e1e..135a474 100644 --- a/spec/gitlab_shell_spec.rb +++ b/spec/gitlab_shell_spec.rb @@ -203,9 +203,11 @@ describe GitlabShell do 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 @@ -213,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 @@ -230,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 @@ -237,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 @@ -259,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 @@ -276,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) } @@ -311,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 @@ -408,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 @@ -415,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 -- cgit v1.2.1