From cb70749bd843197890069581a8649688f35a4297 Mon Sep 17 00:00:00 2001 From: Zack Train Date: Thu, 21 Jul 2016 18:15:21 -0700 Subject: [PATCH] redux of pr 7398 for ssh-agent key fix --- .gitignore | 1 + lib/vagrant/action/builtin/ssh_run.rb | 2 +- lib/vagrant/machine.rb | 4 +- lib/vagrant/util/ssh.rb | 22 +++++--- plugins/commands/ssh_config/command.rb | 9 ++- plugins/communicators/ssh/communicator.rb | 6 +- plugins/synced_folders/rsync/helper.rb | 38 ++++++++++--- templates/commands/ssh_config/config.erb | 10 ++++ .../commands/ssh_config/command_test.rb | 29 ++++++++++ .../synced_folders/rsync/helper_test.rb | 56 ++++++++++++++++++- test/unit/vagrant/machine_test.rb | 17 ++++++ .../docs/vagrantfile/ssh_settings.html.md | 2 +- 12 files changed, 173 insertions(+), 23 deletions(-) diff --git a/.gitignore b/.gitignore index a216d2989..681341552 100644 --- a/.gitignore +++ b/.gitignore @@ -41,6 +41,7 @@ doc/ # Ruby Managers .rbenv +.rbenv-gemsets .ruby-gemset .ruby-version .rvmrc diff --git a/lib/vagrant/action/builtin/ssh_run.rb b/lib/vagrant/action/builtin/ssh_run.rb index e4522d1f0..9d6cb8ab7 100644 --- a/lib/vagrant/action/builtin/ssh_run.rb +++ b/lib/vagrant/action/builtin/ssh_run.rb @@ -30,7 +30,7 @@ module Vagrant info[:private_key_path] ||= [] - if info[:private_key_path].empty? + if info[:keys_only] && info[:private_key_path].empty? raise Errors::SSHRunRequiresKeys end diff --git a/lib/vagrant/machine.rb b/lib/vagrant/machine.rb index 3806366d0..e7107fb62 100644 --- a/lib/vagrant/machine.rb +++ b/lib/vagrant/machine.rb @@ -443,6 +443,8 @@ module Vagrant # provider-returned data. info[:host] = @config.ssh.host if @config.ssh.host info[:port] = @config.ssh.port if @config.ssh.port + info[:keys_only] = @config.ssh.keys_only + info[:paranoid] = @config.ssh.paranoid info[:username] = @config.ssh.username if @config.ssh.username info[:password] = @config.ssh.password if @config.ssh.password @@ -462,7 +464,7 @@ module Vagrant if !info[:private_key_path] && !info[:password] if @config.ssh.private_key_path info[:private_key_path] = @config.ssh.private_key_path - else + elsif info[:keys_only] info[:private_key_path] = @env.default_private_key_path end end diff --git a/lib/vagrant/util/ssh.rb b/lib/vagrant/util/ssh.rb index f5d6b916d..faa965c26 100644 --- a/lib/vagrant/util/ssh.rb +++ b/lib/vagrant/util/ssh.rb @@ -109,19 +109,25 @@ module Vagrant "-p", options[:port].to_s, "-o", "Compression=yes", "-o", "DSAAuthentication=yes", - "-o", "LogLevel=#{log_level}", - "-o", "StrictHostKeyChecking=no", - "-o", "UserKnownHostsFile=/dev/null"] + "-o", "LogLevel=#{log_level}"] # Solaris/OpenSolaris/Illumos uses SunSSH which doesn't support the - # IdentitiesOnly option. Also, we don't enable it in plain mode so - # that SSH properly searches our identities and tries to do it itself. - if !Platform.solaris? && !plain_mode + # IdentitiesOnly option. Also, we don't enable it in plain mode or if + # if keys_only is false so that SSH and Net::SSH properly search our identities + # and tries to do it itself. + if !Platform.solaris? && !plain_mode && ssh_info[:keys_only] command_options += ["-o", "IdentitiesOnly=yes"] end - # If we're not in plain mode, attach the private key path. - if !plain_mode + # no strict hostkey checking unless paranoid + if ! ssh_info[:paranoid] + command_options += [ + "-o", "StrictHostKeyChecking=no", + "-o", "UserKnownHostsFile=/dev/null"] + end + + # If we're not in plain mode and :private_key_path is set attach the private key path(s). + if !plain_mode && options[:private_key_path] options[:private_key_path].each do |path| command_options += ["-i", path.to_s] end diff --git a/plugins/commands/ssh_config/command.rb b/plugins/commands/ssh_config/command.rb index 398a4276b..7e616f0c1 100644 --- a/plugins/commands/ssh_config/command.rb +++ b/plugins/commands/ssh_config/command.rb @@ -37,13 +37,20 @@ module VagrantPlugins ssh_host: ssh_info[:host], ssh_port: ssh_info[:port], ssh_user: ssh_info[:username], - private_key_path: ssh_info[:private_key_path], + keys_only: ssh_info[:keys_only], + paranoid: ssh_info[:paranoid], forward_agent: ssh_info[:forward_agent], forward_x11: ssh_info[:forward_x11], proxy_command: ssh_info[:proxy_command], ssh_command: ssh_info[:ssh_command], forward_env: ssh_info[:forward_env], } + if ssh_info[:private_key_path] + variables['private_key_path'] = ssh_info[:private_key_path] + end + if ssh_info[:log_level] + variables['log_level'] = ssh_info[:log_level] + end # Render the template and output directly to STDOUT template = "commands/ssh_config/config" diff --git a/plugins/communicators/ssh/communicator.rb b/plugins/communicators/ssh/communicator.rb index 49d7eb1f1..ab2eca558 100644 --- a/plugins/communicators/ssh/communicator.rb +++ b/plugins/communicators/ssh/communicator.rb @@ -334,7 +334,6 @@ module VagrantPlugins config: false, forward_agent: ssh_info[:forward_agent], send_env: ssh_info[:forward_env], - keys: ssh_info[:private_key_path], keys_only: ssh_info[:keys_only], paranoid: ssh_info[:paranoid], password: ssh_info[:password], @@ -375,6 +374,10 @@ module VagrantPlugins connect_opts = common_connect_opts.dup connect_opts[:logger] = ssh_logger + if ssh_info[:private_key_path] + connect_opts[:keys] = ssh_info[:private_key_path] + end + if ssh_info[:proxy_command] connect_opts[:proxy] = Net::SSH::Proxy::Command.new(ssh_info[:proxy_command]) end @@ -385,6 +388,7 @@ module VagrantPlugins @logger.info(" - Username: #{ssh_info[:username]}") @logger.info(" - Password? #{!!ssh_info[:password]}") @logger.info(" - Key Path: #{ssh_info[:private_key_path]}") + @logger.debug(" - connect_opts: #{connect_opts}") Net::SSH.start(ssh_info[:host], ssh_info[:username], connect_opts) ensure diff --git a/plugins/synced_folders/rsync/helper.rb b/plugins/synced_folders/rsync/helper.rb index 0efa43485..1a4348ce7 100644 --- a/plugins/synced_folders/rsync/helper.rb +++ b/plugins/synced_folders/rsync/helper.rb @@ -63,7 +63,11 @@ module VagrantPlugins opts[:owner] ||= ssh_info[:username] opts[:group] ||= ssh_info[:username] + # set log level + log_level = ssh_info[:log_level] || "FATAL" + # Connection information + # make it better match lib/vagrant/util/ssh.rb command_options style and logic username = ssh_info[:username] host = ssh_info[:host] proxy_command = "" @@ -80,15 +84,31 @@ module VagrantPlugins control_options = "-o ControlMaster=auto -o ControlPath=#{controlpath} -o ControlPersist=10m " end + # rsh cmd option rsh = [ - "ssh -p #{ssh_info[:port]} " + - proxy_command + - control_options + - "-o StrictHostKeyChecking=no " + - "-o IdentitiesOnly=true " + - "-o UserKnownHostsFile=/dev/null", - ssh_info[:private_key_path].map { |p| "-i '#{p}'" }, - ].flatten.join(" ") + "ssh", "-p", "#{ssh_info[:port]}", + proxy_command, + control_options, + ] + + # Solaris/OpenSolaris/Illumos uses SunSSH which doesn't support the + # IdentitiesOnly option. Also, we don't enable it if keys_only is false + # so that SSH properly searches our identities and tries to do it itself. + if !Vagrant::Util::Platform.solaris? && ssh_info[:keys_only] + rsh += ["-o", "IdentitiesOnly=yes"] + end + + # no strict hostkey checking unless paranoid + if ! ssh_info[:paranoid] + rsh += [ + "-o", "StrictHostKeyChecking=no", + "-o", "UserKnownHostsFile=/dev/null"] + end + + # If specified, attach the private key paths. + if ssh_info[:private_key_path] + ssh_info[:private_key_path].map { |p| "-i '#{p}'" } + end # Exclude some files by default, and any that might be configured # by the user. @@ -130,7 +150,7 @@ module VagrantPlugins command = [ "rsync", args, - "-e", rsh, + "-e", rsh.flatten.join(" "), excludes.map { |e| ["--exclude", e] }, hostpath, "#{username}@#{host}:#{guestpath}", diff --git a/templates/commands/ssh_config/config.erb b/templates/commands/ssh_config/config.erb index 2cdb2dbf4..dff2d6476 100644 --- a/templates/commands/ssh_config/config.erb +++ b/templates/commands/ssh_config/config.erb @@ -2,9 +2,12 @@ Host <%= host_key %> HostName <%= ssh_host %> User <%= ssh_user %> Port <%= ssh_port %> +<% if ! paranoid -%> UserKnownHostsFile /dev/null StrictHostKeyChecking no +<% end -%> PasswordAuthentication no +<% if private_key_path -%> <% private_key_path.each do |path| %> <% if path.include?(" ") -%> IdentityFile "<%= path %>" @@ -12,8 +15,15 @@ Host <%= host_key %> IdentityFile <%= path %> <% end -%> <% end -%> +<% end -%> +<% if keys_only -%> IdentitiesOnly yes +<% end -%> +<% if log_level -%> + LogLevel <%= log_level %> +<% else -%> LogLevel FATAL +<% end -%> <% if forward_agent -%> ForwardAgent yes <% end -%> diff --git a/test/unit/plugins/commands/ssh_config/command_test.rb b/test/unit/plugins/commands/ssh_config/command_test.rb index e458776ec..ba76d5de3 100644 --- a/test/unit/plugins/commands/ssh_config/command_test.rb +++ b/test/unit/plugins/commands/ssh_config/command_test.rb @@ -22,6 +22,8 @@ describe VagrantPlugins::CommandSSHConfig::Command do host: "testhost.vagrant.dev", port: 1234, username: "testuser", + keys_only: true, + paranoid: false, private_key_path: [], forward_agent: false, forward_x11: false @@ -107,5 +109,32 @@ Host #{machine.name} expect(output).to include('IdentityFile "with a space"') end + + it "omits IdentitiesOnly when keys_only is false" do + allow(machine).to receive(:ssh_info) { ssh_info.merge(keys_only: false) } + + output = "" + allow(subject).to receive(:safe_puts) do |data| + output += data if data + end + + subject.execute + + expect(output).not_to include('IdentitiesOnly') + end + + it "omits StrictHostKeyChecking and UserKnownHostsFile when paranoid is true" do + allow(machine).to receive(:ssh_info) { ssh_info.merge(paranoid: true) } + + output = "" + allow(subject).to receive(:safe_puts) do |data| + output += data if data + end + + subject.execute + + expect(output).not_to include('StrictHostKeyChecking ') + expect(output).not_to include('UserKnownHostsFile ') + end end end diff --git a/test/unit/plugins/synced_folders/rsync/helper_test.rb b/test/unit/plugins/synced_folders/rsync/helper_test.rb index 4b92687ab..e437de051 100644 --- a/test/unit/plugins/synced_folders/rsync/helper_test.rb +++ b/test/unit/plugins/synced_folders/rsync/helper_test.rb @@ -56,7 +56,7 @@ describe VagrantPlugins::SyncedFolderRSync::RsyncHelper do let(:result) { Vagrant::Util::Subprocess::Result.new(0, "", "") } let(:ssh_info) {{ - private_key_path: [], + :private_key_path => [], }} let(:opts) {{ hostpath: "/foo", @@ -213,4 +213,58 @@ describe VagrantPlugins::SyncedFolderRSync::RsyncHelper do end end end + + describe "#rsync_single with custom ssh_info" do + let(:result) { Vagrant::Util::Subprocess::Result.new(0, "", "") } + + let(:ssh_info) {{ + :private_key_path => [], + :keys_only => true, + :paranoid => false, + }} + let(:opts) {{ + hostpath: "/foo", + }} + let(:ui) { machine.ui } + + before do + Vagrant::Util::Subprocess.stub(execute: result) + + guest.stub(capability?: false) + end + + it "includes IdentitiesOnly, StrictHostKeyChecking, and UserKnownHostsFile with defaults" do + + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + expect(args[9]).to include('IdentitiesOnly') + expect(args[9]).to include('StrictHostKeyChecking') + expect(args[9]).to include('UserKnownHostsFile') + } + + subject.rsync_single(machine, ssh_info, opts) + end + + it "omits IdentitiesOnly with keys_only = false" do + ssh_info[:keys_only] = false + + Vagrant::Util::Subprocess.should_receive(:execute) do |*args| + expect(args[9]).not_to include('IdentitiesOnly') + result + end + + subject.rsync_single(machine, ssh_info, opts) + end + + it "omits StrictHostKeyChecking and UserKnownHostsFile with paranoid = true" do + ssh_info[:keys_only] = false + + Vagrant::Util::Subprocess.should_receive(:execute) do |*args| + expect(args[9]).not_to include('StrictHostKeyChecking ') + expect(args[9]).not_to include('UserKnownHostsFile ') + result + end + + subject.rsync_single(machine, ssh_info, opts) + end + end end diff --git a/test/unit/vagrant/machine_test.rb b/test/unit/vagrant/machine_test.rb index 3c15f7d87..c59d3b029 100644 --- a/test/unit/vagrant/machine_test.rb +++ b/test/unit/vagrant/machine_test.rb @@ -724,6 +724,23 @@ describe Vagrant::Machine do expect(instance.ssh_info[:password]).to eql("") end end + + context "with custom ssh_info" do + it "keys_only should be default" do + expect(instance.ssh_info[:keys_only]).to be_true + end + it "paranoid should be default" do + expect(instance.ssh_info[:paranoid]).to be_false + end + it "keys_only should be overridden" do + instance.config.ssh.keys_only = false + expect(instance.ssh_info[:keys_only]).to be_false + end + it "paranoid should be overridden" do + instance.config.ssh.paranoid = true + expect(instance.ssh_info[:paranoid]).to be_true + end + end end end diff --git a/website/source/docs/vagrantfile/ssh_settings.html.md b/website/source/docs/vagrantfile/ssh_settings.html.md index 88d94ab98..c45b79a6c 100644 --- a/website/source/docs/vagrantfile/ssh_settings.html.md +++ b/website/source/docs/vagrantfile/ssh_settings.html.md @@ -68,7 +68,7 @@ any keys stored in ssh-agent). The default value is `true`.`
`config.ssh.paranoid` - Perform strict host-key verification. The default value -is `true`. +is `false`.