Merge pull request #9400 from chrisroberts/fall-back-to-system-ssh

Allow which to use original path. Prefer native ssh.

lib/vagrant/util/ssh.rb

@@ -66,7 +66,18 @@ 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!
+
+        # 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")
+          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
           if Platform.windows?
             raise Errors::SSHUnavailableWindows,
@@ -79,9 +90,9 @@ module Vagrant
           raise Errors::SSHUnavailable
         end
 
+        if Platform.windows?
           # On Windows, we need to detect whether SSH is actually "plink"
           # underneath the covers. In this case, we tell the user.
-        if Platform.windows?
           r = Subprocess.execute(ssh_path)
           if r.stdout.include?("PuTTY Link") || r.stdout.include?("Plink: command-line connection utility")
             raise Errors::SSHIsPuttyLink,
@@ -185,7 +196,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]

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

test/unit/vagrant/util/ssh_test.rb

@@ -38,6 +38,21 @@ describe Vagrant::Util::SSH do
       dsa_authentication: true
     }}
 
+    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)
 
@@ -67,7 +82,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 +100,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 +118,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 +137,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 +155,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 +173,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 +191,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 +209,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
 

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']
+    allow(ENV).to receive(:[]).with("PATH").and_return(dir.to_s)
-    ENV['PATH'] = 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