(#550) intermediate certificate chains

For bonus effort, this will also test SANs by moving the validation
logic to openssl, instead of trying to parse the CN out of the subject.

Contains ruby 2.4/2.5 compat for OpenSSL::SSL::Context depreciating the
use of `extra_cert_chain` - the ruby shipped with puppet 5.X is behind
a revision.

lib/mcollective/security/choria.rb

@@ -392,7 +392,7 @@ def should_cache_certname?(pubcert, callerid)
       # @param pubcert [String] PEM encoded X509 public certificate
       # @return [Hash]
       def client_pubcert_metadata(envelope, pubcert)
-        cert = choria.parse_pubcert(pubcert)
+        cert = choria.parse_pubcert(pubcert)[0]
 
         {
           "create_time" => current_timestamp,

lib/mcollective/util/choria.rb

@@ -329,18 +329,19 @@ def have_ssl_files?(log=true)
       # Validates a certificate against the CA
       #
       # @param pubcert [String] PEM encoded X509 public certificate
+      # @param name [String] name that should be present in the certificate
       # @param log [Boolean] log warnings when true
       # @return [String,false] when succesful, the certname else false
       # @raise [StandardError] in case OpenSSL fails to open the various certificates
       # @raise [OpenSSL::X509::CertificateError] if the CA is invalid
-      def valid_certificate?(pubcert, log=true)
+      def valid_certificate?(pubcert, name, log=true)
         unless File.readable?(ca_path)
           raise("Cannot find or read the CA in %s, cannot verify public certificate" % ca_path)
         end
 
         incoming = parse_pubcert(pubcert, log)
 
-        return false unless incoming
+        return false unless incoming.any?
 
         begin
           ca = OpenSSL::X509::Store.new.add_file(ca_path)
@@ -349,27 +350,27 @@ def valid_certificate?(pubcert, log=true)
           raise
         end
 
-        unless ca.verify(incoming)
-          Log.warn("Failed to verify certificate %s against CA %s in %s" % [incoming.subject.to_s, incoming.issuer.to_s, ca_path]) if log
+        unless ca.verify(incoming[0], incoming)
+          Log.warn("Failed to verify certificate %s against CA %s in %s" % [incoming[0].subject.to_s, incoming[0].issuer.to_s, ca_path]) if log
           return false
         end
 
-        Log.debug("Verified certificate %s against CA %s" % [incoming.subject.to_s, incoming.issuer.to_s]) if log
+        Log.debug("Verified certificate %s against CA %s" % [incoming[0].subject.to_s, incoming[0].issuer.to_s]) if log
 
-        cn_parts = incoming.subject.to_a.select {|c| c[0] == "CN"}.flatten
-
-        raise("Could not parse certificate with subject %s as it has no CN part" % [incoming.subject.to_s]) if cn_parts.empty?
+        unless OpenSSL::SSL.verify_certificate_identity(incoming[0], name)
+          raise("Could not parse certificate with subject %s as it has no CN part, or name %s invalid" % [incoming[0].subject.to_s, name])
+        end
 
-        cn_parts[1]
+        name
       end
 
       # Parses a public cert
       #
       # @param pubcert [String] PEM encoded public certificate
       # @param log [Boolean] log warnings when true
-      # @return [OpenSSL::X509::Certificate,nil]
+      # @return [Array<OpenSSL::X509::Certificate,nil>]
       def parse_pubcert(pubcert, log=true)
-        OpenSSL::X509::Certificate.new(pubcert)
+        pubcert.scan(/(-----BEGIN CERTIFICATE-----.+?-----END CERTIFICATE-----)/m).map {|cstr| OpenSSL::X509::Certificate.new(cstr[0])}
       rescue OpenSSL::X509::CertificateError
         Log.warn("Received certificate is not a valid x509 certificate: %s: %s" % [$!.class, $!.to_s]) if log
         nil
@@ -390,7 +391,7 @@ def check_ssl_setup(log=true)
         embedded_certname = nil
 
         begin
-          embedded_certname = valid_certificate?(File.read(client_public_cert))
+          embedded_certname = valid_certificate?(File.read(client_public_cert), certname)
         rescue
           raise(UserError, "The public certificate was not signed by the configured CA")
         end
@@ -640,8 +641,24 @@ def puppet_setting(setting)
       def ssl_context
         context = OpenSSL::SSL::SSLContext.new
         context.ca_file = ca_path
-        context.cert = OpenSSL::X509::Certificate.new(File.read(client_public_cert))
-        context.key = OpenSSL::PKey::RSA.new(File.read(client_private_key))
+
+        cert = OpenSSL::X509::Certificate.new(File.read(client_public_cert))
+        key = OpenSSL::PKey::RSA.new(File.read(client_private_key))
+
+        extra_chain_cert = File.read(client_public_cert).lines.reject { |line| line[0] == "#" }
+                               .join("")
+                               .scan(/(-----BEGIN CERTIFICATE-----.+?-----END CERTIFICATE-----)/m)
+                               .drop(1)
+                               .map {|cstr| OpenSSL::X509::Certificate.new(cstr[0])}
+
+        if OpenSSL::SSL::SSLContext.method_defined? :add_certificate
+          context.add_certificate(cert, key, extra_chain_cert)
+        else
+          context.cert = OpenSSL::X509::Certificate.new(File.read(client_public_cert))
+          context.key = OpenSSL::PKey::RSA.new(File.read(client_private_key))
+          context.extra_chain_cert = extra_chain_cert
+        end
+
         context.verify_mode = OpenSSL::SSL::VERIFY_PEER
 
         context

spec/unit/mcollective/util/choria_spec.rb

@@ -158,21 +158,21 @@ module Util
           choria.expects(:ca_path).returns("/nonexisting").twice
 
           expect {
-            choria.valid_certificate?("x")
+            choria.valid_certificate?("x", "badhostname")
           }.to raise_error("Cannot find or read the CA in /nonexisting, cannot verify public certificate")
         end
 
         it "should fail for CA missmatches" do
           choria.stubs(:ca_path).returns("spec/fixtures/other_ca.pem")
-          expect(choria.valid_certificate?(File.read("spec/fixtures/rip.mcollective.pem"))).to be_falsey
+          expect(choria.valid_certificate?(File.read("spec/fixtures/rip.mcollective.pem"), "rip.mcollective")).to be_falsey
 
           choria.stubs(:ca_path).returns("spec/fixtures/ca_crt.pem")
-          expect(choria.valid_certificate?(File.read("spec/fixtures/other.mcollective.pem"))).to be_falsey
+          expect(choria.valid_certificate?(File.read("spec/fixtures/other.mcollective.pem"), "other.mcollective")).to be_falsey
         end
 
         it "should pass for valid cert/ca combos" do
           choria.stubs(:ca_path).returns("spec/fixtures/ca_crt.pem")
-          expect(choria.valid_certificate?(File.read("spec/fixtures/rip.mcollective.pem"))).to be_truthy
+          expect(choria.valid_certificate?(File.read("spec/fixtures/rip.mcollective.pem"), "rip.mcollective")).to be_truthy
         end
       end
 
@@ -846,7 +846,7 @@ module Util
         it "should fail if the cert is not signed by the CA" do
           choria.expects(:have_ssl_files?).returns(true)
           pub_cert = File.read(choria.client_public_cert)
-          choria.expects(:valid_certificate?).with(pub_cert).raises("rspec fail")
+          choria.expects(:valid_certificate?).with(pub_cert, choria.certname).raises("rspec fail")
           expect { choria.check_ssl_setup }.to raise_error("The public certificate was not signed by the configured CA")
         end