From 343e252574064b5d7ef48afcbac6d3b5e544923f Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Wed, 8 Nov 2017 16:36:07 -0800 Subject: [PATCH] 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