SAML-Toolkits / ruby-saml

SAML SSO for Ruby
MIT License
921 stars 567 forks source link

undefined method 'empty?' for an instance of OpenSSL::X509::Certificate with version 1.17.0 #723

Open tobiasamft opened 2 months ago

tobiasamft commented 2 months ago

Hi, I'm observing the following error after updating ruby-saml from 1.16.0 to 1.17.0:

/usr/local/bundle/gems/ruby-saml-1.17.0/lib/onelogin/ruby-saml/settings.rb:316:in `validate_sp_certs_params!': undefined method `empty?' for an instance of OpenSSL::X509::Certificate (NoMethodError)

        cert     = certificate     && !certificate.empty?
                                                  ^^^^^^^
    from /usr/local/bundle/gems/ruby-saml-1.17.0/lib/onelogin/ruby-saml/settings.rb:309:in `get_all_sp_certs'
    from /usr/local/bundle/gems/ruby-saml-1.17.0/lib/onelogin/ruby-saml/settings.rb:213:in `get_sp_certs'
    from /usr/local/bundle/gems/ruby-saml-1.17.0/lib/onelogin/ruby-saml/settings.rb:252:in `get_sp_decryption_keys'
    from /usr/local/bundle/gems/ruby-saml-1.17.0/lib/onelogin/ruby-saml/response.rb:972:in `generate_decrypted_document'
    from /usr/local/bundle/gems/ruby-saml-1.17.0/lib/onelogin/ruby-saml/response.rb:70:in `initialize'
    from saml_response.rb:8:in `new'
    from saml_response.rb:8:in `<main>'

I've tested the following code snippet:

require 'onelogin/ruby-saml'

certificate = OpenSSL::X509::Certificate.new(File.read('ruby-saml.crt'))
private_key = File.read('ruby-saml.key')
saml_response = Base64.decode64(File.read('signed_message_encrypted_signed_assertion.xml.base64'))

response =
  OneLogin::RubySaml::Response.new(
    saml_response,
    settings: OneLogin::RubySaml::Settings.new(certificate:, private_key:)
  )
pp response

I've used the certificate, key, and xml from this repo to test it:

wget https://raw.githubusercontent.com/SAML-Toolkits/ruby-saml/refs/heads/master/test/certificates/ruby-saml.crt
wget https://raw.githubusercontent.com/SAML-Toolkits/ruby-saml/refs/heads/master/test/certificates/ruby-saml.key
wget https://raw.githubusercontent.com/SAML-Toolkits/ruby-saml/refs/heads/master/test/responses/signed_message_encrypted_signed_assertion.xml.base64

(My assumption is that ruby-saml.crt and ruby-saml.key have been used for encryption, otherwise I would get another error with version 1.16.0 like OpenSSL::PKey::PKeyError: EVP_PKEY_decrypt: failed to decrypt)

Any idea why this fails after updating from 1.16.0 to 1.17.0? Code in settings.rb hasn't been changed with the new version. With version 1.16.0, a valid response is rendered. I've tested with Ruby 3.3.4 and 3.3.5.

pitbulk commented 1 month ago

certificate and private_key are related with the SP.

In order to validate the SAMLResponse, you need to provide the idp_certificate.

tobiasamft commented 1 month ago

In order to validate the SAMLResponse, you need to provide the idp_certificate.

Correct, but this does not seem to be the issue here. I've just realized that there actually was a related change introduced with 1.17.0 (sorry, haven't seen this earlier).

The real issue here is that I'm passing an instance of OpenSSL::X509::Certificate to the Settings (instead of plain cert). This was possible with 1.16.0 and there are some checks in the code that accept either OpenSSL::X509::Certificate or String.

Now with 1.17.0, OpenSSL::X509::Certificate can't be used anymore because #673 has introduced this check : cert = certificate && !certificate.empty? which works for String only.

Question is: should settings.certificate accept both, OpenSSL::X509::Certificate and String? Or should it just accept String?

pitbulk commented 1 month ago

@tobiasamft , Can you check if this change solves your issue?

def validate_sp_certs_params!
  multi    = sp_cert_multi && !sp_cert_multi.empty?
  cert     = certificate if certificate && (certificate.is_a?(OpenSSL::X509::Certificate) || !certificate.empty?)
  cert_new = certificate_new if certificate_new && (certificate_new.is_a?(OpenSSL::X509::Certificate) || !certificate_new.empty?)
  pk       = private_key if private_key && (private_key.is_a?(OpenSSL::PKey::RSA) || !private_key.empty?)
  if multi && (cert || cert_new || pk)
    raise ArgumentError.new("Cannot specify both sp_cert_multi and certificate, certificate_new, private_key parameters")
  end
end
tobiasamft commented 1 month ago

Works partially, then you have another issue with utils.rb line 151:

      # Given a certificate string, return an OpenSSL::X509::Certificate object.
      #
      # @param cert [String] The original certificate
      # @return [OpenSSL::X509::Certificate] The certificate object
      #
      def self.build_cert_object(cert)
        return nil if cert.nil? || cert.empty?

        OpenSSL::X509::Certificate.new(format_cert(cert))
      end

There might be additional methods with this problem.

tobiasamft commented 1 month ago

If you want to support both, OpenSSL::X509::Certificate and String, I could try to support with a PR.

pitbulk commented 1 month ago

Ok please, send that PR and will review it. If it was working in the past, it makes sense to keep it.

cc @johnnyshields

tobiasamft commented 1 month ago

Finally found some time to open PR #726.