(#7224) Reword 'hostname was not match' error message

This error message is grammatically incorrect and unhelpful, so we replace it
with a message that explains more correctly what went wrong and what was
expected. This message happens when making an authenticated connection to a
server where the certificate doesn't match its hostname. This happens in the
REST terminuses, so we wrap their HTTP methods with a helper that will catch
the appropriate SSLError and re-raise it with the better message stating the
hostname used, and the list of hostnames that we were expecting it to be a part
of.

Unfortunately, because the certificate in question isn't available at error
time, we have to use the Net::HTTP#verify_callback to capture it.

Paired-With: Jacob Helwig <jacob@puppetlabs.com>
Reviewed-By: Dominic Maraglia <dominic@puppetlabs.com>

5 files changed, 97 insertions, 14 deletions

diff --git a/acceptance/tests/helpful_error_message_when_hostname_not_match_server_certificate.rb b/acceptance/tests/helpful_error_message_when_hostname_not_match_server_certificate.rb
new file mode 100644
index 000000000..c3b5b6795
--- /dev/null
+++ b/acceptance/tests/helpful_error_message_when_hostname_not_match_server_certificate.rb
@@ -0,0 +1,12 @@
+test_name "generate a helpful error message when hostname doesn't match server certificate"
+
+step "Clear any existing SSL directories"
+on(hosts, "rm -r #{config['puppetpath']}/ssl")
+
+# Start the master with a certname not matching its hostname
+with_master_running_on(master, "--certname foobar_not_my_hostname --certdnsnames one_cert:two_cert:red_cert:blue_cert --autosign true") do
+  run_agent_on(agents, "--no-daemonize --verbose --onetime --server #{master}", :acceptable_exit_codes => (1..255)) do
+    msg = "Server hostname '#{master}' did not match server certificate; expected one of foobar_not_my_hostname, one_cert, two_cert, red_cert, blue_cert"
+    assert_match(msg, stdout)
+  end
+end
diff --git a/acceptance/tests/ticket_3360_allow_duplicate_csr_with_option_set.rb b/acceptance/tests/ticket_3360_allow_duplicate_csr_with_option_set.rb
index 9eaf4c224..a34a3e718 100644
--- a/acceptance/tests/ticket_3360_allow_duplicate_csr_with_option_set.rb
+++ b/acceptance/tests/ticket_3360_allow_duplicate_csr_with_option_set.rb
@@ -2,8 +2,8 @@ test_name "#3360: Allow duplicate CSR when allow_duplicate_certs is on"
 
 agent_hostnames = agents.map {|a| a.to_s}
 
-step "Remove existing SSL directory for agents"
-on agents, "rm -r #{config['puppetpath']}/ssl"
+step "Remove existing SSL directory for hosts"
+on hosts, "rm -r #{config['puppetpath']}/ssl"
 
 with_master_running_on master, "--allow_duplicate_certs --certdnsnames=\"puppet:$(hostname -s):$(hostname -f)\" --verbose --noop" do
   step "Generate a certificate request for the agent"
diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index 0d3997221..8018fe8e3 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -71,16 +71,49 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
     Puppet::Network::HttpPool.http_instance(request.server || self.class.server, request.port || self.class.port)
   end
 
+  [:get, :post, :head, :delete, :put].each do |method|
+    define_method "http_#{method}" do |request, *args|
+      http_request(method, request, *args)
+    end
+  end
+
+  def http_request(method, request, *args)
+    http_connection = network(request)
+    peer_certs = []
+
+    # We add the callback to collect the certificates for use in constructing
+    # the error message if the verification failed.  This is necessary since we
+    # don't have direct access to the cert that we expected the connection to
+    # use otherwise.
+    #
+    http_connection.verify_callback = proc do |preverify_ok, ssl_context|
+      peer_certs << Puppet::SSL::Certificate.from_s(ssl_context.current_cert.to_pem)
+      preverify_ok
+    end
+
+    http_connection.send(method, *args)
+  rescue OpenSSL::SSL::SSLError => error
+    if error.message.include? "hostname was not match"
+      raise unless cert = peer_certs.find { |c| c.name !~ /^puppet ca/i }
+
+      valid_certnames = [cert.name, *cert.alternate_names].uniq
+      msg = valid_certnames.length > 1 ? "one of #{valid_certnames.join(', ')}" : valid_certnames.first
+
+      raise Puppet::Error, "Server hostname '#{http_connection.address}' did not match server certificate; expected #{msg}"
+    else
+      raise
+    end
+  end
+
   def find(request)
     uri, body = request_to_uri_and_body(request)
     uri_with_query_string = "#{uri}?#{body}"
-    http_connection = network(request)
     # WEBrick in Ruby 1.9.1 only supports up to 1024 character lines in an HTTP request
     # http://redmine.ruby-lang.org/issues/show/3991
     response = if "GET #{uri_with_query_string} HTTP/1.1\r\n".length > 1024
-      http_connection.post(uri, body, headers)
+      http_post(request, uri, body, headers)
     else
-      http_connection.get(uri_with_query_string, headers)
+      http_get(request, uri_with_query_string, headers)
     end
     result = deserialize response
     result.name = request.key if result.respond_to?(:name=)
@@ -88,7 +121,7 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
   end
 
   def head(request)
-    response = network(request).head(indirection2uri(request), headers)
+    response = http_head(request, indirection2uri(request), headers)
     case response.code
     when "404"
       return false
@@ -101,7 +134,7 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
   end
 
   def search(request)
-    unless result = deserialize(network(request).get(indirection2uri(request), headers), true)
+    unless result = deserialize(http_get(request, indirection2uri(request), headers), true)
       return []
     end
     result
@@ -109,12 +142,12 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
 
   def destroy(request)
     raise ArgumentError, "DELETE does not accept options" unless request.options.empty?
-    deserialize network(request).delete(indirection2uri(request), headers)
+    deserialize http_delete(request, indirection2uri(request), headers)
   end
 
   def save(request)
     raise ArgumentError, "PUT does not accept options" unless request.options.empty?
-    deserialize network(request).put(indirection2uri(request), request.instance.render, headers.merge({ "Content-Type" => request.instance.mime }))
+    deserialize http_put(request, indirection2uri(request), request.instance.render, headers.merge({ "Content-Type" => request.instance.mime }))
   end
 
   private
diff --git a/spec/unit/indirector/certificate/rest_spec.rb b/spec/unit/indirector/certificate/rest_spec.rb
index 21e10e316..870d9e437 100755
--- a/spec/unit/indirector/certificate/rest_spec.rb
+++ b/spec/unit/indirector/certificate/rest_spec.rb
@@ -47,6 +47,7 @@ rn/G
     response = stub 'response', :code => "200", :body => cert_string
     response.stubs(:[]).with('content-type').returns "text/plain"
     response.stubs(:[]).with('content-encoding')
+    network.stubs(:verify_callback=)
     network.expects(:get).returns response
 
     request = Puppet::Indirector::Request.new(:certificate, :find, "foo.com")
diff --git a/spec/unit/indirector/rest_spec.rb b/spec/unit/indirector/rest_spec.rb
index 5637080e8..ee0111a77 100755
--- a/spec/unit/indirector/rest_spec.rb
+++ b/spec/unit/indirector/rest_spec.rb
@@ -90,6 +90,43 @@ describe Puppet::Indirector::REST do
     @rest_class.port.should == 543
   end
 
+	describe "when making http requests" do
+		it "should provide a helpful error message when hostname was not match with server certificate" do
+			Puppet[:certdnsnames] = 'foo:bar:baz'
+			csr            = OpenSSL::X509::Request.new
+			csr.subject    = OpenSSL::X509::Name.new([['CN', 'not_my_server']])
+			csr.public_key = OpenSSL::PKey::RSA.generate(Puppet[:keylength]).public_key
+			cert = Puppet::SSL::CertificateFactory.new('server', csr, csr, 14).result
+
+			connection = Net::HTTP.new('my_server', 8140)
+			@searcher.stubs(:network).returns(connection)
+			ssl_context = OpenSSL::SSL::SSLContext.new
+			ssl_context.stubs(:current_cert).returns(cert)
+			connection.stubs(:get).with do
+				connection.verify_callback.call(true, ssl_context)
+			end.raises(OpenSSL::SSL::SSLError.new('hostname was not match with server certificate'))
+
+			msg = /Server hostname 'my_server' did not match server certificate; expected one of (.+)/
+			expect { @searcher.http_request(:get, stub('request')) }.to(
+				raise_error(Puppet::Error, msg) do |error|
+					error.message =~ msg
+					$1.split(', ').should =~ ['foo', 'bar', 'baz', 'not_my_server']
+				end
+			)
+		end
+
+		it "should pass along the error message otherwise" do
+			connection = Net::HTTP.new('my_server', 8140)
+			@searcher.stubs(:network).returns(connection)
+
+			connection.stubs(:get).raises(OpenSSL::SSL::SSLError.new('certificate verify failed'))
+
+			expect do
+				@searcher.http_request(:get, stub('request'))
+			end.to raise_error(/certificate verify failed/)
+		end
+	end
+
   describe "when deserializing responses" do
     it "should return nil if the response code is 404" do
       response = mock 'response'
@@ -219,7 +256,7 @@ describe Puppet::Indirector::REST do
 
   describe "when doing a find" do
     before :each do
-      @connection = stub('mock http connection', :get => @response)
+      @connection = stub('mock http connection', :get => @response, :verify_callback= => nil)
       @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
       # Use a key with spaces, so we can test escaping
@@ -313,7 +350,7 @@ describe Puppet::Indirector::REST do
 
   describe "when doing a head" do
     before :each do
-      @connection = stub('mock http connection', :head => @response)
+      @connection = stub('mock http connection', :head => @response, :verify_callback= => nil)
       @searcher.stubs(:network).returns(@connection)
 
       # Use a key with spaces, so we can test escaping
@@ -349,7 +386,7 @@ describe Puppet::Indirector::REST do
 
   describe "when doing a search" do
     before :each do
-      @connection = stub('mock http connection', :get => @response)
+      @connection = stub('mock http connection', :get => @response, :verify_callback= => nil)
       @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
       @model.stubs(:convert_from_multiple)
@@ -397,7 +434,7 @@ describe Puppet::Indirector::REST do
 
   describe "when doing a destroy" do
     before :each do
-      @connection = stub('mock http connection', :delete => @response)
+      @connection = stub('mock http connection', :delete => @response, :verify_callback= => nil)
       @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
       @request = Puppet::Indirector::Request.new(:foo, :destroy, "foo bar")
@@ -453,7 +490,7 @@ describe Puppet::Indirector::REST do
 
   describe "when doing a save" do
     before :each do
-      @connection = stub('mock http connection', :put => @response)
+      @connection = stub('mock http connection', :put => @response, :verify_callback= => nil)
       @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
       @instance = stub 'instance', :render => "mydata", :mime => "mime"