Allow SP certificates to be OpenSSL::X509::Certificate #726
Conversation
|
@tobiasamft can you check if this is solved on the v2.x branch? I think it might be already. If it is, we can close this PR b/c we are releasing v2.x soon. |
|
@johnnyshields unfortunately v2.x does not solve this. Using |
|
ok. Can you raise the PR to the v2.x branch then please? I will review it. |
lib/onelogin/ruby-saml/settings.rb
Outdated
| def build_cert_object(cert) | ||
| return cert if cert.is_a?(OpenSSL::X509::Certificate) | ||
|
|
||
| OneLogin::RubySaml::Utils.build_cert_object(cert) |
There was a problem hiding this comment.
Why not adapt directly the Utils.build_cert_object to be able to handle string and OpenSSL::X509::Certificate?
There was a problem hiding this comment.
Yes, I was thinking about that, too. But then I decided to catch the problem as early as possible. And if it's fixed in Utils, it creates kind of a dependency. Utils would fix problems that come from Settings. Settings would not correctly work on it's own without changing Utils.
This allows settings to accept instances of OpenSSL::X509::Certificate as service provider (SP) certificates.
tobiasamft
force-pushed
the
allow-openssl-certificate-instances
branch
from
c0b1dd7
to
603f97e
Compare
|
@johnnyshields I've rebased the branch onto v2.x |
lib/ruby_saml/settings.rb
Outdated
| def build_cert_object(cert) | ||
| return cert if cert.is_a?(OpenSSL::X509::Certificate) | ||
|
|
||
| OneLogin::RubySaml::Utils.build_cert_object(cert) |
There was a problem hiding this comment.
let's just change this method in OneLogin::RubySaml::Utils to return a cert if one is given as an argument (it should be "idempotent")
Same with RubySaml::Utils.build_private_key_object
There was a problem hiding this comment.
👍 Your code, your decision 😄 Will change that.
|
@tobiasamft see comment |
Return the original certificate from Utils.build_cert_object when an instance of OpenSSL::X509::Certificate is given. And return the original key from Utils.build_private_key_object when an instance of OpenSSL::PKey::PKey is given.
| return true if cert.is_a?(OpenSSL::X509::Certificate) | ||
|
|
||
| cert && !cert.empty? | ||
| end |
There was a problem hiding this comment.
all changes in this file can be rolled back
There was a problem hiding this comment.
Without it we get the initial NoMethodError again (because validate_sp_certs_params is called before Utils.build_cert_object):
NoMethodError: undefined method `empty?' for an instance of OpenSSL::X509::Certificate
lib/ruby_saml/settings.rb:377:in `validate_sp_certs_params!'
There was a problem hiding this comment.
OK I will check this later, thanks!
This allows settings to accept instances of OpenSSL::X509::Certificate as service provider (SP) certificates.
Solves #723
Version 1.16.0 was, at least partially, able to handle
OpenSSL::X509::Certificateas input for settings.certificate (e.g. when usingOneLogin::RubySaml::Response).Since
settings.get_sp_certsis the only interface that is used to access certificates, it should be enough to test that interface with instances ofOpenSSL::X509::Certificate. There are 3 ways to insert certs, all of them have been tested:Note that both deprecated interfaces
settings.get_sp_certandsettings.get_sp_cert_newusesettings.get_sp_certsinternally. Thus, they are covered as well.Same approach could be used for SP private key to accept
OpenSSL::PKey.Maybe it's a good idea to make all certificates from settings
attr_writerfor public andattr_accessorfor private access to ensure that certs are accessed viasettings.get_sp_certsonly (but that would break current interface).