Merge pull request #4670 from gildegoma/dry-check-ssh-perms

Check SSH key permissions in machine.ssh_info

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

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

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

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

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 }
 

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"