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.

Sign up for GitHub

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

Support idp cert multi with string keys #576

Merged
merged 3 commits into from
Jan 2, 2023
Merged

Support idp cert multi with string keys #576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb70449
Document issue: signature verification fails when idp_cert_multi's ke…
Mar 10, 2021
b27dd60
Add support idp_cert_multi with string keys
Mar 10, 2021
01c95db
Test calls to get_idp_cert_multi with string keys
alxckn Sep 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions lib/onelogin/ruby-saml/settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,17 +195,13 @@ def get_idp_cert_multi

certs = {:signing => [], :encryption => [] }

if idp_cert_multi.key?(:signing) and not idp_cert_multi[:signing].empty?
idp_cert_multi[:signing].each do |idp_cert|
formatted_cert = OneLogin::RubySaml::Utils.format_cert(idp_cert)
certs[:signing].push(OpenSSL::X509::Certificate.new(formatted_cert))
end
end
[:signing, :encryption].each do |type|
certs_for_type = idp_cert_multi[type] || idp_cert_multi[type.to_s]
next if !certs_for_type || certs_for_type.empty?

if idp_cert_multi.key?(:encryption) and not idp_cert_multi[:encryption].empty?
idp_cert_multi[:encryption].each do |idp_cert|
certs_for_type.each do |idp_cert|
formatted_cert = OneLogin::RubySaml::Utils.format_cert(idp_cert)
certs[:encryption].push(OpenSSL::X509::Certificate.new(formatted_cert))
certs[type].push(OpenSSL::X509::Certificate.new(formatted_cert))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be:

certs[type.to_sym].push(OpenSSL::X509::Certificate.new(formatted_cert))

So we always generate certs with the symbol index, as it used to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is a symbol, it can only be one of [:signing, :encryption]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alxckn 's code is correct here.

end
end

Expand Down
10 changes: 10 additions & 0 deletions test/response_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,6 +972,16 @@ def generate_audience_error(expected, actual)
assert_empty response_valid_signed.errors
end

it "return true when at least a cert on idp_cert_multi is valid and keys are strings" do
settings.idp_cert_multi = {
"signing" => [ruby_saml_cert_text2, ruby_saml_cert_text],
"encryption" => []
}
response_valid_signed.settings = settings
res = response_valid_signed.send(:validate_signature)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to this unittest, I expect one using a call to get_idp_cert_multi as well that verifies the certs were properly loaded on the settings object at settings_test.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests verifying that calls to get_idp_cert_multi are in fact working: 01c95db

assert_empty response_valid_signed.errors
end

it "return false when none cert on idp_cert_multi is valid" do
settings.idp_cert_fingerprint = ruby_saml_cert_fingerprint
settings.idp_cert_multi = {
Expand Down
43 changes: 43 additions & 0 deletions test/settings_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,35 @@ class SettingsTest < Minitest::Test
assert_equal empty_multi, @settings.get_idp_cert_multi
end

it "returns partial hash when contains some values with string keys" do
empty_multi = {
:signing => [],
:encryption => []
}

@settings.idp_cert_multi = {
"signing" => []
}
assert_equal empty_multi, @settings.get_idp_cert_multi

@settings.idp_cert_multi = {
"encryption" => []
}
assert_equal empty_multi, @settings.get_idp_cert_multi

@settings.idp_cert_multi = {
"signing" => [],
"encryption" => []
}
assert_equal empty_multi, @settings.get_idp_cert_multi

@settings.idp_cert_multi = {
"yyy" => [],
"zzz" => []
}
assert_equal empty_multi, @settings.get_idp_cert_multi
end

it "returns the hash with certificates when values were valid" do
certificates = ruby_saml_cert_text
@settings.idp_cert_multi = {
Expand All @@ -271,6 +300,20 @@ class SettingsTest < Minitest::Test
assert @settings.get_idp_cert_multi[:encryption][0].kind_of? OpenSSL::X509::Certificate
end

it "returns the hash with certificates when values were valid and with string keys" do
certificates = ruby_saml_cert_text
@settings.idp_cert_multi = {
"signing" => [ruby_saml_cert_text],
"encryption" => [ruby_saml_cert_text],
}

assert @settings.get_idp_cert_multi.kind_of? Hash
assert @settings.get_idp_cert_multi[:signing].kind_of? Array
assert @settings.get_idp_cert_multi[:encryption].kind_of? Array
assert @settings.get_idp_cert_multi[:signing][0].kind_of? OpenSSL::X509::Certificate
assert @settings.get_idp_cert_multi[:encryption][0].kind_of? OpenSSL::X509::Certificate
end

it "raises when there is a cert in idp_cert_multi not valid" do
certificate = read_certificate("formatted_certificate")

Expand Down