From 343e252574064b5d7ef48afcbac6d3b5e544923f Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Wed, 8 Nov 2017 16:36:07 -0800 Subject: [PATCH 1/4] Favor system ssh binary over embedded ssh binary --- lib/vagrant/util/ssh.rb | 28 +++++++++++++++++-- test/unit/vagrant/util/ssh_test.rb | 44 ++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/lib/vagrant/util/ssh.rb b/lib/vagrant/util/ssh.rb index eff40bf9e..b80c515fc 100644 --- a/lib/vagrant/util/ssh.rb +++ b/lib/vagrant/util/ssh.rb @@ -54,6 +54,23 @@ module Vagrant raise Errors::SSHKeyBadPermissions, key_path: key_path end + def self.determine_ssh_bin(original_ssh_path) + begin + original_path_env = ENV['PATH'] + ENV['PATH'] = ENV['VAGRANT_OLD_ENV_PATH'] + ssh_path = Which.which("ssh") + puts "the path: #{ssh_path}" + if !ssh_path.nil? + LOGGER.debug("Found default ssh binary. Using that instead...") + original_ssh_path = ssh_path + end + ensure + ENV['PATH'] = original_path_env + end + + return original_ssh_path + end + # Halts the running of this process and replaces it with a full-fledged # SSH shell into a remote machine. # @@ -79,9 +96,9 @@ module Vagrant raise Errors::SSHUnavailable end - # On Windows, we need to detect whether SSH is actually "plink" - # underneath the covers. In this case, we tell the user. if Platform.windows? + # On Windows, we need to detect whether SSH is actually "plink" + # underneath the covers. In this case, we tell the user. r = Subprocess.execute(ssh_path) if r.stdout.include?("PuTTY Link") || r.stdout.include?("Plink: command-line connection utility") raise Errors::SSHIsPuttyLink, @@ -90,6 +107,9 @@ module Vagrant username: ssh_info[:username], key_path: ssh_info[:private_key_path].join(", ") end + + # use system ssh if available + ssh_path = determine_ssh_bin(ssh_path) end # If plain mode is enabled then we don't do any authentication (we don't @@ -185,7 +205,9 @@ module Vagrant # we really don't care since both work. ENV["nodosfilewarning"] = "1" if Platform.cygwin? - ssh = ssh_info[:ssh_command] || 'ssh' + # If an ssh command is defined, use that. If an ssh binary was + # discovered on the path, use that. Otherwise fail to just trying `ssh` + ssh = ssh_info[:ssh_command] || ssh_path || 'ssh' # Invoke SSH with all our options if !opts[:subprocess] diff --git a/test/unit/vagrant/util/ssh_test.rb b/test/unit/vagrant/util/ssh_test.rb index 9d06bb895..351c393dd 100644 --- a/test/unit/vagrant/util/ssh_test.rb +++ b/test/unit/vagrant/util/ssh_test.rb @@ -28,6 +28,32 @@ describe Vagrant::Util::SSH do end end + describe "#determine_ssh_bin" do + let (:original_ssh_path) { "/system/dir/bin/ssh" } + let (:new_ssh_path) { "/new/ssh/path/bin/ssh" } + let (:old_path) { "/old/path/bin:/usr/local/bin:/usr/bin" } + + it "returns passed in ssh path if not found on original path" do + path = ENV["PATH"] + allow(ENV).to receive(:[]).with("PATH").and_return(path) + allow(ENV).to receive(:[]).with("VAGRANT_OLD_ENV_PATH").and_return(old_path) + allow(Vagrant::Util::Which).to receive(:which).and_return(nil) + + expect(described_class.determine_ssh_bin(original_ssh_path)).to eq(original_ssh_path) + expect(ENV["PATH"]).to eq(path) + end + + it "returns system ssh path if found on original path" do + path = ENV["PATH"] + allow(ENV).to receive(:[]).with("PATH").and_return(path) + allow(ENV).to receive(:[]).with("VAGRANT_OLD_ENV_PATH").and_return(old_path) + allow(Vagrant::Util::Which).to receive(:which).and_return(new_ssh_path) + + expect(described_class.determine_ssh_bin(original_ssh_path)).to eq(new_ssh_path) + expect(ENV["PATH"]).to eq(path) + end + end + describe "#exec" do let(:ssh_info) {{ host: "localhost", @@ -38,6 +64,8 @@ describe Vagrant::Util::SSH do dsa_authentication: true }} + let(:ssh_path) { "/usr/bin/ssh" } + it "raises an exception if there is no ssh" do allow(Vagrant::Util::Which).to receive(:which).and_return(nil) @@ -67,7 +95,7 @@ describe Vagrant::Util::SSH do expect(described_class.exec(ssh_info)).to eq(nil) expect(Vagrant::Util::SafeExec).to have_received(:exec) - .with("ssh", "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL","-o", "Compression=yes", "-o", "DSAAuthentication=yes", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"") + .with(ssh_path, "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL","-o", "Compression=yes", "-o", "DSAAuthentication=yes", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"") end context "when disabling compression or dsa_authentication flags" do @@ -85,7 +113,7 @@ describe Vagrant::Util::SSH do expect(described_class.exec(ssh_info)).to eq(nil) expect(Vagrant::Util::SafeExec).to have_received(:exec) - .with("ssh", "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"") + .with(ssh_path, "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"") end end @@ -103,7 +131,7 @@ describe Vagrant::Util::SSH do expect(described_class.exec(ssh_info)).to eq(nil) expect(Vagrant::Util::SafeExec).to have_received(:exec) - .with("ssh", "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"") + .with(ssh_path, "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"") end end @@ -122,7 +150,7 @@ describe Vagrant::Util::SSH do expect(described_class.exec(ssh_info, {plain_mode: true})).to eq(nil) expect(Vagrant::Util::SafeExec).to have_received(:exec) - .with("ssh", "localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null") + .with(ssh_path, "localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null") end end @@ -140,7 +168,7 @@ describe Vagrant::Util::SSH do expect(described_class.exec(ssh_info)).to eq(nil) expect(Vagrant::Util::SafeExec).to have_received(:exec) - .with("ssh", "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"","-o", "ForwardX11=yes", "-o", "ForwardX11Trusted=yes") + .with(ssh_path, "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"","-o", "ForwardX11=yes", "-o", "ForwardX11Trusted=yes") end end @@ -158,7 +186,7 @@ describe Vagrant::Util::SSH do expect(described_class.exec(ssh_info)).to eq(nil) expect(Vagrant::Util::SafeExec).to have_received(:exec) - .with("ssh", "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"","-o", "ForwardAgent=yes") + .with(ssh_path, "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"","-o", "ForwardAgent=yes") end end @@ -176,7 +204,7 @@ describe Vagrant::Util::SSH do expect(described_class.exec(ssh_info)).to eq(nil) expect(Vagrant::Util::SafeExec).to have_received(:exec) - .with("ssh", "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"", "-L", "8008:localhost:80") + .with(ssh_path, "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"", "-L", "8008:localhost:80") end end @@ -194,7 +222,7 @@ describe Vagrant::Util::SSH do expect(described_class.exec(ssh_info)).to eq(nil) expect(Vagrant::Util::SafeExec).to have_received(:exec) - .with("ssh", "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"", "-6") + .with(ssh_path, "vagrant@localhost", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "IdentityFile=\"#{ssh_info[:private_key_path][0]}\"", "-6") end end From 6f6e9364510ed91ebb133f40fbf31b182f8a0fa9 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Wed, 17 Jan 2018 13:14:21 -0800 Subject: [PATCH 2/4] Add option to Which utility for using original path on lookup --- lib/vagrant/util/which.rb | 12 ++++++++++-- test/unit/vagrant/util/which_test.rb | 13 ++++++++++--- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/lib/vagrant/util/which.rb b/lib/vagrant/util/which.rb index 0c8fd34d7..0e0eb03c4 100644 --- a/lib/vagrant/util/which.rb +++ b/lib/vagrant/util/which.rb @@ -11,8 +11,10 @@ module Vagrant # http://stackoverflow.com/questions/2108727/which-in-ruby-checking-if-program-exists-in-path-from-ruby # # @param [String] cmd The command to search for in the PATH. + # @param [Hash] opts Optional flags + # @option [Boolean] :original_path Search within original path if available # @return [String] The full path to the executable or `nil` if not found. - def self.which(cmd) + def self.which(cmd, **opts) exts = nil if !Platform.windows? || ENV['PATHEXT'].nil? @@ -29,8 +31,14 @@ module Vagrant exts = ENV['PATHEXT'].split(';') end + if opts[:original_path] + search_path = ENV.fetch('VAGRANT_OLD_ENV_PATH', ENV['PATH']) + else + search_path = ENV['PATH'] + end + SilenceWarnings.silence! do - ENV['PATH'].encode('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '').split(File::PATH_SEPARATOR).each do |path| + search_path.encode('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '').split(File::PATH_SEPARATOR).each do |path| exts.each do |ext| exe = "#{path}#{File::SEPARATOR}#{cmd}#{ext}" return exe if File.executable? exe diff --git a/test/unit/vagrant/util/which_test.rb b/test/unit/vagrant/util/which_test.rb index 7a7d17b87..d26a24bdb 100644 --- a/test/unit/vagrant/util/which_test.rb +++ b/test/unit/vagrant/util/which_test.rb @@ -13,10 +13,8 @@ describe Vagrant::Util::Which do file.chmod(mode) # set the path to the directory where the file is located - savepath = ENV['PATH'] - ENV['PATH'] = dir.to_s + allow(ENV).to receive(:[]).with("PATH").and_return(dir.to_s) block.call filename + test_extension - ENV['PATH'] = savepath file.unlink end @@ -40,4 +38,13 @@ describe Vagrant::Util::Which do expect(described_class.which(name)).to be_nil end end + + context "original_path option" do + before{ allow(ENV).to receive(:[]).with("PATH").and_return("") } + + it "should use the original path when instructed" do + expect(ENV).to receive(:fetch).with("VAGRANT_OLD_ENV_PATH", any_args).and_return("") + described_class.which("file", original_path: true) + end + end end From 570d5abb9581bccc1e105f452692a047f81c74ad Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Wed, 17 Jan 2018 13:14:48 -0800 Subject: [PATCH 3/4] Prefer ssh executable found on original path --- lib/vagrant/util/ssh.rb | 29 ++++++---------------- test/unit/vagrant/util/ssh_test.rb | 39 ++++++++++-------------------- 2 files changed, 21 insertions(+), 47 deletions(-) diff --git a/lib/vagrant/util/ssh.rb b/lib/vagrant/util/ssh.rb index b80c515fc..8d120aab0 100644 --- a/lib/vagrant/util/ssh.rb +++ b/lib/vagrant/util/ssh.rb @@ -54,23 +54,6 @@ module Vagrant raise Errors::SSHKeyBadPermissions, key_path: key_path end - def self.determine_ssh_bin(original_ssh_path) - begin - original_path_env = ENV['PATH'] - ENV['PATH'] = ENV['VAGRANT_OLD_ENV_PATH'] - ssh_path = Which.which("ssh") - puts "the path: #{ssh_path}" - if !ssh_path.nil? - LOGGER.debug("Found default ssh binary. Using that instead...") - original_ssh_path = ssh_path - end - ensure - ENV['PATH'] = original_path_env - end - - return original_ssh_path - end - # Halts the running of this process and replaces it with a full-fledged # SSH shell into a remote machine. # @@ -83,7 +66,14 @@ module Vagrant def self.exec(ssh_info, opts={}) # Ensure the platform supports ssh. On Windows there are several programs which # include ssh, notably git, mingw and cygwin, but make sure ssh is in the path! - ssh_path = Which.which("ssh") + + # First try using the original path provided + ssh_path = Which.which("ssh", original_path: true) + # If we didn't find an ssh executable, see if we shipped one + if !ssh_path + ssh_path = Which.which("ssh") + end + if !ssh_path if Platform.windows? raise Errors::SSHUnavailableWindows, @@ -107,9 +97,6 @@ module Vagrant username: ssh_info[:username], key_path: ssh_info[:private_key_path].join(", ") end - - # use system ssh if available - ssh_path = determine_ssh_bin(ssh_path) end # If plain mode is enabled then we don't do any authentication (we don't diff --git a/test/unit/vagrant/util/ssh_test.rb b/test/unit/vagrant/util/ssh_test.rb index 351c393dd..7f9b40fdc 100644 --- a/test/unit/vagrant/util/ssh_test.rb +++ b/test/unit/vagrant/util/ssh_test.rb @@ -28,32 +28,6 @@ describe Vagrant::Util::SSH do end end - describe "#determine_ssh_bin" do - let (:original_ssh_path) { "/system/dir/bin/ssh" } - let (:new_ssh_path) { "/new/ssh/path/bin/ssh" } - let (:old_path) { "/old/path/bin:/usr/local/bin:/usr/bin" } - - it "returns passed in ssh path if not found on original path" do - path = ENV["PATH"] - allow(ENV).to receive(:[]).with("PATH").and_return(path) - allow(ENV).to receive(:[]).with("VAGRANT_OLD_ENV_PATH").and_return(old_path) - allow(Vagrant::Util::Which).to receive(:which).and_return(nil) - - expect(described_class.determine_ssh_bin(original_ssh_path)).to eq(original_ssh_path) - expect(ENV["PATH"]).to eq(path) - end - - it "returns system ssh path if found on original path" do - path = ENV["PATH"] - allow(ENV).to receive(:[]).with("PATH").and_return(path) - allow(ENV).to receive(:[]).with("VAGRANT_OLD_ENV_PATH").and_return(old_path) - allow(Vagrant::Util::Which).to receive(:which).and_return(new_ssh_path) - - expect(described_class.determine_ssh_bin(original_ssh_path)).to eq(new_ssh_path) - expect(ENV["PATH"]).to eq(path) - end - end - describe "#exec" do let(:ssh_info) {{ host: "localhost", @@ -66,6 +40,19 @@ describe Vagrant::Util::SSH do let(:ssh_path) { "/usr/bin/ssh" } + it "searches original PATH for exectuable" do + expect(Vagrant::Util::Which).to receive(:which).with("ssh", original_path: true).and_return("valid-return") + allow(Vagrant::Util::SafeExec).to receive(:exec).and_return(nil) + described_class.exec(ssh_info) + end + + it "searches current PATH if original PATH did not result in valid executable" do + expect(Vagrant::Util::Which).to receive(:which).with("ssh", original_path: true).and_return(nil) + expect(Vagrant::Util::Which).to receive(:which).with("ssh").and_return("valid-return") + allow(Vagrant::Util::SafeExec).to receive(:exec).and_return(nil) + described_class.exec(ssh_info) + end + it "raises an exception if there is no ssh" do allow(Vagrant::Util::Which).to receive(:which).and_return(nil) From 19d1cae92cd17f70044082505d7b0f9ed5aab2af Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 25 Jan 2018 08:35:08 -0800 Subject: [PATCH 4/4] Add log warning when native ssh executable is not found --- lib/vagrant/util/ssh.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/vagrant/util/ssh.rb b/lib/vagrant/util/ssh.rb index 8d120aab0..31ce31da5 100644 --- a/lib/vagrant/util/ssh.rb +++ b/lib/vagrant/util/ssh.rb @@ -72,6 +72,10 @@ module Vagrant # If we didn't find an ssh executable, see if we shipped one if !ssh_path ssh_path = Which.which("ssh") + if ssh_path && Platform.windows? && (Platform.cygwin? || Platform.msys?) + LOGGER.warn("Failed to locate native SSH executable. Using vendored version.") + LOGGER.warn("If display issues are encountered, install the ssh package for your environment.") + end end if !ssh_path