diff --git a/lib/vagrant/action/builtin/ssh_exec.rb b/lib/vagrant/action/builtin/ssh_exec.rb index 23b209066..81ae53cd6 100644 --- a/lib/vagrant/action/builtin/ssh_exec.rb +++ b/lib/vagrant/action/builtin/ssh_exec.rb @@ -31,11 +31,6 @@ module Vagrant info[:private_key_path] ||= [] - # Check SSH key permissions - info[:private_key_path].each do |path| - SSH.check_key_permissions(Pathname.new(path)) - end - if info[:private_key_path].empty? && info[:password] env[:ui].warn(I18n.t("vagrant.ssh_exec_password")) end diff --git a/lib/vagrant/action/builtin/ssh_run.rb b/lib/vagrant/action/builtin/ssh_run.rb index db2a0d79e..d4058e6e4 100644 --- a/lib/vagrant/action/builtin/ssh_run.rb +++ b/lib/vagrant/action/builtin/ssh_run.rb @@ -29,11 +29,6 @@ module Vagrant info[:private_key_path] ||= [] - # Check SSH key permissions - info[:private_key_path].each do |path| - SSH.check_key_permissions(Pathname.new(path)) - end - if info[:private_key_path].empty? raise Errors::SSHRunRequiresKeys end diff --git a/lib/vagrant/machine.rb b/lib/vagrant/machine.rb index ed077fd6e..d23f759a2 100644 --- a/lib/vagrant/machine.rb +++ b/lib/vagrant/machine.rb @@ -1,3 +1,5 @@ +require_relative "util/ssh" + require "digest/md5" require "thread" @@ -70,6 +72,12 @@ module Vagrant # @return [Vagrantfile] attr_reader :vagrantfile + # The SSH information for accessing this machine. + # This attribute is set only when the machine is ready for SSH communication. + # + # @return [Hash] + attr_reader :ssh_info + # Initialize a new machine. # # @param [String] name Name of the virtual machine. @@ -383,6 +391,9 @@ module Vagrant # # @return [Hash] SSH information. def ssh_info + + return @ssh_info unless @ssh_info.nil? + # First, ask the provider for their information. If the provider # returns nil, then the machine is simply not ready for SSH, and # we return nil as well. @@ -442,8 +453,16 @@ module Vagrant File.expand_path(path, @env.root_path) end - # Return the final compiled SSH info data - info + # Check that the private key permissions are valid + info[:private_key_path].each do |path| + key_path = Pathname.new(path) + if key_path.exist? + Vagrant::Util::SSH.check_key_permissions(key_path) + end + end + + # Memoize the final compiled SSH info data and return it + @ssh_info = info end # Returns the state of this machine. The state is queried from the diff --git a/plugins/communicators/ssh/communicator.rb b/plugins/communicators/ssh/communicator.rb index 01764b9a0..cb9452d1c 100644 --- a/plugins/communicators/ssh/communicator.rb +++ b/plugins/communicators/ssh/communicator.rb @@ -13,7 +13,6 @@ require 'vagrant/util/ansi_escape_code_remover' require 'vagrant/util/file_mode' require 'vagrant/util/platform' require 'vagrant/util/retryable' -require 'vagrant/util/ssh' module VagrantPlugins module CommunicatorSSH @@ -314,11 +313,6 @@ module VagrantPlugins verbose: :debug, } - # Check that the private key permissions are valid - ssh_info[:private_key_path].each do |path| - Vagrant::Util::SSH.check_key_permissions(Pathname.new(path)) - end - # Connect to SSH, giving it a few tries connection = nil begin diff --git a/test/unit/vagrant/action/builtin/ssh_exec_test.rb b/test/unit/vagrant/action/builtin/ssh_exec_test.rb index 17f355497..94b42b99c 100644 --- a/test/unit/vagrant/action/builtin/ssh_exec_test.rb +++ b/test/unit/vagrant/action/builtin/ssh_exec_test.rb @@ -1,7 +1,5 @@ require File.expand_path("../../../../base", __FILE__) -require "vagrant/util/ssh" - describe Vagrant::Action::Builtin::SSHExec do let(:app) { lambda { |env| } } let(:env) { { machine: machine } } @@ -16,7 +14,6 @@ describe Vagrant::Action::Builtin::SSHExec do before(:each) do # Stub the methods so that even if we test incorrectly, no side # effects actually happen. - allow(ssh_klass).to receive(:check_key_permissions) allow(ssh_klass).to receive(:exec) end @@ -29,23 +26,6 @@ describe Vagrant::Action::Builtin::SSHExec do to raise_error(Vagrant::Errors::SSHNotReady) end - it "should check key permissions then exec" do - key_path = "/foo" - machine_ssh_info[:private_key_path] = [key_path] - - expect(ssh_klass).to receive(:check_key_permissions). - with(Pathname.new(key_path)). - once. - ordered - - expect(ssh_klass).to receive(:exec). - with(machine_ssh_info, nil). - once. - ordered - - described_class.new(app, env).call(env) - end - it "should exec with the SSH info in the env if given" do ssh_info = { foo: :bar } diff --git a/test/unit/vagrant/machine_test.rb b/test/unit/vagrant/machine_test.rb index 063043399..bc912a6d7 100644 --- a/test/unit/vagrant/machine_test.rb +++ b/test/unit/vagrant/machine_test.rb @@ -473,7 +473,8 @@ describe Vagrant::Machine do end end - describe "ssh info" do + describe "#ssh_info" do + describe "with the provider returning nil" do it "should return nil if the provider returns nil" do expect(provider).to receive(:ssh_info).and_return(nil) @@ -483,9 +484,13 @@ describe Vagrant::Machine do describe "with the provider returning data" do let(:provider_ssh_info) { {} } + let(:ssh_klass) { Vagrant::Util::SSH } before(:each) do allow(provider).to receive(:ssh_info).and_return(provider_ssh_info) + # Stub the check_key_permissions method so that even if we test incorrectly, + # no side effects actually happen. + allow(ssh_klass).to receive(:check_key_permissions) end [:host, :port, :username].each do |type| @@ -557,6 +562,41 @@ describe Vagrant::Machine do ]) end + it "should check and try to fix the permissions of the default private key file" do + provider_ssh_info[:private_key_path] = nil + instance.config.ssh.private_key_path = nil + + expect(ssh_klass).to receive(:check_key_permissions).once.with(Pathname.new(instance.env.default_private_key_path.to_s)) + instance.ssh_info + end + + it "should check and try to fix the permissions of given private key files" do + provider_ssh_info[:private_key_path] = nil + # Use __FILE__ to provide an existing file + instance.config.ssh.private_key_path = [File.expand_path(__FILE__), File.expand_path(__FILE__)] + + expect(ssh_klass).to receive(:check_key_permissions).twice.with(Pathname.new(File.expand_path(__FILE__))) + instance.ssh_info + end + + it "should not check the permissions of a private key file that does not exist" do + provider_ssh_info[:private_key_path] = "/foo" + + expect(ssh_klass).to_not receive(:check_key_permissions) + instance.ssh_info + end + + # It is not possible to test the memoization of a Ruby Hash with object equality, + # but we can verify that some code of ssh_info method is not executed again. + it "should check and try to fix the permissions of the private key file only once" do + provider_ssh_info[:private_key_path] = nil + instance.config.ssh.private_key_path = nil + + expect(ssh_klass).to receive(:check_key_permissions).once.with(Pathname.new(instance.env.default_private_key_path.to_s)) + instance.ssh_info + instance.ssh_info + end + context "expanding path relative to the root path" do it "should with the provider key path" do provider_ssh_info[:private_key_path] = "~/foo"