From 4546d804b145008a2224def2a5c49882cb012555 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 20 Apr 2020 10:32:05 -0700 Subject: [PATCH] Add support for configuring SSH connect timeout Retains the original default value of 15 seconds for SSH connect timeout. Allows users to modify this timeout via SSH communicator option. Enforces integer values for timeout and validates custom values are greater than 0. --- lib/vagrant/machine.rb | 5 +- plugins/commands/ssh_config/command.rb | 2 +- plugins/communicators/ssh/communicator.rb | 13 +--- plugins/kernel_v2/config/ssh_connect.rb | 24 ++++++++ .../communicators/ssh/communicator_test.rb | 35 ++++++----- .../kernel_v2/config/ssh_connect_test.rb | 59 +++++++++++++++++++ test/unit/vagrant/machine_test.rb | 10 ++++ .../docs/vagrantfile/ssh_settings.html.md | 3 + 8 files changed, 121 insertions(+), 30 deletions(-) diff --git a/lib/vagrant/machine.rb b/lib/vagrant/machine.rb index f782a60f9..7f36d76cc 100644 --- a/lib/vagrant/machine.rb +++ b/lib/vagrant/machine.rb @@ -478,8 +478,9 @@ module Vagrant # We also set some fields that are purely controlled by Vagrant info[:forward_agent] = @config.ssh.forward_agent - info[:forward_x11] = @config.ssh.forward_x11 - info[:forward_env] = @config.ssh.forward_env + info[:forward_x11] = @config.ssh.forward_x11 + info[:forward_env] = @config.ssh.forward_env + info[:connect_timeout] = @config.ssh.connect_timeout info[:ssh_command] = @config.ssh.ssh_command if @config.ssh.ssh_command diff --git a/plugins/commands/ssh_config/command.rb b/plugins/commands/ssh_config/command.rb index 8b0d2830a..e0bf79ff2 100644 --- a/plugins/commands/ssh_config/command.rb +++ b/plugins/commands/ssh_config/command.rb @@ -55,7 +55,7 @@ module VagrantPlugins proxy_command: ssh_info[:proxy_command], ssh_command: ssh_info[:ssh_command], forward_env: ssh_info[:forward_env], - config: ssh_info[:config], + config: ssh_info[:config] } # Render the template and output directly to STDOUT diff --git a/plugins/communicators/ssh/communicator.rb b/plugins/communicators/ssh/communicator.rb index c333f519b..da43cb23e 100644 --- a/plugins/communicators/ssh/communicator.rb +++ b/plugins/communicators/ssh/communicator.rb @@ -407,14 +407,6 @@ module VagrantPlugins auth_methods << "publickey" if ssh_info[:private_key_path] auth_methods << "password" if ssh_info[:password] - # yanked directly from ruby's Net::SSH, but with `none` last - # TODO: Remove this once Vagrant has updated its dependency on Net:SSH - # to be > 4.1.0, which should include this fix. - cipher_array = Net::SSH::Transport::Algorithms::ALGORITHMS[:encryption].dup - if cipher_array.delete("none") - cipher_array.push("none") - end - # Build the options we'll use to initiate the connection via Net::SSH common_connect_opts = { auth_methods: auth_methods, @@ -425,10 +417,9 @@ module VagrantPlugins verify_host_key: ssh_info[:verify_host_key], password: ssh_info[:password], port: ssh_info[:port], - timeout: 15, + timeout: ssh_info[:connect_timeout], user_known_hosts_file: [], - verbose: :debug, - encryption: cipher_array, + verbose: :debug } # Connect to SSH, giving it a few tries diff --git a/plugins/kernel_v2/config/ssh_connect.rb b/plugins/kernel_v2/config/ssh_connect.rb index 897046bad..21116d26e 100644 --- a/plugins/kernel_v2/config/ssh_connect.rb +++ b/plugins/kernel_v2/config/ssh_connect.rb @@ -1,9 +1,12 @@ module VagrantPlugins module Kernel_V2 class SSHConnectConfig < Vagrant.plugin("2", :config) + DEFAULT_SSH_CONNECT_TIMEOUT = 15 + attr_accessor :host attr_accessor :port attr_accessor :config + attr_accessor :connect_timeout attr_accessor :private_key_path attr_accessor :username attr_accessor :password @@ -20,6 +23,7 @@ module VagrantPlugins @host = UNSET_VALUE @port = UNSET_VALUE @config = UNSET_VALUE + @connect_timeout = UNSET_VALUE @private_key_path = UNSET_VALUE @username = UNSET_VALUE @password = UNSET_VALUE @@ -47,6 +51,7 @@ module VagrantPlugins @dsa_authentication = true if @dsa_authentication == UNSET_VALUE @extra_args = nil if @extra_args == UNSET_VALUE @config = nil if @config == UNSET_VALUE + @connect_timeout = DEFAULT_SSH_CONNECT_TIMEOUT if @connect_timeout == UNSET_VALUE if @private_key_path && !@private_key_path.is_a?(Array) @private_key_path = [@private_key_path] @@ -76,6 +81,15 @@ module VagrantPlugins when :secure @verify_host_key = :always end + + # Attempt to convert timeout to integer value + # If we can't convert the connect timeout into an integer or + # if the value is less than 1, set it to the default value + begin + @connect_timeout = @connect_timeout.to_i + rescue + # ignore + end end # NOTE: This is _not_ a valid config validation method, since it @@ -110,6 +124,16 @@ module VagrantPlugins machine.env.ui.warn(I18n.t("vagrant.config.ssh.paranoid_deprecated")) end + if !@connect_timeout.is_a?(Integer) + errors << I18n.t( + "vagrant.config.ssh.connect_timeout_invalid_type", + given: @connect_timeout.class.name) + elsif @connect_timeout < 1 + errors << I18n.t( + "vagrant.config.ssh.connect_timeout_invalid_value", + given: @connect_timeout.to_s) + end + errors end end diff --git a/test/unit/plugins/communicators/ssh/communicator_test.rb b/test/unit/plugins/communicators/ssh/communicator_test.rb index f1c8df03b..d6e9d2b85 100644 --- a/test/unit/plugins/communicators/ssh/communicator_test.rb +++ b/test/unit/plugins/communicators/ssh/communicator_test.rb @@ -656,22 +656,6 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do ).and_return(true) communicator.send(:connect) end - - it "includes the default cipher array for encryption" do - cipher_array = %w(aes256-ctr aes192-ctr aes128-ctr - aes256-cbc aes192-cbc aes128-cbc - rijndael-cbc@lysator.liu.se blowfish-ctr - blowfish-cbc cast128-ctr cast128-cbc - 3des-ctr 3des-cbc idea-cbc arcfour256 - arcfour128 arcfour none) - - expect(Net::SSH).to receive(:start).with( - nil, nil, hash_including( - encryption: cipher_array - ) - ).and_return(true) - communicator.send(:connect) - end end context "with keys_only disabled and verify_host_key enabled" do @@ -885,6 +869,25 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do communicator.send(:connect) end end + + context "with connect_timeout configured" do + before do + expect(machine).to receive(:ssh_info).and_return( + host: '127.0.0.1', + port: 2222, + connect_timeout: 30 + ) + end + + it "has connect_timeout defined" do + expect(Net::SSH).to receive(:start).with( + anything, anything, hash_including( + timeout: 30 + ) + ).and_return(true) + communicator.send(:connect) + end + end end describe ".generate_environment_export" do diff --git a/test/unit/plugins/kernel_v2/config/ssh_connect_test.rb b/test/unit/plugins/kernel_v2/config/ssh_connect_test.rb index e11386f31..da6ba0a59 100644 --- a/test/unit/plugins/kernel_v2/config/ssh_connect_test.rb +++ b/test/unit/plugins/kernel_v2/config/ssh_connect_test.rb @@ -103,4 +103,63 @@ describe VagrantPlugins::Kernel_V2::SSHConnectConfig do expect(subject.remote_user).to eq(remote_user) end end + + describe "#connect_timeout" do + let(:timeout_value) { 1 } + + it "should default to the default value" do + subject.finalize! + expect(subject.connect_timeout). + to eq(described_class.const_get(:DEFAULT_SSH_CONNECT_TIMEOUT)) + end + + + it "should be set to provided value" do + subject.connect_timeout = timeout_value + subject.finalize! + expect(subject.connect_timeout).to eq(timeout_value) + end + + it "should cast given value to integer" do + subject.connect_timeout = timeout_value.to_s + subject.finalize! + expect(subject.connect_timeout).to eq(timeout_value) + end + + it "should properly validate" do + subject.connect_timeout = timeout_value + subject.finalize! + expect(subject.validate(machine)).to be_empty + end + + context "when value cannot be cast" do + let(:timeout_value) { :value } + + it "should not raise an error" do + subject.connect_timeout = timeout_value + expect { subject.finalize! }.not_to raise_error + end + + it "should not validate" do + subject.connect_timeout = timeout_value + subject.finalize! + expect(subject.validate(machine)).not_to be_empty + end + end + + context "when value is less than 1" do + let(:timeout_value) { 0 } + + it "should not raise an error" do + subject.connect_timeout = timeout_value + expect { subject.finalize! }.not_to raise_error + end + + it "should not validate" do + subject.connnect_timeout = timeout_value + subject.finalize! + expect(subject.validate(machine)).not_to be_empty + end + end + end end diff --git a/test/unit/vagrant/machine_test.rb b/test/unit/vagrant/machine_test.rb index 9665f48f9..646fc419e 100644 --- a/test/unit/vagrant/machine_test.rb +++ b/test/unit/vagrant/machine_test.rb @@ -825,6 +825,16 @@ describe Vagrant::Machine do expect(instance.ssh_info[:config]).to eq("/path/to/ssh_config") end + it "should return the default connect_timeout" do + expect(instance.ssh_info[:connect_timeout]). + to eq(VagrantPlugins::Kernel_V2::SSHConnectConfig::DEFAULT_SSH_CONNECT_TIMEOUT) + end + + it "should return the connect_timeout when set" do + instance.config.ssh.connect_timeout = 2 + expect(instance.ssh_info[:connect_timeout]).to eq(2) + end + context "with no data dir" do let(:base) { true } let(:data_dir) { nil } diff --git a/website/source/docs/vagrantfile/ssh_settings.html.md b/website/source/docs/vagrantfile/ssh_settings.html.md index f117436d9..6760848c2 100644 --- a/website/source/docs/vagrantfile/ssh_settings.html.md +++ b/website/source/docs/vagrantfile/ssh_settings.html.md @@ -22,6 +22,9 @@ defaults are typically fine, but you can fine tune whatever you would like. compression setting when ssh'ing into a machine. If this is not set, it will default to `true` and `Compression=yes` will be enabled with ssh. +* `config.ssh.connect_timeout` (integer) - Number of seconds to wait for establishing +an SSH connection to the guest. Defaults to `15`. + * `config.ssh.config` (string) - Path to a custom ssh_config file to use for configuring the SSH connections.