diff --git a/lib/saml_idp/service_provider.rb b/lib/saml_idp/service_provider.rb index 56dff010..6ec05934 100644 --- a/lib/saml_idp/service_provider.rb +++ b/lib/saml_idp/service_provider.rb @@ -21,10 +21,8 @@ def valid? end def valid_signature?(doc, require_signature = false, options = {}) - Array(certs).any? do |cert| - if doc.valid_signature?(fingerprint_cert(cert), options.merge(cert: cert)) - @matching_cert = cert - end + @matching_cert = Array(certs).find do |cert| + doc.valid_signature?(fingerprint_cert(cert), options.merge(cert: cert)) end if require_signature || should_validate_signature? diff --git a/spec/lib/saml_idp/service_provider_spec.rb b/spec/lib/saml_idp/service_provider_spec.rb index 5b6b950d..12bbd116 100644 --- a/spec/lib/saml_idp/service_provider_spec.rb +++ b/spec/lib/saml_idp/service_provider_spec.rb @@ -3,6 +3,8 @@ module SamlIdp describe ServiceProvider do subject { described_class.new attributes } let(:attributes) { {} } + let(:cert) { saml_settings.get_sp_cert } + let(:options) { {} } it { is_expected.to respond_to :metadata_url } it { is_expected.not_to be_valid } @@ -17,5 +19,145 @@ module SamlIdp it { is_expected.to be_valid } end + + describe "#valid_signature" do + let(:raw_xml) do + SamlIdp::Request.from_deflated_request( + make_saml_request + ).raw_xml + end + + let(:doc) { Saml::XML::Document.parse(raw_xml) } + + describe 'the signature is not required' do + describe "the service provider has no certs" do + it "returns true" do + expect(subject.valid_signature?(doc)).to be true + end + end + + describe "the service provider has certs" do + before { subject.certs = [cert] } + it "returns true" do + expect(subject.valid_signature?(doc)).to be true + end + end + end + + describe "the signature is required" do + before { subject.validate_signature = true } + + describe "a cert is not present in the document" do + let(:raw_xml) do + SamlIdp::Request.from_deflated_request( + signed_auth_request_options["SAMLRequest"] + ).raw_xml + end + let(:options) { {get_params: signed_auth_request_options}.with_indifferent_access } + + describe "the service provider has no certs" do + it "returns false" do + expect(subject.valid_signature?(doc, true, options)).to be false + end + end + + describe "the service provider has one or more certs" do + describe 'one valid cert' do + before { subject.certs = [cert] } + + it "returns true" do + expect(subject.valid_signature?(doc, true, options)).to be true + end + end + + describe "one invalid cert" do + before { subject.certs = [invalid_cert]} + + it "returns false" do + expect(subject.valid_signature?(doc, true, options)).to be false + end + end + + describe "multiple certs" do + before { subject.certs = [invalid_cert, cert] } + + it "returns true" do + expect(subject.valid_signature?(doc, true, options)).to be true + end + end + end + end + + describe "a cert is present in the document" do + let(:raw_xml) do + SamlIdp::Request.from_deflated_request( + signed_auth_request + ).raw_xml + end + + describe "the service provider has no certs" do + it "returns false" do + expect(subject.valid_signature?(doc, true)).to be false + end + + describe "the requirement is passed through the method" do + it "returns false" do + expect(subject.valid_signature?(doc, true)).to be false + end + end + end + + describe "the service provider has one or more certs" do + describe 'one valid cert' do + before { subject.certs = [cert] } + + it "returns true" do + expect(subject.valid_signature?(doc)).to be true + end + end + + describe "one invalid cert" do + before { subject.certs = [invalid_cert]} + + it "returns false" do + expect(subject.valid_signature?(doc, true)).to be false + end + end + + describe "multiple certs" do + let(:other_cert) do + OpenSSL::X509::Certificate.new(cloudhsm_idp_x509_cert) + end + + describe "the valid cert is registered in the idp" do + before { subject.certs = [other_cert, invalid_cert, cert] } + + it "returns true" do + expect(subject.valid_signature?(doc, true)).to be true + end + + it "matches the right cert" do + subject.valid_signature?(doc) + expect(subject.matching_cert).to eq cert + end + end + + describe "the valid cert is not registered in the idp" do + before { subject.certs = [other_cert, invalid_cert] } + + it "returns false" do + expect(subject.valid_signature?(doc, true)).to be false + end + + it "matches the right cert" do + subject.valid_signature?(doc) + expect(subject.matching_cert).to be nil + end + end + end + end + end + end + end end end diff --git a/spec/support/certificate_helpers.rb b/spec/support/certificate_helpers.rb index f49ca879..53db42f0 100644 --- a/spec/support/certificate_helpers.rb +++ b/spec/support/certificate_helpers.rb @@ -37,4 +37,8 @@ def encrypted_secret_key def encrypted_secret_key_password 'im a secret password.' end + + def invalid_cert + OpenSSL::X509::Certificate.new(File.read('spec/support/certificates/too_short_cert.crt')) + end end diff --git a/spec/support/certificates/too_short_cert.crt b/spec/support/certificates/too_short_cert.crt new file mode 100644 index 00000000..c9d170d4 --- /dev/null +++ b/spec/support/certificates/too_short_cert.crt @@ -0,0 +1,3 @@ +-----BEGIN CERTIFICATE----- +MIICnTCCAgagAwIBAgIGAWVhrSugMA0GCSqGSIb3DQEBCwUAMIGHMR0wGwYDVQQDDBRzZWN1cmVwb3J0YWwub3BtLmdvdjEnMCUGA1UECgweT2ZmaWNlIG9mIFBlcnNvbm5lbCBNYW5hZ2VtZW50MQ4wDAYDVQQLDAVPQ0lTTzETMBEGA1UEBwwKV2FzaGluZ3RvbjELMAkGA1UECAwCREMxCzAJBgNVBAYTAlVTMB4XDTE4MDgyMjEyNDc0M1oXDTIzMDgyMjA0MDAwMFowgYcxHTAbBgNVBAMMFHNlY3VyZXBvcnRhbC5vcG0uZ292MScwJQYDVQQKDB5PZmZpY2Ugb2YgUGVyc29ubmVsIE1hbmFnZW1lbnQxDjAMBgNVBAsMBU9DSVNPMRMwEQYDVQQHDApXYXNoaW5ndG9uMQswCQYDVQQIDAJEQzELMAkGA1UEBhMCVVMwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAIpUzkPbQdNQIRA3/64quioyOIetu4101bWg2NJFFu1Iy/xOAI7+hyGIbnvP1ZwlzB65+0f6yzZfrBy78uMIaPBQycDUXjrGAe9JS6eoJynSZiFvlUs5OOSQmoEZ1MfajH0EDhGV0drWTHFoPCdcckbdTd9TnyZdolVoC/3L93fbAgMBAAGjEjAQMA4GA1UdDwEB/wQEAwIC9DANBgkqhkiG9w0BAQsFAAOBgQBlqriiUBu0iiF+hMHqws5oHY+d6x66rR52XoBgnfIgKuwuQqWU57HCYXw8fho4ykefn+F7fdgAOOHNXjdTYVSlPihsU0pXqEA5NBDyCg5rzyvoLIz9jLDiIrmgZ8Tjog2wRm5aOV02fp4gcnjjBB+vNe8U+lmiJaBhY3AeiA7gSA== +-----END CERTIFICATE----- diff --git a/spec/support/saml_request_macros.rb b/spec/support/saml_request_macros.rb index f65e62e5..fa739e78 100644 --- a/spec/support/saml_request_macros.rb +++ b/spec/support/saml_request_macros.rb @@ -3,14 +3,27 @@ module SamlRequestMacros def make_saml_request(requested_saml_acs_url = "https://foo.example.com/saml/consume") - auth_request = OneLogin::RubySaml::Authrequest.new - auth_url = auth_request.create(saml_settings(requested_saml_acs_url)) + auth_url = url(saml_settings(requested_saml_acs_url)) CGI.unescape(auth_url.split("=").last) end - def make_invalid_saml_request(requested_saml_acs_url = "https://foo.example.com/saml/consume") + def url(saml_settings) auth_request = OneLogin::RubySaml::Authrequest.new - auth_url = auth_request.create(invalid_saml_settings) + auth_request.create(saml_settings) + end + + def signed_auth_request + CGI.unescape(url(signed_saml_settings).split("=").last) + end + + def signed_auth_request_options + signed_auth_request_options ||= + uri = URI(url(signed_saml_settings(embed: false))) + Rack::Utils.parse_nested_query uri.query + end + + def make_invalid_saml_request + auth_url = url(invalid_saml_settings) CGI.unescape(auth_url.split("=").last) end @@ -32,9 +45,9 @@ def make_sp_logout_request(requested_saml_logout_url = 'https://foo.example.com/ OneLogin::RubySaml::Logoutrequest.new.create(settings) end - def saml_settings(saml_acs_url = "https://foo.example.com/saml/consume") + def saml_settings(requested_saml_acs_url = "https://foo.example.com/saml/consume") settings = OneLogin::RubySaml::Settings.new - settings.assertion_consumer_service_url = saml_acs_url + settings.assertion_consumer_service_url = requested_saml_acs_url settings.issuer = "http://example.com/issuer" settings.idp_sso_target_url = "http://idp.com/saml/idp" settings.idp_slo_target_url = "http://idp.com/saml/idp-slo" @@ -51,7 +64,19 @@ def saml_settings(saml_acs_url = "https://foo.example.com/saml/consume") settings end - def invalid_saml_settings(saml_acs_url = "https://foo.example.com/saml/consume") + def signed_saml_settings(embed: true) + settings = saml_settings("https://foo.example.com/saml/consume") + settings.security = { + embed_sign: embed, + authn_requests_signed: true, + want_assertions_signed: true, + digest_method: 'http://www.w3.org/2001/04/xmlenc#sha256', + signature_method: 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256' + } + settings + end + + def invalid_saml_settings settings = saml_settings.dup settings.issuer = '' settings