Merge pull request #7703 from chrisroberts/cleanup/ssh_agent_2

ssh-agent interaction fix

.gitignore

@@ -41,6 +41,7 @@ doc/
 
 # Ruby Managers
 .rbenv
+.rbenv-gemsets
 .ruby-gemset
 .ruby-version
 .rvmrc

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
 

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

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

plugins/commands/ssh_config/command.rb

@@ -37,7 +37,10 @@ module VagrantPlugins
             ssh_host: ssh_info[:host],
             ssh_port: ssh_info[:port],
             ssh_user: ssh_info[:username],
+            keys_only: ssh_info[:keys_only],
+            paranoid: ssh_info[:paranoid],
             private_key_path: ssh_info[:private_key_path],
+            log_level: ssh_info[:log_level],
             forward_agent: ssh_info[:forward_agent],
             forward_x11:   ssh_info[:forward_x11],
             proxy_command: ssh_info[:proxy_command],

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

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,32 @@ 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]}",
+          "-o", "LogLevel=#{log_level}",
+          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]
+          rsh += 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 +151,7 @@ module VagrantPlugins
         command = [
           "rsync",
           args,
-          "-e", rsh,
+          "-e", rsh.flatten.join(" "),
           excludes.map { |e| ["--exclude", e] },
           hostpath,
           "#{username}@#{host}:#{guestpath}",

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 -%>

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

test/unit/plugins/synced_folders/rsync/helper_test.rb

@@ -213,4 +213,59 @@ 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 => ['/path/to/key'],
+      :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')
+        expect(args[9]).to include("-i '/path/to/key'")
+      }
+
+      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

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
 

website/source/docs/vagrantfile/ssh_settings.html.md

@@ -68,7 +68,7 @@ any keys stored in ssh-agent). The default value is `true`.`
 <hr>
 
 `config.ssh.paranoid` - Perform strict host-key verification. The default value
-is `true`.
+is `false`.
 
 <hr>