diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index ca5013e9517..b371a823c01 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -164,7 +164,7 @@ def saml_response def matching_cert return @matching_cert if defined?(@matching_cert) - @matching_cert = current_service_provider.ssl_certs.find do |ssl_cert| + matching_cert = current_service_provider.ssl_certs.find do |ssl_cert| fingerprint = Fingerprinter.fingerprint_cert(ssl_cert) saml_request = SamlIdp::Request.from_deflated_request( @@ -177,7 +177,11 @@ def matching_cert saml_request.service_provider.fingerprint = fingerprint saml_request.valid_signature? end + rescue OpenSSL::X509::CertificateError + false end + + @matching_cert = matching_cert || current_service_provider.ssl_certs.first end def encryption_opts diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 5e8a95a9bec..d9544c86bc9 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -36,8 +36,9 @@ def logout # Plumb the fingerprint through to the internal service_provider representation if saml_request&.service_provider - saml_request.service_provider.fingerprint = - Fingerprinter.fingerprint_cert(matching_cert || current_service_provider.ssl_certs.first) + saml_request.service_provider.fingerprint = matching_cert ? + Fingerprinter.fingerprint_cert(matching_cert) : + 'some-non-nil-value' end track_logout_event @@ -63,6 +64,12 @@ def saml_request_valid?(saml_request) valid_saml_request? end + def valid_saml_request? + super + rescue OpenSSL::X509::CertificateError + false + end + def saml_metadata SamlEndpoint.new(request).saml_metadata end diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 9e6af8fa348..e309473f23d 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -69,6 +69,35 @@ expect(response).to be_bad_request end + + it 'handles bad issuer' do + settings = sp1_saml_settings.dup + settings.issuer = 'BAD ISSUER' + + request_url = OneLogin::RubySaml::Logoutrequest.new.create(settings) + saml_request = UriService.params(request_url)[:SAMLRequest] + + delete :logout, params: { SAMLRequest: saml_request } + + expect(response.code).to_not eq(500) + expect(response).to be_bad_request + end + + it 'handles not enough cert data error' do + settings = sp1_saml_settings.dup + settings.issuer = 'BAD ISSUER' + + allow_any_instance_of(SamlIdp::XMLSecurity::SignedDocument).to receive(:validate). + and_raise(OpenSSL::X509::CertificateError.new('not enough cert data')) + + request_url = OneLogin::RubySaml::Logoutrequest.new.create(settings) + saml_request = UriService.params(request_url)[:SAMLRequest] + + delete :logout, params: { SAMLRequest: saml_request } + + expect(response.code).to_not eq(500) + expect(response).to be_bad_request + end end describe '/api/saml/metadata' do