Check SSH key permissions in machine.ssh_info

With this change, any caller of machine.ssh_info is assured that best
efforts will be done to fix possible wrong permissions on the private
key files.

Fix #4652

lib/vagrant/action/builtin/ssh_exec.rb

@@ -31,11 +31,6 @@ def call(env)
 
           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 @@ def call(env)
 
           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"
 
@@ -434,6 +436,14 @@ def ssh_info
         File.expand_path(path, @env.root_path)
       end
 
+      # 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
+
       # Return the final compiled SSH info data
       info
     end

plugins/communicators/ssh/communicator.rb

@@ -13,7 +13,6 @@
 require 'vagrant/util/file_mode'
 require 'vagrant/util/platform'
 require 'vagrant/util/retryable'
-require 'vagrant/util/ssh'
 
 module VagrantPlugins
   module CommunicatorSSH
@@ -305,11 +304,6 @@ def connect(**opts)
           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 @@
   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 @@
       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

@@ -470,7 +470,8 @@ def detect?(machine)
     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)
@@ -480,9 +481,13 @@ def detect?(machine)
 
     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|
@@ -554,6 +559,30 @@ def detect?(machine)
         ])
       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
+
       context "expanding path relative to the root path" do
         it "should with the provider key path" do
           provider_ssh_info[:private_key_path] = "~/foo"