From 4e81be879c04f6f6c3dbd87b0f78a4a7206dd957 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Mon, 20 Oct 2014 17:33:06 +0200 Subject: [PATCH 1/2] 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 | 5 --- lib/vagrant/action/builtin/ssh_run.rb | 5 --- lib/vagrant/machine.rb | 10 ++++++ plugins/communicators/ssh/communicator.rb | 6 ---- .../vagrant/action/builtin/ssh_exec_test.rb | 20 ------------ test/unit/vagrant/machine_test.rb | 31 ++++++++++++++++++- 6 files changed, 40 insertions(+), 37 deletions(-) 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 9d2c0d15c..c38b983b2 100644 --- a/lib/vagrant/machine.rb +++ b/lib/vagrant/machine.rb @@ -1,3 +1,5 @@ +require_relative "util/ssh" + require "digest/md5" require "thread" @@ -434,6 +436,14 @@ module Vagrant 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 diff --git a/plugins/communicators/ssh/communicator.rb b/plugins/communicators/ssh/communicator.rb index f4d152002..f8d0978df 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 @@ -305,11 +304,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 d8e55e346..ec7c67f53 100644 --- a/test/unit/vagrant/machine_test.rb +++ b/test/unit/vagrant/machine_test.rb @@ -470,7 +470,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) @@ -480,9 +481,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| @@ -554,6 +559,30 @@ 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 + context "expanding path relative to the root path" do it "should with the provider key path" do provider_ssh_info[:private_key_path] = "~/foo" From 89a4a29d65cace4d970d0f220ad01815883ab8aa Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Mon, 20 Oct 2014 17:45:02 +0200 Subject: [PATCH 2/2] Memoize machine.ssh_info when ready for connection --- lib/vagrant/machine.rb | 13 +++++++++++-- test/unit/vagrant/machine_test.rb | 11 +++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/vagrant/machine.rb b/lib/vagrant/machine.rb index c38b983b2..4e3638c9b 100644 --- a/lib/vagrant/machine.rb +++ b/lib/vagrant/machine.rb @@ -72,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. @@ -377,6 +383,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. @@ -444,8 +453,8 @@ module Vagrant end end - # Return the final compiled SSH info data - info + # 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/test/unit/vagrant/machine_test.rb b/test/unit/vagrant/machine_test.rb index ec7c67f53..f24b4bd42 100644 --- a/test/unit/vagrant/machine_test.rb +++ b/test/unit/vagrant/machine_test.rb @@ -583,6 +583,17 @@ describe Vagrant::Machine do 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"