Merge pull request #732 from johnnyshields/2.x-cert-pkey-objects

[READY] v2.x - Allow SP certificates to be OpenSSL::X509::Certificate / private_key to be OpenSSL::PKey::PKey

CHANGELOG.md

@@ -14,6 +14,8 @@
 * [#715](https://github.com/SAML-Toolkits/ruby-saml/pull/715) Fix typo in error when SPNameQualifier value does not match the SP entityID.
 * [#718](https://github.com/SAML-Toolkits/ruby-saml/pull/718) Add support to retrieve from SAMLResponse the AuthnInstant and AuthnContextClassRef values
 * [#711](https://github.com/SAML-Toolkits/ruby-saml/pull/711) Standardize how RubySaml reads and formats certificate and private_key PEM values, including the `RubySaml::Util#format_cert` and `#format_private_key` methods.
+* [#732](https://github.com/SAML-Toolkits/ruby-saml/pull/732) Return original certificate and private key objects from `RubySaml::Utils` build functions.
+* [#732](https://github.com/SAML-Toolkits/ruby-saml/pull/732) Allow SP `certificate`, `certificate_new`, and `private_key` settings to be `OpenSSL::X509::Certificate` and `OpenSSL::PKey::PKey` objects respectively.
 * [#733](https://github.com/SAML-Toolkits/ruby-saml/pull/733) Allow `RubySaml::Utils.is_cert_expired` and `is_cert_active` to accept an optional time argument.
 * [#731](https://github.com/SAML-Toolkits/ruby-saml/pull/731) Add CI coverage for Ruby 3.4. Remove CI coverage for Ruby 1.x and 2.x.
 

lib/ruby_saml/settings.rb

@@ -363,6 +363,24 @@ def compress_deprecation(old_param, new_param)
                         '"HTTP-Redirect" will always be compressed, and "HTTP-POST" will always be uncompressed.'
     end
 
+    # Quick check if a certificate value is present as a string or OpenSSL::X509::Certificate.
+    # Does not check if the string can actually be parsed.
+    #
+    # @param cert [OpenSSL::X509::Certificate|String] The x509 certificate.
+    # @return [true|false] Whether the certificate value is present.
+    def cert?(cert)
+      !!cert && (cert.is_a?(OpenSSL::X509::Certificate) || !cert.empty?)
+    end
+
+    # Quick check if a private key value is present as a string or OpenSSL::PKey::PKey.
+    # Does not check if the string can actually be parsed.
+    #
+    # @param private_key [OpenSSL::PKey::PKey|String] The private key.
+    # @return [true|false] Whether the private key value is present.
+    def private_key?(private_key)
+      !!private_key && (private_key.is_a?(OpenSSL::PKey::PKey) || !private_key.empty?)
+    end
+
     # @return [Hash<Symbol, Array<Array<OpenSSL::X509::Certificate, OpenSSL::PKey::RSA>>>]
     #   Build the SP certificates and private keys from the settings. Returns all
     #   certificates and private keys, even if they are expired.
@@ -373,11 +391,8 @@ def get_all_sp_certs
 
     # Validate certificate, certificate_new, private_key, and sp_cert_multi params.
     def validate_sp_certs_params!
-      multi    = sp_cert_multi   && !sp_cert_multi.empty?
-      cert     = certificate     && !certificate.empty?
-      cert_new = certificate_new && !certificate_new.empty?
-      pk       = private_key     && !private_key.empty?
-      if multi && (cert || cert_new || pk)
+      has_multi = sp_cert_multi && !sp_cert_multi.empty?
+      if has_multi && (cert?(certificate) || cert?(certificate_new) || private_key?(private_key))
         raise ArgumentError.new("Cannot specify both sp_cert_multi and certificate, certificate_new, private_key parameters")
       end
     end

lib/ruby_saml/utils.rb

@@ -39,7 +39,7 @@ module Utils # rubocop:disable Metrics/ModuleLength
     # @return [true|false] Whether the certificate is expired.
     def is_cert_expired(cert, now = Time.now)
       now = Time.at(now) if now.is_a?(Integer)
-      cert = build_cert_object(cert) if cert.is_a?(String)
+      cert = build_cert_object(cert)
       cert.not_after < now
     end
 
@@ -50,7 +50,7 @@ def is_cert_expired(cert, now = Time.now)
     # @return [true|false] Whether the certificate is currently active.
     def is_cert_active(cert, now = Time.now)
       now = Time.at(now) if now.is_a?(Integer)
-      cert = build_cert_object(cert) if cert.is_a?(String)
+      cert = build_cert_object(cert)
       cert.not_before <= now && cert.not_after >= now
     end
 
@@ -119,6 +119,7 @@ def format_private_key(key, multi: false)
     # @param pem [String] The original certificate
     # @return [OpenSSL::X509::Certificate] The certificate object
     def build_cert_object(pem)
+      return pem if pem.is_a?(OpenSSL::X509::Certificate)
       return unless (pem = PemFormatter.format_cert(pem, multi: false))
 
       OpenSSL::X509::Certificate.new(pem)
@@ -129,6 +130,7 @@ def build_cert_object(pem)
     # @param pem [String] The original private key.
     # @return [OpenSSL::PKey::PKey] The private key object.
     def build_private_key_object(pem)
+      return pem if pem.is_a?(OpenSSL::PKey::PKey)
       return unless (pem = PemFormatter.format_private_key(pem, multi: false))
 
       error = nil

test/settings_test.rb

@@ -515,6 +515,78 @@ class SettingsTest < Minitest::Test
         end
       end
 
+      it 'handles OpenSSL::X509::Certificate objects for single case' do
+        @settings.certificate = OpenSSL::X509::Certificate.new(cert_text1)
+        @settings.private_key = key_text1
+
+        actual = @settings.get_sp_certs
+        expected = [[cert_text1, key_text1]]
+        assert_equal [:signing, :encryption], actual.keys
+        assert_equal expected, actual[:signing].map { |ary| ary.map(&:to_pem) }
+        assert_equal expected, actual[:encryption].map { |ary| ary.map(&:to_pem) }
+      end
+
+      it 'handles OpenSSL::X509::Certificate objects for single case with new cert' do
+        @settings.certificate = cert_text1
+        @settings.certificate_new = OpenSSL::X509::Certificate.new(cert_text2)
+        @settings.private_key = key_text1
+
+        actual = @settings.get_sp_certs
+        expected = [[cert_text1, key_text1], [cert_text2, key_text1]]
+        assert_equal [:signing, :encryption], actual.keys
+        assert_equal expected, actual[:signing].map { |ary| ary.map(&:to_pem) }
+        assert_equal expected, actual[:encryption].map { |ary| ary.map(&:to_pem) }
+      end
+
+      it 'handles OpenSSL::X509::Certificate objects for multi case' do
+        x509_certificate1 = OpenSSL::X509::Certificate.new(cert_text1)
+        x509_certificate2 = OpenSSL::X509::Certificate.new(cert_text2)
+        @settings.sp_cert_multi = {
+          signing: [{ certificate: x509_certificate1, private_key: key_text1 },
+                    { certificate: cert_text2, private_key: key_text1 }],
+          encryption: [{ certificate: x509_certificate2, private_key: key_text1 },
+                       { certificate: cert_text3, private_key: key_text2 }]
+        }
+
+        actual = @settings.get_sp_certs
+        expected_signing = [[cert_text1, key_text1], [cert_text2, key_text1]]
+        expected_encryption = [[cert_text2, key_text1], [cert_text3, key_text2]]
+        assert_equal [:signing, :encryption], actual.keys
+        assert_equal expected_signing, actual[:signing].map { |ary| ary.map(&:to_pem) }
+        assert_equal expected_encryption, actual[:encryption].map { |ary| ary.map(&:to_pem) }
+      end
+
+
+      it 'handles OpenSSL::PKey::PKey objects for single case' do
+        @settings.certificate = cert_text1
+        @settings.private_key = OpenSSL::PKey::RSA.new(key_text1)
+
+        actual = @settings.get_sp_certs
+        expected = [[cert_text1, key_text1]]
+        assert_equal [:signing, :encryption], actual.keys
+        assert_equal expected, actual[:signing].map { |ary| ary.map(&:to_pem) }
+        assert_equal expected, actual[:encryption].map { |ary| ary.map(&:to_pem) }
+      end
+
+      it 'handles OpenSSL::PKey::PKey objects for multi case' do
+        pkey2 = OpenSSL::PKey::RSA.new(key_text2)
+        pkey3 = CertificateHelper.generate_private_key(:dsa)
+        pkey4 = CertificateHelper.generate_private_key(:ecdsa)
+        @settings.sp_cert_multi = {
+          signing: [{ certificate: cert_text1, private_key: pkey3 },
+                    { certificate: cert_text2, private_key: pkey4 }],
+          encryption: [{ certificate: cert_text2, private_key: key_text1 },
+                       { certificate: cert_text3, private_key: pkey2 }]
+        }
+
+        actual = @settings.get_sp_certs
+        expected_signing = [[cert_text1, pkey3.to_pem], [cert_text2, pkey4.to_pem]]
+        expected_encryption = [[cert_text2, key_text1], [cert_text3, key_text2]]
+        assert_equal [:signing, :encryption], actual.keys
+        assert_equal expected_signing, actual[:signing].map { |ary| ary.map(&:to_pem) }
+        assert_equal expected_encryption, actual[:encryption].map { |ary| ary.map(&:to_pem) }
+      end
+
       it "sp_cert_multi allows sending only signing" do
         @settings.sp_cert_multi = {
           signing: [{ certificate: cert_text1, private_key: key_text1 },
@@ -920,5 +992,49 @@ class SettingsTest < Minitest::Test
         end
       end
     end
+
+    describe '#cert?' do
+      it 'returns true for a valid OpenSSL::X509::Certificate object' do
+        cert = CertificateHelper.generate_cert
+        assert @settings.send(:cert?, cert)
+      end
+
+      it 'returns true for a non-empty certificate string' do
+        cert_string = '-----BEGIN CERTIFICATE-----\nVALID_CERTIFICATE\n-----END CERTIFICATE-----'
+        assert @settings.send(:cert?, cert_string)
+      end
+
+      it 'returns false for an empty certificate string' do
+        cert_string = ''
+        refute @settings.send(:cert?, cert_string)
+      end
+
+      it 'returns false for nil' do
+        cert = nil
+        refute @settings.send(:cert?, cert)
+      end
+    end
+
+    describe '#private_key?' do
+      it 'returns true for a valid OpenSSL::PKey::PKey object' do
+        private_key = CertificateHelper.generate_private_key
+        assert @settings.send(:private_key?, private_key)
+      end
+
+      it 'returns true for a non-empty private key string' do
+        private_key_string = '-----BEGIN PRIVATE KEY-----\nVALID_PRIVATE_KEY\n-----END PRIVATE KEY-----'
+        assert @settings.send(:private_key?, private_key_string)
+      end
+
+      it 'returns false for an empty private key string' do
+        private_key_string = ''
+        refute @settings.send(:private_key?, private_key_string)
+      end
+
+      it 'returns false for nil' do
+        private_key = nil
+        refute @settings.send(:private_key?, private_key)
+      end
+    end
   end
 end

test/utils_test.rb

@@ -156,6 +156,11 @@ def result(duration, reference = 0)
       end
     end
 
+    it 'returns the original certificate when an OpenSSL::X509::Certificate is given' do
+      certificate = OpenSSL::X509::Certificate.new
+      assert_same certificate, RubySaml::Utils.build_cert_object(certificate)
+    end
+
     it 'returns nil for nil certificate string' do
       assert_nil RubySaml::Utils.build_cert_object(nil)
     end
@@ -180,6 +185,13 @@ def result(duration, reference = 0)
       end
     end
 
+    [OpenSSL::PKey::RSA, OpenSSL::PKey::DSA, OpenSSL::PKey::EC].each do |key_class|
+      it 'returns the original private key when an instance of OpenSSL::PKey::PKey is given' do
+        private_key = key_class.new
+        assert_same private_key, RubySaml::Utils.build_private_key_object(private_key)
+      end
+    end
+
     it 'returns nil for nil private key string' do
       assert_nil RubySaml::Utils.build_private_key_object(nil)
     end