From 4b7e4c88e183ad4502c5ae433a7e6570e60d218f Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 29 Oct 2024 14:58:01 +0100 Subject: [PATCH] [improvements] Return error to application and support SLO request validation (#224) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Squash commits for saml_idp gem * [feat] Allow SP config force signature validation (#16) * Allow SP config force signature validation * Allow SP config force signature validation Tested with Slack with Authn request signature option --------- Co-authored-by: zogoo * [feat] Don’t ignore certificates without usage (#17) I have tested with live SAML SP apps and it works fine * Unspecified certifciate from SP metadata --------- Co-authored-by: zogoo * wip add error collector * Fix type and rewrite request with proper validation test cases * Try with proper way to update helper method (#19) * Set minimum test coverage (#207) * Set minimum test coverage to a very high value for testing * Update minimum coverage to actual current value * Try with proper way to update helper method * Correctly decode and mock with correct REXML class * Drop the min coverage --------- Co-authored-by: Mathieu Jobin Co-authored-by: zogoo * Lead error render decision to gem user * Validate the certificate's existence before verifying the signature. * [feat] Collect request validation errors (#18) * wip add error collector * Fix type and rewrite request with proper validation test cases * Lead error render decision to gem user * Validate the certificate's existence before verifying the signature. --------- Co-authored-by: zogoo * Support lowercase percent-encoded sequences for URL encoding (#20) Co-authored-by: zogoo * Remove duplications * Pre-conditions need to be defined in before section * Le's not test logger in here * Let's not break anything for now * Rename correctly --------- Co-authored-by: zogoo Co-authored-by: Mathieu Jobin --- lib/saml_idp/incoming_metadata.rb | 9 + lib/saml_idp/request.rb | 43 ++- lib/saml_idp/service_provider.rb | 1 + saml_idp.gemspec | 2 +- spec/lib/saml_idp/configurator_spec.rb | 17 +- spec/lib/saml_idp/controller_spec.rb | 25 +- spec/lib/saml_idp/incoming_metadata_spec.rb | 75 +++++ spec/lib/saml_idp/request_spec.rb | 304 ++++++++++---------- spec/support/saml_request_macros.rb | 56 ++-- spec/support/security_helpers.rb | 4 +- 10 files changed, 344 insertions(+), 192 deletions(-) diff --git a/lib/saml_idp/incoming_metadata.rb b/lib/saml_idp/incoming_metadata.rb index fe0f5c61..250a5fea 100644 --- a/lib/saml_idp/incoming_metadata.rb +++ b/lib/saml_idp/incoming_metadata.rb @@ -63,6 +63,15 @@ def contact_person end hashable :contact_person + def unspecified_certificate + xpath( + "//md:SPSSODescriptor/md:KeyDescriptor[not(@use)]/ds:KeyInfo/ds:X509Data/ds:X509Certificate", + ds: signature_namespace, + md: metadata_namespace + ).first.try(:content).to_s + end + hashable :unspecified_certificate + def signing_certificate xpath( "//md:SPSSODescriptor/md:KeyDescriptor[@use='signing']/ds:KeyInfo/ds:X509Data/ds:X509Certificate", diff --git a/lib/saml_idp/request.rb b/lib/saml_idp/request.rb index eb651145..bd84c5d8 100644 --- a/lib/saml_idp/request.rb +++ b/lib/saml_idp/request.rb @@ -3,6 +3,8 @@ require 'logger' module SamlIdp class Request + attr_accessor :errors + def self.from_deflated_request(raw, external_attributes = {}) if raw decoded = Base64.decode64(raw) @@ -34,6 +36,7 @@ def initialize(raw_xml = "", external_attributes = {}) self.relay_state = external_attributes[:relay_state] self.sig_algorithm = external_attributes[:sig_algorithm] self.signature = external_attributes[:signature] + self.errors = [] end def logout_request? @@ -89,37 +92,53 @@ def log(msg) end end + def collect_errors(error_type) + errors.push(error_type) + end + def valid?(external_attributes = {}) unless service_provider? log "Unable to find service provider for issuer #{issuer}" + collect_errors(:sp_not_found) return false end unless (authn_request? ^ logout_request?) log "One and only one of authnrequest and logout request is required. authnrequest: #{authn_request?} logout_request: #{logout_request?} " + collect_errors(:unaccepted_request) + return false + end + + if (logout_request? || validate_auth_request_signature?) && (service_provider.cert.to_s.empty? || !!service_provider.fingerprint.to_s.empty?) + log "Verifying request signature is required. But certificate and fingerprint was empty." + collect_errors(:empty_certificate) return false end # XML embedded signature if signature.nil? && !valid_signature? log "Requested document signature is invalid in #{raw_xml}" + collect_errors(:invalid_embedded_signature) return false end # URI query signature if signature.present? && !valid_external_signature? log "Requested URI signature is invalid in #{raw_xml}" + collect_errors(:invalid_external_signature) return false end if response_url.nil? log "Unable to find response url for #{issuer}: #{raw_xml}" + collect_errors(:empty_response_url) return false end if !service_provider.acceptable_response_hosts.include?(response_host) log "#{service_provider.acceptable_response_hosts} compare to #{response_host}" log "No acceptable AssertionConsumerServiceURL, either configure them via config.service_provider.response_hosts or match to your metadata_url host" + collect_errors(:not_allowed_host) return false end @@ -128,9 +147,7 @@ def valid?(external_attributes = {}) def valid_signature? # Force signatures for logout requests because there is no other protection against a cross-site DoS. - # Validate signature when metadata specify AuthnRequest should be signed - metadata = service_provider.current_metadata - if logout_request? || authn_request? && metadata.respond_to?(:sign_authn_request?) && metadata.sign_authn_request? + if logout_request? || authn_request? && validate_auth_request_signature? document.valid_signature?(service_provider.cert, service_provider.fingerprint) else true @@ -138,6 +155,8 @@ def valid_signature? end def valid_external_signature? + return true if authn_request? && !validate_auth_request_signature? + cert = OpenSSL::X509::Certificate.new(service_provider.cert) sha_version = sig_algorithm =~ /sha(.*?)$/i && $1.to_i @@ -151,7 +170,14 @@ def valid_external_signature? OpenSSL::Digest::SHA1 end - cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string) + result = cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string) + # Match all percent-encoded sequences (e.g., %20, %2B) and convert them to lowercase + # Upper case is recommended for consistency but some services such as MS Entra Id not follows it + # https://datatracker.ietf.org/doc/html/rfc3986#section-2.1 + result || cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string.gsub(/%[A-F0-9]{2}/) { |match| match.downcase }) + rescue OpenSSL::X509::CertificateError => e + log e.message + collect_errors(:cert_format_error) end def service_provider? @@ -232,5 +258,14 @@ def service_provider_finder config.service_provider.finder end private :service_provider_finder + + def validate_auth_request_signature? + # Validate signature when metadata specify AuthnRequest should be signed + metadata = service_provider.current_metadata + sign_authn_request = metadata.respond_to?(:sign_authn_request?) && metadata.sign_authn_request? + sign_authn_request = service_provider.sign_authn_request unless service_provider.sign_authn_request.nil? + sign_authn_request + end + private :validate_auth_request_signature? end end diff --git a/lib/saml_idp/service_provider.rb b/lib/saml_idp/service_provider.rb index eed24bfc..bedc1c33 100644 --- a/lib/saml_idp/service_provider.rb +++ b/lib/saml_idp/service_provider.rb @@ -11,6 +11,7 @@ class ServiceProvider attribute :fingerprint attribute :metadata_url attribute :validate_signature + attribute :sign_authn_request attribute :acs_url attribute :assertion_consumer_logout_service_url attribute :response_hosts diff --git a/saml_idp.gemspec b/saml_idp.gemspec index 49044ff4..1a332250 100644 --- a/saml_idp.gemspec +++ b/saml_idp.gemspec @@ -54,8 +54,8 @@ Gem::Specification.new do |s| s.add_development_dependency('appraisal') s.add_development_dependency('capybara', '>= 2.16') s.add_development_dependency('rails', '>= 5.2') - s.add_development_dependency('rake') s.add_development_dependency('debug') + s.add_development_dependency('rake') s.add_development_dependency('rspec', '>= 3.7.0') s.add_development_dependency('ruby-saml', '>= 1.7.2') s.add_development_dependency('simplecov') diff --git a/spec/lib/saml_idp/configurator_spec.rb b/spec/lib/saml_idp/configurator_spec.rb index 33141117..59c399a8 100644 --- a/spec/lib/saml_idp/configurator_spec.rb +++ b/spec/lib/saml_idp/configurator_spec.rb @@ -50,18 +50,21 @@ module SamlIdp context "logger initialization" do context 'when Rails has been properly initialized' do - it 'sets logger to Rails.logger' do - rails_logger = double("Rails.logger") - stub_const("Rails", double(logger: rails_logger)) + before do + stub_const("Rails", double(logger: double("Rails.logger"))) + end + it 'sets logger to Rails.logger' do expect(subject.logger).to eq(Rails.logger) end end context 'when Rails is not fully initialized' do - it 'sets logger to a lambda' do - stub_const("Rails", Class.new) + before do + stub_const("Rails", Class.new) + end + it 'sets logger to a lambda' do expect(subject.logger).to be_a(Proc) expect { subject.logger.call("test") }.to output("test\n").to_stdout end @@ -75,6 +78,10 @@ module SamlIdp expect { subject.logger.call("test") }.to output("test\n").to_stdout end end + + after do + hide_const("Rails") + end end end end diff --git a/spec/lib/saml_idp/controller_spec.rb b/spec/lib/saml_idp/controller_spec.rb index 735cdaae..94e189e9 100644 --- a/spec/lib/saml_idp/controller_spec.rb +++ b/spec/lib/saml_idp/controller_spec.rb @@ -81,16 +81,6 @@ def params expect(response.is_valid?).to be_truthy end - it "should create a SAML Logout Response" do - params[:SAMLRequest] = make_saml_logout_request - expect(validate_saml_request).to eq(true) - expect(saml_request.logout_request?).to eq true - saml_response = encode_response(principal) - response = OneLogin::RubySaml::Logoutresponse.new(saml_response, saml_settings) - expect(response.validate).to eq(true) - expect(response.issuer).to eq("http://example.com") - end - it "should by default create a SAML Response with a signed assertion" do saml_response = encode_response(principal) response = OneLogin::RubySaml::Response.new(saml_response) @@ -128,7 +118,7 @@ def params context "Single Logout Request" do before do idp_configure("https://foo.example.com/saml/consume", true) - slo_request = make_saml_sp_slo_request + slo_request = make_saml_sp_slo_request(security_options: { embed_sign: false }) params[:SAMLRequest] = slo_request['SAMLRequest'] params[:RelayState] = slo_request['RelayState'] params[:SigAlg] = slo_request['SigAlg'] @@ -138,5 +128,18 @@ def params it 'should successfully validate signature' do expect(validate_saml_request).to eq(true) end + + context "solicited Response" do + let(:principal) { double email_address: "foo@example.com" } + + it "should create a SAML Logout Response" do + expect(validate_saml_request).to eq(true) + expect(saml_request.logout_request?).to eq true + saml_response = encode_response(principal) + response = OneLogin::RubySaml::Logoutresponse.new(saml_response, saml_settings) + expect(response.validate).to eq(true) + expect(response.issuer).to eq("http://idp.com/saml/idp") + end + end end end diff --git a/spec/lib/saml_idp/incoming_metadata_spec.rb b/spec/lib/saml_idp/incoming_metadata_spec.rb index a966e17d..fb00ec53 100644 --- a/spec/lib/saml_idp/incoming_metadata_spec.rb +++ b/spec/lib/saml_idp/incoming_metadata_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' + module SamlIdp metadata_1 = <<-eos @@ -29,6 +30,48 @@ module SamlIdp eos + metadata_5 = <<-eos + + + + + + MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnht3GR... + + + + + + eos + + metadata_6 = <<-eos + + + + + + MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAmw6vGr... + + + + + + eos + + metadata_7 = <<-eos + + + + + + MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1dX3Gr... + + + + + + eos + describe IncomingMetadata do it 'should properly set sign_assertions to false' do metadata = SamlIdp::IncomingMetadata.new(metadata_1) @@ -55,5 +98,37 @@ module SamlIdp metadata = SamlIdp::IncomingMetadata.new(metadata_4) expect(metadata.sign_authn_request).to eq(false) end + + it 'should properly set unspecified_certificate when present' do + metadata = SamlIdp::IncomingMetadata.new(metadata_5) + expect(metadata.unspecified_certificate).to eq('MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnht3GR...') + end + + it 'should return empty unspecified_certificate when not present' do + metadata = SamlIdp::IncomingMetadata.new(metadata_1) + expect(metadata.unspecified_certificate).to eq('') + end + + it 'should properly set signing_certificate when present but not unspecified_certificate' do + metadata = SamlIdp::IncomingMetadata.new(metadata_6) + expect(metadata.signing_certificate).to eq('MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAmw6vGr...') + expect(metadata.unspecified_certificate).to eq('') + end + + it 'should return empty signing_certificate when not present' do + metadata = SamlIdp::IncomingMetadata.new(metadata_1) + expect(metadata.signing_certificate).to eq('') + end + + it 'should properly set encryption_certificate when present but not unspecified_certificate' do + metadata = SamlIdp::IncomingMetadata.new(metadata_7) + expect(metadata.encryption_certificate).to eq('MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1dX3Gr...') + expect(metadata.unspecified_certificate).to eq('') + end + + it 'should return empty encryption_certificate when not present' do + metadata = SamlIdp::IncomingMetadata.new(metadata_1) + expect(metadata.encryption_certificate).to eq('') + end end end diff --git a/spec/lib/saml_idp/request_spec.rb b/spec/lib/saml_idp/request_spec.rb index 794fc361..151e391a 100644 --- a/spec/lib/saml_idp/request_spec.rb +++ b/spec/lib/saml_idp/request_spec.rb @@ -1,189 +1,195 @@ require 'spec_helper' -module SamlIdp - describe Request do - let(:issuer) { 'localhost:3000' } - let(:raw_authn_request) do - "#{issuer}urn:oasis:names:tc:SAML:2.0:ac:classes:Password" - end - describe "deflated request" do - let(:deflated_request) { Base64.encode64(Zlib::Deflate.deflate(raw_authn_request, 9)[2..-5]) } +RSpec.describe SamlIdp::Request, type: :model do + let(:valid_saml_request) { make_saml_request("https://foo.example.com/saml/consume", true) } + let(:valid_logout_request) { make_saml_sp_slo_request(security_options: { embed_sign: true })['SAMLRequest'] } + let(:invalid_saml_request) { "invalid_saml_request" } + let(:external_attributes) { { saml_request: valid_saml_request, relay_state: "state" } } - subject { described_class.from_deflated_request deflated_request } + describe ".from_deflated_request" do + context "when request is valid and deflated" do + it "inflates and decodes the request" do + request = SamlIdp::Request.from_deflated_request(valid_saml_request) - it "inflates" do - expect(subject.request_id).to eq("_af43d1a0-e111-0130-661a-3c0754403fdb") + expect { Saml::XML::Document.parse(request.raw_xml) }.not_to raise_error end + end - it "handles invalid SAML" do - req = described_class.from_deflated_request "bang!" - expect(req.valid?).to eq(false) + context "when request is invalid" do + it "returns an empty inflated string" do + request = SamlIdp::Request.from_deflated_request(nil) + expect(request.raw_xml).to eq("") end end + end - describe "authn request" do - subject { described_class.new raw_authn_request } + describe "#logout_request?" do + it "returns true for a valid logout request" do + request = SamlIdp::Request.from_deflated_request(valid_logout_request) + expect(request.logout_request?).to be true + end - it "has a valid request_id" do - expect(subject.request_id).to eq("_af43d1a0-e111-0130-661a-3c0754403fdb") - end + it "returns false for a non-logout request" do + request = SamlIdp::Request.from_deflated_request(valid_saml_request) + expect(request.logout_request?).to be false + end + end - it "has a valid acs_url" do - expect(subject.acs_url).to eq("http://localhost:3000/saml/consume") - end + describe "#authn_request?" do + it "returns true for a valid authn request" do + request = SamlIdp::Request.from_deflated_request(valid_saml_request) + expect(request.authn_request?).to be true + end - it "has a valid service_provider" do - expect(subject.service_provider).to be_a ServiceProvider - end + it "returns false for a non-authn request" do + request = SamlIdp::Request.from_deflated_request(valid_logout_request) + expect(request.authn_request?).to be false + end + end - it "has a valid service_provider" do - expect(subject.service_provider).to be_truthy - end + describe "#valid?" do + let(:sp_issuer) { "test_issuer" } + let(:valid_service_provider) do + instance_double( + "SamlIdp::ServiceProvider", + valid?: true, + acs_url: 'https://foo.example.com/saml/consume', + current_metadata: instance_double("Metadata", sign_authn_request?: true), + assertion_consumer_logout_service_url: 'https://foo.example.com/saml/logout', + sign_authn_request: true, + acceptable_response_hosts: ["foo.example.com"], + cert: sp_x509_cert, + fingerprint: SamlIdp::Fingerprint.certificate_digest(sp_x509_cert, :sha256), + ) + end + + before do + allow_any_instance_of(SamlIdp::Request).to receive(:service_provider).and_return(valid_service_provider) + allow_any_instance_of(SamlIdp::Request).to receive(:issuer).and_return(sp_issuer) + end - it "has a valid issuer" do - expect(subject.issuer).to eq("localhost:3000") + context "when the request is valid" do + it "returns true for a valid authn request" do + request = SamlIdp::Request.from_deflated_request(valid_saml_request) + expect(request.errors).to be_empty + expect(request.valid?).to be true end - it "has a valid valid_signature" do - expect(subject.valid_signature?).to be_truthy + it "returns true for a valid logout request" do + request = SamlIdp::Request.from_deflated_request(valid_logout_request) + expect(request.errors).to be_empty + expect(request.valid?).to be true end + end - it "should return acs_url for response_url" do - expect(subject.response_url).to eq(subject.acs_url) + context 'when signature provided as external param' do + let!(:uri_query) { make_saml_sp_slo_request(security_options: { embed_sign: false }) } + let(:raw_saml_request) { uri_query['SAMLRequest'] } + let(:relay_state) { uri_query['RelayState'] } + let(:siging_algorithm) { uri_query['SigAlg'] } + let(:signature) { uri_query['Signature'] } + + subject do + described_class.from_deflated_request( + raw_saml_request, + saml_request: raw_saml_request, + relay_state: relay_state, + sig_algorithm: siging_algorithm, + signature: signature + ) end - it "is a authn request" do - expect(subject.authn_request?).to eq(true) + it "should validate the request" do + expect(subject.valid_external_signature?).to be true + expect(subject.errors).to be_empty end - it "fetches internal request" do - expect(subject.request['ID']).to eq(subject.request_id) + it "should collect errors when the signature is invalid" do + allow(subject).to receive(:valid_external_signature?).and_return(false) + expect(subject.valid?).to eq(false) + expect(subject.errors).to include(:invalid_external_signature) end + end - it 'has a valid authn context' do - expect(subject.requested_authn_context).to eq('urn:oasis:names:tc:SAML:2.0:ac:classes:Password') - end + context "when the service provider is invalid" do + it "returns false and logs an error" do + allow_any_instance_of(SamlIdp::Request).to receive(:service_provider?).and_return(false) + request = SamlIdp::Request.from_deflated_request(valid_saml_request) - context 'the issuer is empty' do - let(:issuer) { nil } - let(:logger) { ->(msg) { puts msg } } - - before do - allow(SamlIdp.config).to receive(:logger).and_return(logger) - end - - it 'is invalid' do - expect(subject.issuer).to_not eq('') - expect(subject.issuer).to be_nil - expect(subject.valid?).to eq(false) - end - - context 'a Ruby Logger is configured' do - let(:logger) { Logger.new($stdout) } - - before do - allow(logger).to receive(:info) - end - - it 'logs an error message' do - expect(subject.valid?).to be false - expect(logger).to have_received(:info).with('Unable to find service provider for issuer ') - end - end - - context 'a Logger-like logger is configured' do - let(:logger) do - Class.new { - def info(msg); end - }.new - end - - before do - allow(logger).to receive(:info) - end - - it 'logs an error message' do - expect(subject.valid?).to be false - expect(logger).to have_received(:info).with('Unable to find service provider for issuer ') - end - end - - context 'a logger lambda is configured' do - let(:logger) { double } - - before { allow(logger).to receive(:call) } - - it 'logs an error message' do - expect(subject.valid?).to be false - expect(logger).to have_received(:call).with('Unable to find service provider for issuer ') - end - end + expect(request.valid?).to be false + expect(request.errors).to include(:sp_not_found) end end - describe "logout request" do - context 'when POST binding' do - let(:raw_logout_request) { "http://example.comsome_name_idabc123index" } - - subject { described_class.new raw_logout_request } + context "when empty certificate for authn request validation" do + let(:valid_service_provider) do + instance_double( + "SamlIdp::ServiceProvider", + valid?: true, + acs_url: 'https://foo.example.com/saml/consume', + current_metadata: instance_double("Metadata", sign_authn_request?: true), + assertion_consumer_logout_service_url: 'https://foo.example.com/saml/logout', + sign_authn_request: true, + acceptable_response_hosts: ["foo.example.com"], + cert: nil, + fingerprint: nil, + ) + end + it "returns false and logs an error" do + request = SamlIdp::Request.from_deflated_request(valid_saml_request) - it "has a valid request_id" do - expect(subject.request_id).to eq('_some_response_id') - end + expect(request.valid?).to be false + expect(request.errors).to include(:empty_certificate) + end + end - it "should be flagged as a logout_request" do - expect(subject.logout_request?).to eq(true) - end + context "when empty certificate for logout validation" do + let(:valid_service_provider) do + instance_double( + "SamlIdp::ServiceProvider", + valid?: true, + acs_url: 'https://foo.example.com/saml/consume', + current_metadata: instance_double("Metadata", sign_authn_request?: true), + assertion_consumer_logout_service_url: 'https://foo.example.com/saml/logout', + sign_authn_request: true, + acceptable_response_hosts: ["foo.example.com"], + cert: nil, + fingerprint: nil, + ) + end - it "should have a valid name_id" do - expect(subject.name_id).to eq('some_name_id') - end + before do + allow_any_instance_of(SamlIdp::Request).to receive(:authn_request?).and_return(false) + allow_any_instance_of(SamlIdp::Request).to receive(:logout_request?).and_return(true) + end - it "should have a session index" do - expect(subject.session_index).to eq('abc123index') - end + it "returns false and logs an error" do + request = SamlIdp::Request.from_deflated_request(valid_saml_request) - it "should have a valid issuer" do - expect(subject.issuer).to eq('http://example.com') - end + expect(request.valid?).to be false + expect(request.errors).to include(:empty_certificate) + end + end - it "fetches internal request" do - expect(subject.request['ID']).to eq(subject.request_id) - end + context "when both authn and logout requests are present" do + it "returns false and logs an error" do + allow_any_instance_of(SamlIdp::Request).to receive(:authn_request?).and_return(true) + allow_any_instance_of(SamlIdp::Request).to receive(:logout_request?).and_return(true) + request = SamlIdp::Request.from_deflated_request(valid_saml_request) - it "should return logout_url for response_url" do - expect(subject.response_url).to eq(subject.logout_url) - end + expect(request.valid?).to be false + expect(request.errors).to include(:unaccepted_request) end + end + + context "when the signature is invalid" do + it "returns false and logs an error" do + allow_any_instance_of(SamlIdp::Request).to receive(:valid_signature?).and_return(false) + allow_any_instance_of(SamlIdp::Request).to receive(:log) + request = SamlIdp::Request.from_deflated_request(valid_saml_request) - context 'when signature provided as external param' do - let!(:uri_query) { make_saml_sp_slo_request } - let(:raw_saml_request) { uri_query['SAMLRequest'] } - let(:relay_state) { uri_query['RelayState'] } - let(:siging_algorithm) { uri_query['SigAlg'] } - let(:signature) { uri_query['Signature'] } - - subject do - described_class.from_deflated_request( - raw_saml_request, - saml_request: raw_saml_request, - relay_state: relay_state, - sig_algorithm: siging_algorithm, - signature: signature - ) - end - - it "should validate the request" do - allow(ServiceProvider).to receive(:new).and_return( - ServiceProvider.new( - issuer: "http://example.com/issuer", - cert: sp_x509_cert, - response_hosts: ["example.com"], - assertion_consumer_logout_service_url: "http://example.com/logout" - ) - ) - expect(subject.valid?).to be true - end + expect(request.valid?).to be false + expect(request.errors).to include(:invalid_embedded_signature) end end end diff --git a/spec/support/saml_request_macros.rb b/spec/support/saml_request_macros.rb index 4f3049ad..d4abf3d6 100644 --- a/spec/support/saml_request_macros.rb +++ b/spec/support/saml_request_macros.rb @@ -18,10 +18,9 @@ def make_saml_logout_request(requested_saml_logout_url = 'https://foo.example.co Base64.strict_encode64(request_builder.signed) end - def make_saml_sp_slo_request(param_type: true, embed_sign: false) + def make_saml_sp_slo_request(param_type: true, security_options: {}) logout_request = OneLogin::RubySaml::Logoutrequest.new - saml_sp_setting = saml_settings("https://foo.example.com/saml/consume") - add_securty_options(saml_sp_setting, embed_sign: embed_sign) + saml_sp_setting = saml_settings("https://foo.example.com/saml/consume", true, security_options: security_options) if param_type logout_request.create_params(saml_sp_setting, 'RelayState' => 'https://foo.example.com/home') else @@ -34,7 +33,7 @@ def generate_sp_metadata(saml_acs_url = "https://foo.example.com/saml/consume", sp_metadata.generate(saml_settings(saml_acs_url, enable_secure_options), true) end - def saml_settings(saml_acs_url = "https://foo.example.com/saml/consume", enable_secure_options = false) + def saml_settings(saml_acs_url = "https://foo.example.com/saml/consume", enable_secure_options = false, security_options: {}) settings = OneLogin::RubySaml::Settings.new settings.assertion_consumer_service_url = saml_acs_url settings.issuer = "http://example.com/issuer" @@ -43,28 +42,22 @@ def saml_settings(saml_acs_url = "https://foo.example.com/saml/consume", enable_ settings.assertion_consumer_logout_service_url = 'https://foo.example.com/saml/logout' settings.idp_cert_fingerprint = SamlIdp::Default::FINGERPRINT settings.name_identifier_format = SamlIdp::Default::NAME_ID_FORMAT - add_securty_options(settings) if enable_secure_options + add_securty_options(settings, default_sp_security_options.merge!(security_options)) if enable_secure_options settings end - def add_securty_options(settings, authn_requests_signed: true, - embed_sign: true, - logout_requests_signed: true, - logout_responses_signed: true, - digest_method: XMLSecurity::Document::SHA256, - signature_method: XMLSecurity::Document::RSA_SHA256, - assertions_signed: true) + def add_securty_options(settings, options = default_sp_security_options) # Security section settings.idp_cert = SamlIdp::Default::X509_CERTIFICATE # Signed embedded singature - settings.security[:authn_requests_signed] = authn_requests_signed - settings.security[:embed_sign] = embed_sign - settings.security[:logout_requests_signed] = logout_requests_signed - settings.security[:logout_responses_signed] = logout_responses_signed - settings.security[:metadata_signed] = digest_method - settings.security[:digest_method] = digest_method - settings.security[:signature_method] = signature_method - settings.security[:want_assertions_signed] = assertions_signed + settings.security[:authn_requests_signed] = options[:authn_requests_signed] + settings.security[:embed_sign] = options[:embed_sign] + settings.security[:logout_requests_signed] = options[:logout_requests_signed] + settings.security[:logout_responses_signed] = options[:logout_responses_signed] + settings.security[:metadata_signed] = options[:digest_method] + settings.security[:digest_method] = options[:digest_method] + settings.security[:signature_method] = options[:signature_method] + settings.security[:want_assertions_signed] = options[:assertions_signed] settings.private_key = sp_pv_key settings.certificate = sp_x509_cert end @@ -120,4 +113,27 @@ def print_pretty_xml(xml_string) doc.write(outbuf, 1) puts outbuf end + + def decode_saml_request(saml_request) + decoded_request = Base64.decode64(saml_request) + begin + # Try to decompress, since SAMLRequest might be compressed + Zlib::Inflate.new(-Zlib::MAX_WBITS).inflate(decoded_request) + rescue Zlib::DataError + # If it's not compressed, just return the decoded request + decoded_request + end + end + + def default_sp_security_options + { + authn_requests_signed: true, + embed_sign: true, + logout_requests_signed: true, + logout_responses_signed: true, + digest_method: XMLSecurity::Document::SHA256, + signature_method: XMLSecurity::Document::RSA_SHA256, + assertions_signed: true + } + end end diff --git a/spec/support/security_helpers.rb b/spec/support/security_helpers.rb index 2deb38ae..f6952ce7 100644 --- a/spec/support/security_helpers.rb +++ b/spec/support/security_helpers.rb @@ -51,8 +51,8 @@ def signature_fingerprint_1 @signature_fingerprint1 ||= "C5:19:85:D9:47:F1:BE:57:08:20:25:05:08:46:EB:27:F6:CA:B7:83" end - def signature_1 - @signature1 ||= File.read(File.join(File.dirname(__FILE__), 'certificates', 'certificate1')) + def certificate_1 + @certificate_1 ||= File.read(File.join(File.dirname(__FILE__), 'certificates', 'certificate1')) end def r1_signature_2