(PUP-8213) Display correct message when certname is mismatched

Previously, if the server's cert did not match the hostname we tried to connect
to, puppet would output a confusing message:

    certificate verify failed: [ok for /CN=XXX]

This occurs because ruby 2.4 introduced a new security feature whereby the cert
is automatically verified during the call to `SSLSocket#connect`[1]. In earlier
ruby versions, the application had to call `SSLSocket#post_connection_check`,
but of course, many people forgot to, or didn't know they had to, leading to
MITM vulnerabilities.

However, when a mismatch occurs, ruby 2.4 invokes our `verify_callback` with
`preverify_ok=false`, but `store_context.error=0` which is `OpenSSL::SSL::V_OK`.
Ruby then raises an `SSLError` whose message is 'certificate verify failed',
which matches the first "if" statement in our error handler.

This commit changes the order so that if an SSLError is rescued, we
check to see if there's a host mismatch first. If not, we check if there
were *any* verify errors, or raise the original error.

This change is compatible with ruby versions prior to 2.4, because both
`SSLSocket#post_connection_check` and our error handler use
`OpenSSL::SSL.verify_certificate_identity` to detect the certname mismatch.

[1] ruby/openssl#60

Author: joshcooper

lib/puppet/network/http/connection.rb

@@ -318,11 +318,7 @@ def with_connection(site, &block)
       # can be nil
       peer_cert = @verify.peer_certs.last
 
-      if error.message.include? "certificate verify failed"
-        msg = error.message
-        msg << ": [" + @verify.verify_errors.join('; ') + "]"
-        raise Puppet::Error, msg, error.backtrace
-      elsif peer_cert && !OpenSSL::SSL.verify_certificate_identity(peer_cert.content, site.host)
+      if peer_cert && !OpenSSL::SSL.verify_certificate_identity(peer_cert.content, site.host)
         valid_certnames = [peer_cert.name, *peer_cert.subject_alt_names].uniq
         if valid_certnames.size > 1
           expected_certnames = _("expected one of %{certnames}") % { certnames: valid_certnames.join(', ') }
@@ -332,6 +328,10 @@ def with_connection(site, &block)
 
         msg = _("Server hostname '%{host}' did not match server certificate; %{expected_certnames}") % { host: site.host, expected_certnames: expected_certnames }
         raise Puppet::Error, msg, error.backtrace
+      elsif !@verify.verify_errors.empty?
+        msg = error.message
+        msg << ": [" + @verify.verify_errors.join('; ') + "]"
+        raise Puppet::Error, msg, error.backtrace
       else
         raise
       end

spec/unit/network/http/connection_spec.rb

@@ -113,7 +113,7 @@ def verify_errors
       WebMock.enable!
     end
 
-    it "should provide a useful error message when one is available and certificate validation fails", :unless => Puppet.features.microsoft_windows? do
+    it "should provide a useful error message when one is available and certificate validation fails in ruby 2.4 and up", :unless => Puppet.features.microsoft_windows? do
       connection = Puppet::Network::HTTP::Connection.new(
         host, port,
         :verify => ConstantErrorValidator.new(:fails_with => 'certificate verify failed',
@@ -124,13 +124,13 @@ def verify_errors
       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.features.microsoft_windows? do
+    it "should provide a helpful error message when hostname does not match server certificate before ruby 2.4", :unless => Puppet.features.microsoft_windows? 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::SSL::CertificateAuthority.new.generate(
           'not_my_server', :dns_alt_names => 'foo,bar,baz')]))
 
@@ -142,6 +142,42 @@ def verify_errors
       end
     end
 
+    it "should provide a helpful error message when hostname does not match server certificate in ruby 2.4 or greater", :unless => Puppet.features.microsoft_windows? do
+      Puppet[:confdir] = tmpdir('conf')
+
+      connection = Puppet::Network::HTTP::Connection.new(
+        host, port,
+        :verify => ConstantErrorValidator.new(
+          :fails_with => "certificate verify failed",
+          :peer_certs => [Puppet::SSL::CertificateAuthority.new.generate(
+                            'not_my_server', :dns_alt_names => 'foo,bar,baz')]))
+
+      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 was not match with server certificate", :unless => Puppet.features.microsoft_windows? 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',
+          :peer_certs => [Puppet::SSL::CertificateAuthority.new.generate(
+                            'not_my_server', :dns_alt_names => 'foo,bar,baz')]))
+
+      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 pass along the error message otherwise" do
       connection = Puppet::Network::HTTP::Connection.new(
         host, port,