diff options
author | Marc A. Paradise <marc.paradise@gmail.com> | 2019-05-14 16:20:29 -0400 |
---|---|---|
committer | Marc A. Paradise <marc.paradise@gmail.com> | 2019-05-14 16:35:02 -0400 |
commit | 210900d54cb5fea5a77ffb39c2b8ae3f540ca1fd (patch) | |
tree | c3e991f75bec0939a346f581be53e4b4b5391c0a | |
parent | c6f3b113057e4848e80cb04c30f3298898faa030 (diff) | |
download | chef-210900d54cb5fea5a77ffb39c2b8ae3f540ca1fd.tar.gz |
Bootstrap updatesmp/bootstrap-updates
This includes a few fixes:
1. properly handle the default value for session_timeout in cases where
merge_config is not called (plugins) to populate defaults.
2. verify that we're using the ssh protoocl before referencing ssh
constants.
3. capture and compare the higher-level Train::Error for the check
to see if we've gotten a missing fingerprint error, instead of using
Train::Transports::SSHFailed which may not be loaded when exception
is being evaluated.
4. Add tests for missing fingerprint behavior
5. do not save session_timeout to Chef::Config[:knife], it was not
referenced as knife config in any location and we're trying to move away
from pushing CLI values into knife config.
Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>
-rw-r--r-- | lib/chef/knife/bootstrap.rb | 57 | ||||
-rw-r--r-- | spec/unit/knife/bootstrap_spec.rb | 53 |
2 files changed, 73 insertions, 37 deletions
diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb index 477074fe49..1e5580a61d 100644 --- a/lib/chef/knife/bootstrap.rb +++ b/lib/chef/knife/bootstrap.rb @@ -61,7 +61,6 @@ class Chef option :session_timeout, long: "--session-timeout SECONDS", description: "The number of seconds to wait for each connection operation to be acknowledged while running bootstrap.", - proc: Proc.new { |protocol| Chef::Config[:knife][:session_timeout] = protocol }, default: 60 # WinRM Authentication @@ -572,7 +571,7 @@ class Chef chef_vault_handler.run(client_builder.client) else ui.info <<~EOM - Performing legacy client registration with the validation key at #{Chef::Config[:validation_key]}..." + Performing legacy client registration with the validation key at #{Chef::Config[:validation_key]}... Delete your validation key in order to use your user credentials for client registration instead. EOM @@ -596,27 +595,28 @@ class Chef ui.info("Connecting to #{ui.color(server_name, :bold)}") opts = connection_opts.dup do_connect(opts) - rescue Train::Transports::SSHFailed => e - if e.message =~ /fingerprint (\S+) is unknown for "(.+)"/ + rescue Train::Error => e + # We handle these by message text only because train only loads the + # transports and protocols that it needs - so the exceptions may not be defined, + # and we don't want to require files internal to train. + if e.message =~ /fingerprint (\S+) is unknown for "(.+)"/ # Train::Transports::SSHFailed fingerprint = $1 hostname, ip = $2.split(",") # TODO: convert the SHA256 base64 value to hex with colons # 'ssh' example output: # RSA key fingerprint is e5:cb:c0:e2:21:3b:12:52:f8:ce:cb:00:24:e2:0c:92. # ECDSA key fingerprint is 5d:67:61:08:a9:d7:01:fd:5e:ae:7e:09:40:ef:c0:3c. - puts "The authenticity of host '#{hostname} (#{ip})' can't be established." - puts "fingerprint is #{fingerprint}." - ui.confirm("Are you sure you want to continue connecting") # will exit 3 on N + # will exit 3 on N + ui.confirm <<~EOM + The authenticity of host '#{hostname} (#{ip})' can't be established. + fingerprint is #{fingerprint}. + + Are you sure you want to continue connecting + EOM # FIXME: this should save the key to known_hosts but doesn't appear to be config[:ssh_verify_host_key] = :accept_new - connection_opts(reset: true) - retry - end - - raise e - rescue Train::Error => e - require "net/ssh" - if e.cause && e.cause.class == Net::SSH::AuthenticationFailed + do_connect(connection_opts(reset: true)) + elsif ssh? && e.cause && e.cause.class == Net::SSH::AuthenticationFailed if connection.password_auth? raise else @@ -632,6 +632,9 @@ class Chef end end + def handle_ssh_error(e) + end + # url values override CLI flags, if you provide both # we'll use the one that you gave in the URL. def connection_protocol @@ -769,15 +772,11 @@ class Chef # minutes as its unit, instead of seconds. # Warn the human so that they are not surprised. # - # This will also erroneously warn if a string value is given, - # but argument type validation is something that needs addressing - # more broadly. def warn_on_short_session_timeout - timeout = config_value(:session_timeout).to_i - if timeout <= 15 + if session_timeout && session_timeout <= 15 ui.warn <<~EOM - --session-timeout is set to #{config[:session_timeout]} minutes. - Did you mean "--session-timeout #{config[:session_timeout] * 60}" seconds? + You provided '--session-timeout #{session_timeout}' second(s). + Did you mean '--session-timeout #{session_timeout * 60}' seconds? EOM end end @@ -868,7 +867,7 @@ class Chef return opts if winrm? opts[:non_interactive] = true # Prevent password prompts from underlying net/ssh opts[:forward_agent] = (config_value(:ssh_forward_agent) === true) - opts[:connection_timeout] = config_value(:session_timeout).to_i + opts[:connection_timeout] = session_timeout opts end @@ -964,10 +963,10 @@ class Chef end if config_value(:ca_trust_file) - opts[:ca_trust_file] = config_value(:ca_trust_file) + opts[:ca_trust_path] = config_value(:ca_trust_file) end - opts[:operation_timeout] = config_value(:session_timeout).to_i + opts[:operation_timeout] = session_timeout opts end @@ -1052,6 +1051,14 @@ class Chef def incomplete_policyfile_options? (!!config[:policy_name] ^ config[:policy_group]) end + + # session_timeout option has a default that may not arrive, particularly if + # we're being invoked from a plugin that doesn't merge_config. + def session_timeout + timeout = config_value(:session_timeout) + return options[:session_timeout][:default] if timeout.nil? + timeout.to_i + end end end end diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb index 4bfd1011fa..5118442327 100644 --- a/spec/unit/knife/bootstrap_spec.rb +++ b/spec/unit/knife/bootstrap_spec.rb @@ -19,7 +19,6 @@ require "spec_helper" Chef::Knife::Bootstrap.load_deps -require "net/ssh" describe Chef::Knife::Bootstrap do let(:bootstrap_template) { nil } @@ -853,7 +852,7 @@ describe Chef::Knife::Bootstrap do let(:expected_result) do { logger: Chef::Log, # not configurable - ca_trust_file: "trust.me", + ca_trust_path: "trust.me", max_wait_until_ready: 9999, operation_timeout: 9999, ssl_peer_fingerprint: "ABCDEF", @@ -878,7 +877,7 @@ describe Chef::Knife::Bootstrap do let(:expected_result) do { logger: Chef::Log, # not configurable - ca_trust_file: "no trust", + ca_trust_path: "no trust", max_wait_until_ready: 9999, operation_timeout: 60, ssl_peer_fingerprint: "ABCDEF", @@ -933,7 +932,7 @@ describe Chef::Knife::Bootstrap do let(:expected_result) do { logger: Chef::Log, # not configurable - ca_trust_file: "trust.the.internet", + ca_trust_path: "trust.the.internet", max_wait_until_ready: 1000, operation_timeout: 1000, ssl_peer_fingerprint: "FEDCBA", @@ -1594,7 +1593,7 @@ describe Chef::Knife::Bootstrap do context "with ca_trust_file" do let(:ca_trust_expected) do - expected.merge({ ca_trust_file: "/trust.me" }) + expected.merge({ ca_trust_path: "/trust.me" }) end before do knife.config[:ca_trust_file] = "/trust.me" @@ -1806,6 +1805,14 @@ describe Chef::Knife::Bootstrap do end describe "#connect!" do + before do + # These are not required at run-time because train will handle its own + # protocol loading. In this case, we're simulating train failures and have to load + # them ourselves. + require "net/ssh" + require "train/transports/ssh" + end + context "in the normal case" do it "connects using the connection_opts and notifies the operator of progress" do expect(knife.ui).to receive(:info).with(/Connecting to.*/) @@ -1815,7 +1822,7 @@ describe Chef::Knife::Bootstrap do end end - context "when a non-auth-failure occurs" do + context "when a general non-auth-failure occurs" do let(:expected_error) { RuntimeError.new } before do allow(knife).to receive(:do_connect).and_raise(expected_error) @@ -1825,6 +1832,23 @@ describe Chef::Knife::Bootstrap do end end + context "when ssh fingerprint is invalid" do + let(:expected_error) { Train::Error.new("fingerprint AA:BB is unknown for \"blah,127.0.0.1\"") } + before do + allow(knife).to receive(:do_connect).and_raise(expected_error) + end + it "warns, prompts to accept, then connects with verify_host_key of accept_new" do + expect(knife).to receive(:do_connect).and_raise(expected_error) + expect(knife.ui).to receive(:confirm) + .with(/.*host 'blah \(127.0.0.1\)'.*AA:BB.*Are you sure you want to continue.*/m) + .and_return(true) + expect(knife).to receive(:do_connect) do |opts| + expect(opts[:verify_host_key]).to eq :accept_new + end + knife.connect! + end + end + context "when an auth failure occurs" do let(:expected_error) do e = Train::Error.new @@ -1835,10 +1859,6 @@ describe Chef::Knife::Bootstrap do e end - before do - require "net/ssh" - end - context "and password auth was used" do before do allow(connection).to receive(:password_auth?).and_return true @@ -2136,9 +2156,18 @@ describe Chef::Knife::Bootstrap do end describe "#warn_on_short_session_timeout" do - let(:session_timeout) { 0 } + let(:session_timeout) { 60 } + before do - allow(knife).to receive(:config).and_return(session_timeout: session_timeout) + allow(knife).to receive(:session_timeout).and_return(session_timeout) + end + + context "timeout is not set at all" do + let(:session_timeout) { nil } + it "does not issue a warning" do + expect(knife.ui).to_not receive(:warn) + knife.warn_on_short_session_timeout + end end context "timeout is more than 15" do |