Merge remote-tracking branch 'upstream/5.5.x' into 6.0.x

* upstream/5.5.x:
  (maint) Remove unnecessary condition
  (PUP-9357) Redact checks if sensitive
  (packaging) Updating the puppet.pot file
  (maint) Enable windows tests
  (PUP-8213) Display correct message when certname is mismatched
  (packaging) Updating the puppet.pot file
  (PUP-9076) Do not push nil on server context
  (maint) modify test to regex, rather than exact
  (maint) Clarify error on Timeout::Error exception

Conflicts:
	lib/puppet/configurer.rb
	lib/puppet/network/http/connection.rb
	locales/puppet.pot
	spec/unit/configurer_spec.rb
	spec/unit/network/http/connection_spec.rb

In 5.5.x, if the connection rescues an SSLError, then it calls
`verify_certificate_identity` first before checking for validation errors. In
6.0.x, that logic was moved to Puppet::Util::SSL.handle_connection_error, so
apply the same change there.

About configurer, enable tests that were previously skipped on Windows, but
continue skipping on JRuby. If we exhaust the server list, then it is now a hard
error, so update conflicted tests.

Author: joshcooper

lib/puppet/configurer.rb

@@ -226,13 +226,13 @@ def run(options = {})
         # mode. We shouldn't try to do any failover in that case.
         if options[:catalog].nil? && do_failover
           server, port = find_functional_server
+          if server.nil?
+            raise Puppet::Error, _("Could not select a functional puppet master from server_list: '%{server_list}'") % { server_list: Puppet[:server_list] }
+          else
+            Puppet.debug _("Selected puppet server: %{server}:%{port}") % { server: server, port: port }
+            report.master_used = "#{server}:#{port}"
+          end
           Puppet.override(server: server, serverport: port) do
-            if server
-              Puppet.debug _("Selected puppet server: %{server}:%{port}") % { server: server, port: port }
-              report.master_used = "#{server}:#{port}"
-            else
-              Puppet.warning _("Could not select a functional puppet server")
-            end
             completed = run_internal(options)
           end
         else

lib/puppet/network/http/connection.rb

@@ -298,12 +298,22 @@ def apply_options_to(request, options)
     def execute_request(connection, request)
       start = Time.now
       connection.request(request)
-    rescue EOFError => e
+    rescue => exception
       elapsed = (Time.now - start).to_f.round(3)
-      uri = @site.addr + request.path.split('?')[0]
-      eof = EOFError.new(_('request %{uri} interrupted after %{elapsed} seconds') % {uri: uri, elapsed: elapsed})
-      eof.set_backtrace(e.backtrace) unless e.backtrace.empty?
-      raise eof
+      uri = [@site.addr, request.path.split('?')[0]].join('/')
+      eclass = exception.class
+
+      err = case exception
+            when EOFError
+              eclass.new(_('request %{uri} interrupted after %{elapsed} seconds') % {uri: uri, elapsed: elapsed})
+            when Timeout::Error
+              eclass.new(_('request %{uri} timed out after %{elapsed} seconds') % {uri: uri, elapsed: elapsed})
+            else
+              eclass.new(_('request %{uri} failed: %{msg}') % {uri: uri, msg: exception.message})
+            end
+
+      err.set_backtrace(exception.backtrace) unless exception.backtrace.empty?
+      raise err
     end
 
     def with_connection(site, &block)

lib/puppet/type/exec.rb

@@ -622,18 +622,26 @@ def current_username
 
     private
     def set_sensitive_parameters(sensitive_parameters)
-      # Respect sensitive commands
-      set_one_sensitive_parameter(:command, sensitive_parameters)
-      set_one_sensitive_parameter(:unless, sensitive_parameters)
-      set_one_sensitive_parameter(:onlyif, sensitive_parameters)
-      super(sensitive_parameters)
-    end
+      # If any are sensitive, mark all as sensitive
+      sensitive = false
+      parameters_to_check = [:command, :unless, :onlyif]
+
+      parameters_to_check.each do |p| 
+        if sensitive_parameters.include?(p)
+          sensitive_parameters.delete(p)
+          sensitive = true
+        end
+      end
 
-    def set_one_sensitive_parameter(parameter, sensitive_parameters)
-      if sensitive_parameters.include?(parameter)
-        sensitive_parameters.delete(parameter)
-        parameter(parameter).sensitive = true
+      if sensitive
+        parameters_to_check.each do |p|
+          if parameters.include?(p)
+            parameter(p).sensitive = true
+          end
+        end
       end
+
+      super(sensitive_parameters)
     end
   end
 end

lib/puppet/util/ssl.rb

@@ -70,11 +70,7 @@ def self.handle_connection_error(error, verifier, host)
     # can be nil
     peer_cert = verifier.peer_certs.last
 
-    if error.message.include? "certificate verify failed"
-      msg = error.message
-      msg << ": [" + verifier.verify_errors.join('; ') + "]"
-      raise Puppet::Error, msg, error.backtrace
-    elsif peer_cert && !OpenSSL::SSL.verify_certificate_identity(peer_cert, host)
+    if peer_cert && !OpenSSL::SSL.verify_certificate_identity(peer_cert, host)
       valid_certnames = [peer_cert.subject.to_s.sub(/.*=/, ''),
                          *Puppet::SSL::Certificate.subject_alt_names_for(peer_cert)].uniq
       if valid_certnames.size > 1
@@ -85,6 +81,10 @@ def self.handle_connection_error(error, verifier, host)
 
       msg = _("Server hostname '%{host}' did not match server certificate; %{expected_certnames}") % { host: host, expected_certnames: expected_certnames }
       raise Puppet::Error, msg, error.backtrace
+    elsif !verifier.verify_errors.empty?
+      msg = error.message
+      msg << ": [" + verifier.verify_errors.join('; ') + "]"
+      raise Puppet::Error, msg, error.backtrace
     else
       raise error
     end

spec/unit/configurer_spec.rb

@@ -1034,26 +1034,25 @@ def expects_neither_new_or_cached_catalog
       Puppet.settings[:server_list] = ["myserver:123"]
       response = Net::HTTPInternalServerError.new(nil, 500, 'Internal Server Error')
       Puppet::Network::HttpPool.stubs(:http_ssl_instance).with('myserver', '123').returns(mock('request', get: response))
-      @agent.stubs(:run_internal)
 
       Puppet.expects(:debug).with("Puppet server myserver:123 is unavailable: 500 Internal Server Error")
-      @agent.run
+      expect{ @agent.run }.to raise_error(Puppet::Error, /Could not select a functional puppet master from server_list/)
     end
 
-    it "should fallback to an empty server when failover fails" do
+    it "should error when no servers in 'server_list' are reachable" do
       Puppet.settings[:server_list] = ["myserver:123"]
       error = Net::HTTPError.new(400, 'dummy server communication error')
-      Puppet::Network::HttpPool.stubs(:http_ssl_instance).with('myserver', '123').returns(error)
-      @agent.stubs(:run_internal)
+      Puppet::Network::HttpPool.stubs(:http_ssl_instance).with('myserver', '123').returns(mock('request', get: error))
 
       options = {}
-      @agent.run(options)
+      expect{ @agent.run(options) }.to raise_error(Puppet::Error, /Could not select a functional puppet master from server_list/)
       expect(options[:report].master_used).to be_nil
     end
 
     it "should not make multiple node requets when the server is found" do
       Puppet.settings[:server_list] = ["myserver:123"]
-      Puppet::Network::HttpPool.expects(:http_ssl_instance).once
+      response = Net::HTTPOK.new(nil, 200, 'OK')
+      Puppet::Network::HttpPool.stubs(:http_ssl_instance).with('myserver', '123').returns(mock('request', get: response))
       @agent.stubs(:run_internal)
 
       @agent.run

spec/unit/network/http/connection_spec.rb

@@ -78,27 +78,43 @@
       WebMock.enable!
     end
 
-    it "should provide a useful error message when one is available and certificate validation fails", :unless => Puppet::Util::Platform.windows? do
+    it "should provide a useful error message when one is available and certificate validation fails in ruby 2.4 and up" do
       connection = Puppet::Network::HTTP::Connection.new(
         host, port,
         :verify => ConstantErrorValidator.new(:fails_with => 'certificate verify failed',
                                               :error_string => 'shady looking signature'))
 
       expect do
         connection.get('request')
-      end.to raise_error(Puppet::Error, "certificate verify failed: [shady looking signature]")
+      end.to raise_error(Puppet::Error, /certificate verify failed: \[shady looking signature\]/)
     end
 
-    it "should provide a helpful error message when hostname was not match with server certificate", :unless => Puppet::Util::Platform.windows? || RUBY_PLATFORM == 'java' do
+    it "should provide a helpful error message when hostname does not match server certificate before ruby 2.4", :unless => RUBY_PLATFORM == 'java' do
       Puppet[:confdir] = tmpdir('conf')
 
       connection = Puppet::Network::HTTP::Connection.new(
       host, port,
       :verify => ConstantErrorValidator.new(
-        :fails_with => 'hostname was not match with server certificate',
+        :fails_with => "hostname 'myserver' does not match the server certificate",
         :peer_certs => [Puppet::TestCa.new.generate('not_my_server',
                                                     :subject_alt_names => 'DNS:foo,DNS:bar,DNS:baz,DNS:not_my_server')[:cert]]))
+      expect do
+        connection.get('request')
+      end.to raise_error(Puppet::Error) do |error|
+        error.message =~ /\AServer hostname 'my_server' did not match server certificate; expected one of (.+)/
+        expect($1.split(', ')).to match_array(%w[DNS:foo DNS:bar DNS:baz DNS:not_my_server not_my_server])
+      end
+    end
+
+    it "should provide a helpful error message when hostname does not match server certificate in ruby 2.4 or greater" do
+      Puppet[:confdir] = tmpdir('conf')
 
+      connection = Puppet::Network::HTTP::Connection.new(
+        host, port,
+        :verify => ConstantErrorValidator.new(
+          :fails_with => "certificate verify failed",
+          :peer_certs => [Puppet::TestCa.new.generate('not_my_server',
+                                                      :subject_alt_names => 'DNS:foo,DNS:bar,DNS:baz,DNS:not_my_server')[:cert]]))
       expect do
         connection.get('request')
       end.to raise_error(Puppet::Error) do |error|
@@ -117,7 +133,7 @@
       end.to raise_error(/some other message/)
     end
 
-    it "should check all peer certificates for upcoming expiration", :unless => Puppet::Util::Platform.windows? || RUBY_PLATFORM == 'java' do
+    it "should check all peer certificates for upcoming expiration", :unless => RUBY_PLATFORM == 'java' do
       Puppet[:confdir] = tmpdir('conf')
       cert = Puppet::TestCa.new.generate('server',
                                          :subject_alt_names => 'DNS:foo,DNS:bar,DNS:baz,DNS:server')[:cert]

spec/unit/type/exec_spec.rb

@@ -46,10 +46,10 @@ def exec_stub(options = {})
     dummy = Puppet::Type.type(:exec).new(:name => @command)
     dummy.provider.class.any_instance.stubs(:validatecmd)
     dummy.provider.class.any_instance.stubs(:checkexe).returns(true)
-    pass_status = stub('status', :exitstatus => 0)
-    fail_status = stub('status', :exitstatus => 1)
-    dummy.provider.class.any_instance.stubs(:run).with(:true, true).returns(['test output pass', pass_status])
-    dummy.provider.class.any_instance.stubs(:run).with(:false, true).returns(['test output fail', fail_status])
+    pass_status = stub('status', :exitstatus => 0, :split => ["pass output"])
+    fail_status = stub('status', :exitstatus => 1, :split => ["fail output"])
+    Puppet::Util::Execution.stubs(:execute).with(:true, anything).returns(pass_status)
+    Puppet::Util::Execution.stubs(:execute).with(:false, anything).returns(fail_status)
 
     test = Puppet::Type.type(:exec).new(type_args)
 
@@ -212,12 +212,13 @@ def exec_stub(options = {})
           expect(@logs).to include(an_object_having_attributes(level: :debug, message: output))
         end
 
-        it "should log a message with a redacted command if command or #{check} is sensitive" do
-          output = "'[command redacted]' won't be executed because of failed check '#{check}'"
-          test = exec_stub({:command => @command, check => result, :sensitive_parameters => [:command, check]})
+        it "should log a message with a redacted command and check if #{check} is sensitive" do
+          output1 = "Executing check '[redacted]'"
+          output2 = "'[command redacted]' won't be executed because of failed check '#{check}'"
+          test = exec_stub({:command => @command, check => result, :sensitive_parameters => [check]})
           expect(test.check_all_attributes).to eq(false)
-
-          expect(@logs).to include(an_object_having_attributes(level: :debug, message: output))
+          expect(@logs).to include(an_object_having_attributes(level: :debug, message: output1))
+          expect(@logs).to include(an_object_having_attributes(level: :debug, message: output2))
         end
       end
     end