From 02c3c9dad5bc497e416040734bde90951d8f13a8 Mon Sep 17 00:00:00 2001 From: Mark Lapierre Date: Fri, 25 Jan 2019 16:47:06 -0500 Subject: [PATCH 1/2] Add git credentials to .netrc when needed Avoid having to remember to call try_add_credentials_to_netrc after setting credentials. --- qa/qa/git/repository.rb | 47 ++++++--- qa/qa/resource/repository/push.rb | 2 - qa/spec/git/repository_spec.rb | 153 +++++++++++++++++++++--------- 3 files changed, 141 insertions(+), 61 deletions(-) diff --git a/qa/qa/git/repository.rb b/qa/qa/git/repository.rb index ac8dcbf0d83..df254ef288b 100644 --- a/qa/qa/git/repository.rb +++ b/qa/qa/git/repository.rb @@ -5,15 +5,19 @@ require 'uri' require 'open3' require 'fileutils' require 'tmpdir' +require 'tempfile' +require 'securerandom' module QA module Git class Repository include Scenario::Actable - attr_writer :password, :use_lfs + attr_writer :use_lfs attr_accessor :env_vars + InvalidCredentialsError = Class.new(RuntimeError) + def initialize # We set HOME to the current working directory (which is a # temporary directory created in .perform()) so the temporarily dropped @@ -28,6 +32,14 @@ module QA end end + def password=(password) + @password = password + + raise InvalidCredentialsError, "Please provide a username when setting a password" unless username + + try_add_credentials_to_netrc + end + def uri=(address) @uri = URI(address) end @@ -105,6 +117,8 @@ module QA File.binwrite(private_key_file, key.private_key) File.chmod(0700, private_key_file) + try_add_credentials_to_netrc + @known_hosts_file = Tempfile.new("known_hosts_#{SecureRandom.hex(8)}") keyscan_params = ['-H'] keyscan_params << "-p #{uri.port}" if uri.port @@ -148,16 +162,7 @@ module QA return unless add_credentials? return if netrc_already_contains_content? - # Despite libcurl supporting a custom .netrc location through the - # CURLOPT_NETRC_FILE environment variable, git does not support it :( - # Info: https://curl.haxx.se/libcurl/c/CURLOPT_NETRC_FILE.html - # - # This will create a .netrc in the correct working directory, which is - # a temporary directory created in .perform() - # - FileUtils.mkdir_p(tmp_home_dir) - File.open(netrc_file_path, 'a') { |file| file.puts(netrc_content) } - File.chmod(0600, netrc_file_path) + save_netrc_content end private @@ -214,6 +219,23 @@ module QA end end + def read_netrc_content + File.exist?(netrc_file_path) ? File.readlines(netrc_file_path) : [] + end + + def save_netrc_content + # Despite libcurl supporting a custom .netrc location through the + # CURLOPT_NETRC_FILE environment variable, git does not support it :( + # Info: https://curl.haxx.se/libcurl/c/CURLOPT_NETRC_FILE.html + # + # This will create a .netrc in the correct working directory, which is + # a temporary directory created in .perform() + # + FileUtils.mkdir_p(tmp_home_dir) + File.open(netrc_file_path, 'a') { |file| file.puts(netrc_content) } + File.chmod(0600, netrc_file_path) + end + def tmp_home_dir @tmp_home_dir ||= File.join(Dir.tmpdir, "qa-netrc-credentials", $$.to_s) end @@ -227,8 +249,7 @@ module QA end def netrc_already_contains_content? - File.exist?(netrc_file_path) && - File.readlines(netrc_file_path).grep(/^#{netrc_content}$/).any? + read_netrc_content.grep(/^#{netrc_content}$/).any? end end end diff --git a/qa/qa/resource/repository/push.rb b/qa/qa/resource/repository/push.rb index 32f15547da2..a5827fb6e73 100644 --- a/qa/qa/resource/repository/push.rb +++ b/qa/qa/resource/repository/push.rb @@ -67,8 +67,6 @@ module QA email = user.email end - repository.try_add_credentials_to_netrc - @output += repository.clone repository.configure_identity(username, email) diff --git a/qa/spec/git/repository_spec.rb b/qa/spec/git/repository_spec.rb index faa154c78da..48bcc8e8276 100644 --- a/qa/spec/git/repository_spec.rb +++ b/qa/spec/git/repository_spec.rb @@ -1,69 +1,130 @@ describe QA::Git::Repository do include Support::StubENV - let(:repository) { described_class.new } + shared_context 'git directory' do + let(:repository) { described_class.new } + let(:tmp_git_dir) { Dir.mktmpdir } + let(:tmp_netrc_dir) { Dir.mktmpdir } - before do - stub_env('GITLAB_USERNAME', 'root') - cd_empty_temp_directory - set_bad_uri - repository.use_default_credentials - end - - describe '#clone' do - it 'is unable to resolve host' do - expect(repository.clone).to include("fatal: unable to access 'http://root@foo/bar.git/'") - end - end - - describe '#push_changes' do before do - `git init` # need a repo to push from + stub_env('GITLAB_USERNAME', 'root') + cd_empty_temp_directory + set_bad_uri + + allow(repository).to receive(:tmp_home_dir).and_return(tmp_netrc_dir) end - it 'fails to push changes' do - expect(repository.push_changes).to include("error: failed to push some refs to 'http://root@foo/bar.git'") + after do + # Switch to a safe dir before deleting tmp dirs to avoid dir access errors + FileUtils.cd __dir__ + FileUtils.remove_entry_secure(tmp_git_dir, true) + FileUtils.remove_entry_secure(tmp_netrc_dir, true) + end + + def cd_empty_temp_directory + FileUtils.cd tmp_git_dir + end + + def set_bad_uri + repository.uri = 'http://foo/bar.git' end end - describe '#git_protocol=' do - [0, 1, 2].each do |version| - it "configures git to use protocol version #{version}" do - expect(repository).to receive(:run).with("git config protocol.version #{version}") - repository.git_protocol = version + context 'with default credentials' do + include_context 'git directory' do + before do + repository.use_default_credentials end end - it 'raises an error if the version is unsupported' do - expect { repository.git_protocol = 'foo' }.to raise_error(ArgumentError, "Please specify the protocol you would like to use: 0, 1, or 2") + describe '#clone' do + it 'is unable to resolve host' do + expect(repository.clone).to include("fatal: unable to access 'http://root@foo/bar.git/'") + end + end + + describe '#push_changes' do + before do + `git init` # need a repo to push from + end + + it 'fails to push changes' do + expect(repository.push_changes).to include("error: failed to push some refs to 'http://root@foo/bar.git'") + end + end + + describe '#git_protocol=' do + [0, 1, 2].each do |version| + it "configures git to use protocol version #{version}" do + expect(repository).to receive(:run).with("git config protocol.version #{version}") + repository.git_protocol = version + end + end + + it 'raises an error if the version is unsupported' do + expect { repository.git_protocol = 'foo' }.to raise_error(ArgumentError, "Please specify the protocol you would like to use: 0, 1, or 2") + end + end + + describe '#fetch_supported_git_protocol' do + it "reports the detected version" do + expect(repository).to receive(:run).and_return("packet: git< version 2") + expect(repository.fetch_supported_git_protocol).to eq('2') + end + + it 'reports unknown if version is unknown' do + expect(repository).to receive(:run).and_return("packet: git< version -1") + expect(repository.fetch_supported_git_protocol).to eq('unknown') + end + + it 'reports unknown if content does not identify a version' do + expect(repository).to receive(:run).and_return("foo") + expect(repository.fetch_supported_git_protocol).to eq('unknown') + end + end + + describe '#use_default_credentials' do + it 'adds credentials to .netrc' do + expect(File.read(File.join(tmp_netrc_dir, '.netrc'))) + .to eq("machine foo login #{QA::Runtime::User.default_username} password #{QA::Runtime::User.default_password}\n") + end end end - describe '#fetch_supported_git_protocol' do - it "reports the detected version" do - expect(repository).to receive(:run).and_return("packet: git< version 2") - expect(repository.fetch_supported_git_protocol).to eq('2') + context 'with specific credentials' do + include_context 'git directory' + + context 'before setting credentials' do + it 'does not add credentials to .netrc' do + expect(repository).not_to receive(:save_netrc_content) + end end - it 'reports unknown if version is unknown' do - expect(repository).to receive(:run).and_return("packet: git< version -1") - expect(repository.fetch_supported_git_protocol).to eq('unknown') + describe '#password=' do + it 'raises an error if no username was given' do + expect { repository.password = 'foo' } + .to raise_error(QA::Git::Repository::InvalidCredentialsError, + "Please provide a username when setting a password") + end + + it 'adds credentials to .netrc' do + repository.username = 'user' + repository.password = 'foo' + + expect(File.read(File.join(tmp_netrc_dir, '.netrc'))) + .to eq("machine foo login user password foo\n") + end end - it 'reports unknown if content does not identify a version' do - expect(repository).to receive(:run).and_return("foo") - expect(repository.fetch_supported_git_protocol).to eq('unknown') + describe '#use_ssh_key' do + it 'does not add credentials to .netrc' do + key = Struct.new(:private_key).new('foo') + + expect(repository).to receive(:try_add_credentials_to_netrc).and_call_original + expect(repository).not_to receive(:save_netrc_content) + + repository.use_ssh_key(key) + end end end - - def cd_empty_temp_directory - tmp_dir = 'tmp/git-repository-spec/' - FileUtils.rm_rf(tmp_dir) if ::File.exist?(tmp_dir) - FileUtils.mkdir_p tmp_dir - FileUtils.cd tmp_dir - end - - def set_bad_uri - repository.uri = 'http://foo/bar.git' - end end From f653952202a6734be597dd971c1155c82c6a1fba Mon Sep 17 00:00:00 2001 From: Mark Lapierre Date: Mon, 28 Jan 2019 15:30:50 -0500 Subject: [PATCH 2/2] Don't use .netrc with SSH There was a bug that required credentials when using SSH key auth when using LFS. That bug was fixed so we shouldn't need to add credentials to .netrc when using SSH anymore. --- qa/qa/git/repository.rb | 3 --- qa/spec/git/repository_spec.rb | 11 ----------- 2 files changed, 14 deletions(-) diff --git a/qa/qa/git/repository.rb b/qa/qa/git/repository.rb index df254ef288b..0aa94101098 100644 --- a/qa/qa/git/repository.rb +++ b/qa/qa/git/repository.rb @@ -117,8 +117,6 @@ module QA File.binwrite(private_key_file, key.private_key) File.chmod(0700, private_key_file) - try_add_credentials_to_netrc - @known_hosts_file = Tempfile.new("known_hosts_#{SecureRandom.hex(8)}") keyscan_params = ['-H'] keyscan_params << "-p #{uri.port}" if uri.port @@ -180,7 +178,6 @@ module QA def add_credentials? return false if !username || !password return true unless ssh_key_set? - return true if ssh_key_set? && use_lfs? false end diff --git a/qa/spec/git/repository_spec.rb b/qa/spec/git/repository_spec.rb index 48bcc8e8276..4a350cd6c42 100644 --- a/qa/spec/git/repository_spec.rb +++ b/qa/spec/git/repository_spec.rb @@ -115,16 +115,5 @@ describe QA::Git::Repository do .to eq("machine foo login user password foo\n") end end - - describe '#use_ssh_key' do - it 'does not add credentials to .netrc' do - key = Struct.new(:private_key).new('foo') - - expect(repository).to receive(:try_add_credentials_to_netrc).and_call_original - expect(repository).not_to receive(:save_netrc_content) - - repository.use_ssh_key(key) - end - end end end