Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tobiasamft opened this issue Sep 23, 2024 · 7 comments

Comments

@tobiasamft
Copy link

tobiasamft commented Sep 23, 2024

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
Copy link
Collaborator

pitbulk commented Sep 27, 2024

certificate and private_key are related with the SP.

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

@tobiasamft
Copy link
Author

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
Copy link
Collaborator

pitbulk commented Sep 30, 2024

@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
Copy link
Author

tobiasamft commented Sep 30, 2024

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
Copy link
Author

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

@pitbulk
Copy link
Collaborator

pitbulk commented Oct 1, 2024

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
Copy link
Author

Finally found some time to open PR #726.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants