Improvements

CHANGELOG.md

@@ -11,6 +11,7 @@
 * [#690](https://github.com/SAML-Toolkits/ruby-saml/pull/690) Remove deprecated `settings.security[:embed_sign]` parameter.
 * [#697](https://github.com/SAML-Toolkits/ruby-saml/pull/697) Add deprecation for various parameters in `RubySaml::Settings`.
 * [#709](https://github.com/SAML-Toolkits/ruby-saml/pull/709) Allow passing in `Net::HTTP` `:open_timeout`, `:read_timeout`, and `:max_retries` settings to `IdpMetadataParser#parse_remote`.
+* [#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.
 
 ### 1.17.0
 * [#687](https://github.com/SAML-Toolkits/ruby-saml/pull/687) Add CI coverage for Ruby 3.3 and Windows.

UPGRADING.md

@@ -50,7 +50,7 @@ settings.security[:digest_method] = RubySaml::XML::Document::SHA1
 settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1
 ```
 
-### Removal of embed_sign Setting
+### Removal of embed_sign setting
 
 The deprecated `settings.security[:embed_sign]` parameter has been removed. If you were using it, please instead switch
 to using both the `settings.idp_sso_service_binding` and `settings.idp_slo_service_binding` parameters as show below.
@@ -68,7 +68,7 @@ settings.idp_slo_service_binding = :redirect
 
 For clarity, the default value of both parameters is `:redirect` if they are not set.
 
-### Deprecation of Compression Settings
+### Deprecation of compression settings
 
 The `settings.compress_request` and `settings.compress_response` parameters have been deprecated
 and are no longer functional. They will be removed in RubySaml 2.1.0. Please remove `compress_request`
@@ -80,7 +80,7 @@ The SAML SP request/response message compression behavior is now controlled auto
 "compression" is used to make redirect URLs which contain SAML messages be shorter. For POST messages,
 compression may be achieved by enabling `Content-Encoding: gzip` on your webserver.
 
-## Settings deprecations
+### Other settings deprecations
 
 The following parameters in `RubySaml::Settings` are deprecated and will be removed in RubySaml 2.1.0:
 
@@ -92,6 +92,34 @@ The following parameters in `RubySaml::Settings` are deprecated and will be remo
 - `#certificate_new` is deprecated and replaced by `#sp_cert_multi`. Refer to documentation as `#sp_cert_multi`
   has a different value type than `#certificate_new`.
 
+### Minor changes to Util#format_cert and #format_private_key
+
+Version 2.0.0 standardizes how RubySaml reads and formats certificate and private key
+PEM strings. In general, version 2.0.0 is more permissive than 1.x, and the changes
+are not anticipated to affect most users. Please note the change affects parameters
+such `#idp_cert` and `#certificate`, as well as the `RubySaml::Util#format_cert`
+and `#format_private_key` methods. Specifically:
+
+| # | Case                                                 | RubySaml 2.0.0                                          | RubySaml 1.x                     |
+|---|------------------------------------------------------|---------------------------------------------------------|----------------------------------|
+| 1 | Input contains a bad (e.g. non-base64) PEM           | Skip PEM formatting                                     | Attempt to format as a (bad) PEM |
+| 2 | Input contains `\r` character(s)                     | Strip out all `\r` character(s) and format as PEM       | Skip PEM formatting              |
+| 3 | PEM header other than `CERTIFICATE` or `PRIVATE KEY` | Format if header ends in `CERTIFICATE` or `PRIVATE KEY` | Skip PEM formatting              |
+| 4 | Text outside header/footer values                    | Strip out text outside header/footer values             | Skip PEM formatting              |
+| 5 | Input non-ASCII characters                           | Ignore non-ASCII chars if they are outside the PEM      | Skip PEM formatting              |
+| 6 | `#format_cert` input contains mix of good/bad certs  | Return only good certs joined with `\n`                 | Return good and bad certs        |
+
+**Notes**
+- Case 3: For example, `-----BEGIN TRUSTED X509 CERTIFICATE-----` is now
+  considered a valid header as an input, but it will be formatted to
+  `-----BEGIN CERTIFICATE-----` in the output. As a special case, in both 2.0.0
+  and 1.x, if `RSA PRIVATE KEY` is present in the input string, the `RSA` prefix will
+  be preserved in the output.
+- Case 4: When formatting multiple certificates in one string (i.e. a certificate chain),
+  text present between the footer and header of two different certificates will also be
+  stripped out.
+- Case 5: If no valid certificates are found, the entire original string will be returned.
+
 ## Updating from 1.12.x to 1.13.0
 
 Version `1.13.0` adds `settings.idp_sso_service_binding` and `settings.idp_slo_service_binding`, and

lib/ruby_saml/pem_formatter.rb

@@ -1,68 +1,103 @@
 # frozen_string_literal: true
 
 module RubySaml
-  # Formats PEM-encoded X.509 certificates and private keys to
-  # canonical PEM format with 64-char lines and BEGIN/END headers.
+  # Formats PEM-encoded X.509 certificates and private keys to canonical
+  # RFC 7468 PEM format, including 64-char lines and BEGIN/END headers.
   #
   # @api private
   module PemFormatter
     extend self
 
-    # Formats one or many X.509 certificate(s) to canonical
-    # PEM format with 64-char lines and BEGIN/END headers.
+    PEM_SCAN_BASE64 = '[A-Za-z\d+/\s]+=?\s*=?\s*'
+
+    # Formats one or multiple X.509 certificate(s) to an array of strings
+    # in canonical RFC 7468 PEM format.
+    #
+    # @param certs [String] A string containing unformatted certificate(s).
+    # @return [Array<String>] The formatted certificate(s).
+    def format_cert_array(certs)
+      format_pem_array(certs, 'CERTIFICATE')
+    end
+
+    # Formats one or multiple X.509 certificate(s) to canonical RFC 7468 PEM format.
     #
-    # @param cert [String] The original certificate(s)
-    # @param multi [true|false] Whether to return multiple keys delimited by newline
-    # @return [String|nil] The formatted certificate(s)
+    # @param cert [String] A string containing unformatted certificate(s).
+    # @param multi [true|false] Whether to return multiple certificates delimited by newline.
+    #   Default false.
+    # @return [String] The formatted certificate(s). Returns nil if the input is blank.
     def format_cert(cert, multi: false)
-      detect_pems(cert, 'CERTIFICATE', multi: multi) do |pem|
-        format_cert_single(pem)
-      end
+      pem_array_to_string(format_cert_array(cert), multi: multi)
+    end
+
+    # Formats one or multiple X.509 private keys(s) to canonical RFC 7468 PEM format.
+    #
+    # @param keys [String] A string containing unformatted private keys(s).
+    # @return [Array<String>] The formatted private keys(s).
+    def format_private_key_array(keys)
+      format_pem_array(keys, 'PRIVATE KEY', %w[RSA ECDSA EC DSA])
     end
 
-    # Formats one or many private key(s) to canonical PEM format
-    # with 64-char lines and BEGIN/END headers.
+    # Formats one or multiple private key(s) to canonical RFC 7468 PEM format.
     #
-    # @param key [String] The original private key(s)
-    # @param multi [true|false] Whether to return multiple keys delimited by newline
-    # @return [String|nil] The formatted private key(s)
+    # @param key [String] A string containing unformatted private keys(s).
+    # @param multi [true|false] Whether to return multiple keys delimited by newline.
+    #   Default false.
+    # @return [String|nil] The formatted private key(s). Returns nil if the input is blank.
     def format_private_key(key, multi: false)
-      detect_pems(key, '(?:RSA|DSA|EC|ECDSA) PRIVATE KEY', multi: multi) do |pem|
-        format_private_key_single(pem)
-      end
+      pem_array_to_string(format_private_key_array(key), multi: multi)
     end
 
     private
 
-    def detect_pems(str, label, multi: false, &block)
-      return if str.nil? || str.empty?
-      return str unless str.ascii_only?
-      return if str.match?(/\A\s*\z/)
+    def format_pem_array(str, label, known_prefixes = nil)
+      str = str&.encode('UTF-8', invalid: :replace, undef: :replace, replace: '?')
+      return [] if !str || str.strip.empty?
 
-      pems = str.scan(/-{5}\s*BEGIN #{label}\s*-{5}.*?-{5}\s*END #{label}\s*-{5}?/m).map(&block)
+      # Scan the string for PEMs and format each one
+      pems = str.scan(pem_scan_regexp(label)).map { |pem| format_pem(pem, label, known_prefixes) }
+
+      # Try to format the original string if no PEMs were found
+      pems = format_pem(str, label, known_prefixes).scan(pem_scan_regexp(label)) if pems.empty?
+      pems
+    end
+
+    def pem_array_to_string(pem_array, multi: false)
+      pem_array = Array(pem_array)
+      return if pem_array.empty?
+      multi ? pem_array.join("\n") : pem_array.first
+    end
+
+    # Given a PEM, a label such as "PRIVATE KEY", and a list of known prefixes
+    # such as "RSA", "DSA", etc., returns the formatted PEM preserving the known
+    # prefix if possible.
+    def format_pem(pem, label, known_prefixes = nil)
+      detected_prefix = detect_label_prefix(pem, label, known_prefixes)
+      prefixed_label = "#{detected_prefix}#{label}"
+      "-----BEGIN #{prefixed_label}-----\n#{format_pem_body(pem)}\n-----END #{prefixed_label}-----"
+    end
 
-      # Try to format the original string if no pems were found
-      return yield(str) if pems.empty?
+    # Given a PEM, a label such as "PRIVATE KEY", and a list of known prefixes
+    # such as "RSA", "DSA", etc., detects and returns the known prefix if it exists.
+    def detect_label_prefix(pem, label, known_prefixes)
+      return unless known_prefixes && !known_prefixes.empty?
 
-      multi ? pems.join("\n") : pems.first
+      pem.match(/((?:#{Array(known_prefixes).join('|')}) )#{label}/)&.[](1)
     end
 
-    def format_cert_single(cert)
-      format_pem(cert, 'CERTIFICATE')
+    # Given a PEM, strips all whitespace and the BEGIN/END lines,
+    # then splits the body into 64-character lines.
+    def format_pem_body(pem)
+      pem.gsub(/\s|#{pem_scan_header}/, '').scan(/.{1,64}/).join("\n")
     end
 
-    def format_private_key_single(key)
-      algo = key.match(/((?:RSA|ECDSA|EC|DSA) )PRIVATE KEY/)&.[](1)
-      label = "#{algo}PRIVATE KEY"
-      format_pem(key, label)
+    # Returns a regexp which can be used to loosely match unformatted PEM(s) in a string.
+    def pem_scan_regexp(label)
+      /#{pem_scan_header('BEGIN', label)}#{PEM_SCAN_BASE64}#{pem_scan_header('END', label)}/m
     end
 
-    # Strips all whitespace and the BEGIN/END lines,
-    # then splits the string into 64-character lines,
-    # and re-applies BEGIN/END labels
-    def format_pem(str, label)
-      str = str.gsub(/\s|-{5}\s*(BEGIN|END) [A-Z\d\s]+-{5}/, '').scan(/.{1,64}/).join("\n")
-      "-----BEGIN #{label}-----\n#{str}\n-----END #{label}-----"
+    # Returns a regexp component string to match PEM headers.
+    def pem_scan_header(marker = '(BEGIN|END)', label = '[A-Z\d]+')
+      "-{5}\\s*#{marker} (?:[A-Z\\d]+ )*#{label}\\s*-{5}"
     end
   end
 end

lib/ruby_saml/utils.rb

@@ -37,8 +37,7 @@ module Utils
     # @param cert [OpenSSL::X509::Certificate|String] The x509 certificate.
     # @return [true|false] Whether the certificate is expired.
     def is_cert_expired(cert)
-      cert = OpenSSL::X509::Certificate.new(cert) if cert.is_a?(String)
-
+      cert = build_cert_object(cert) if cert.is_a?(String)
       cert.not_after < Time.now
     end
 
@@ -47,7 +46,7 @@ def is_cert_expired(cert)
     # @param cert [OpenSSL::X509::Certificate|String] The x509 certificate.
     # @return [true|false] Whether the certificate is currently active.
     def is_cert_active(cert)
-      cert = OpenSSL::X509::Certificate.new(cert) if cert.is_a?(String)
+      cert = build_cert_object(cert) if cert.is_a?(String)
       now = Time.now
       cert.not_before <= now && cert.not_after >= now
     end
@@ -87,42 +86,44 @@ def parse_duration(duration, timestamp=Time.now.utc)
       datetime.to_time.utc.to_i + (durHours * 3600) + (durMinutes * 60) + durSeconds
     end
 
-    # Formats one or many X.509 certificate(s) to canonical
-    # PEM format with 64-char lines and BEGIN/END headers.
+    # Formats one or multiple X.509 certificate(s) to canonical RFC 7468 PEM format.
     #
-    # @param cert [String] The original certificate(s)
-    # @param multi [true|false] Whether to return multiple keys delimited by newline
-    # @return [String|nil] The formatted certificate(s)
+    # @param cert [String] The original certificate(s).
+    # @param multi [true|false] Whether to return multiple keys delimited by newline.
+    #   Default true for compatibility with legacy behavior (i.e. to parse cert chains).
+    # @return [String] The formatted certificate(s). For legacy compatibility reasons,
+    #   this method returns the original string if the input cannot be parsed.
     def format_cert(cert, multi: true)
-      PemFormatter.format_cert(cert, multi: multi)
+      PemFormatter.format_cert(cert, multi: multi) || cert
     end
 
-    # Formats one or many private key(s) to canonical PEM format
-    # with 64-char lines and BEGIN/END headers.
+    # Formats one or multiple private key(s) to canonical RFC 7468 PEM format.
     #
     # @param key [String] The original private key(s)
-    # @param multi [true|false] Whether to return multiple keys delimited by newline
-    # @return [String|nil] The formatted private key(s)
+    # @param multi [true|false] Whether to return multiple keys delimited by newline.
+    #   Default false for compatibility with legacy behavior.
+    # @return [String] The formatted private key(s). For legacy compatibility reasons,
+    #   this method returns the original string if the input cannot be parsed.
     def format_private_key(key, multi: false)
-      PemFormatter.format_private_key(key, multi: multi)
+      PemFormatter.format_private_key(key, multi: multi) || key
     end
 
     # Given a certificate string, return an OpenSSL::X509::Certificate object.
     #
-    # @param cert [String] The original certificate
+    # @param pem [String] The original certificate
     # @return [OpenSSL::X509::Certificate] The certificate object
-    def build_cert_object(cert)
-      return unless (pem = format_cert(cert, multi: false))
+    def build_cert_object(pem)
+      return unless (pem = PemFormatter.format_cert(pem, multi: false))
 
       OpenSSL::X509::Certificate.new(pem)
     end
 
     # Given a private key string, return an OpenSSL::PKey::RSA object.
     #
-    # @param cert [String] The original private key
-    # @return [OpenSSL::PKey::RSA] The private key object
-    def build_private_key_object(private_key)
-      return unless (pem = format_private_key(private_key, multi: false))
+    # @param pem [String] The original private key.
+    # @return [OpenSSL::PKey::RSA] The private key object.
+    def build_private_key_object(pem)
+      return unless (pem = PemFormatter.format_private_key(pem, multi: false))
 
       OpenSSL::PKey::RSA.new(pem)
     end

test/utils_test.rb

@@ -45,7 +45,7 @@ def result(duration, reference = 0)
 
     it "returns empty string when the cert is an empty string" do
       cert = ''
-      assert_nil RubySaml::Utils.format_cert(cert)
+      assert_equal '', RubySaml::Utils.format_cert(cert)
     end
 
     it "returns nil when the cert is nil" do
@@ -67,7 +67,7 @@ def result(duration, reference = 0)
       assert_equal formatted_certificate, RubySaml::Utils.format_cert(invalid_certificate2)
     end
 
-    it "returns the cert when it's encoded" do
+    it "returns the original cert when it's encoded" do
       encoded_certificate = read_certificate("certificate.der")
       assert_equal encoded_certificate, RubySaml::Utils.format_cert(encoded_certificate)
     end
@@ -94,7 +94,7 @@ def result(duration, reference = 0)
 
     it "returns empty string when the private key is an empty string" do
       private_key = ''
-      assert_nil RubySaml::Utils.format_private_key(private_key)
+      assert_equal '', RubySaml::Utils.format_private_key(private_key)
     end
 
     it "returns nil when the private key is nil" do