From 3e58ea24348c2ba96bca3f138e728310ff8c6e82 Mon Sep 17 00:00:00 2001 From: zogoo Date: Thu, 25 Jul 2024 00:50:53 +0800 Subject: [PATCH 1/5] Squash commits for saml_idp gem --- lib/saml_idp.rb | 4 +- lib/saml_idp/controller.rb | 12 ++- lib/saml_idp/request.rb | 51 +++++++++++-- lib/saml_idp/xml_security.rb | 33 +++++---- spec/lib/saml_idp/controller_spec.rb | 15 ++++ spec/lib/saml_idp/incoming_metadata_spec.rb | 1 - spec/lib/saml_idp/metadata_builder_spec.rb | 2 +- spec/lib/saml_idp/request_spec.rb | 74 +++++++++++++------ .../app/views/saml_idp/idp/new.html.erb | 3 + spec/support/saml_request_macros.rb | 15 +++- spec/xml_security_spec.rb | 18 +++-- 11 files changed, 170 insertions(+), 58 deletions(-) diff --git a/lib/saml_idp.rb b/lib/saml_idp.rb index 1e8532f2..99df05e1 100644 --- a/lib/saml_idp.rb +++ b/lib/saml_idp.rb @@ -70,9 +70,9 @@ def signed? !!xpath("//ds:Signature", ds: signature_namespace).first end - def valid_signature?(fingerprint) + def valid_signature?(certificate, fingerprint) signed? && - signed_document.validate(fingerprint, :soft) + signed_document.validate(certificate, fingerprint, :soft) end def signed_document diff --git a/lib/saml_idp/controller.rb b/lib/saml_idp/controller.rb index e2bf25e7..d5bd20c9 100644 --- a/lib/saml_idp/controller.rb +++ b/lib/saml_idp/controller.rb @@ -33,15 +33,21 @@ def acs_url end def validate_saml_request(raw_saml_request = params[:SAMLRequest]) - decode_request(raw_saml_request) + decode_request(raw_saml_request, params[:Signature], params[:SigAlg], params[:RelayState]) return true if valid_saml_request? head :forbidden if defined?(::Rails) false end - def decode_request(raw_saml_request) - @saml_request = Request.from_deflated_request(raw_saml_request) + def decode_request(raw_saml_request, signature, sig_algorithm, relay_state) + @saml_request = Request.from_deflated_request( + raw_saml_request, + saml_request: raw_saml_request, + signature: signature, + sig_algorithm: sig_algorithm, + relay_state: relay_state + ) end def authn_context_classref diff --git a/lib/saml_idp/request.rb b/lib/saml_idp/request.rb index 4b8b891f..eb651145 100644 --- a/lib/saml_idp/request.rb +++ b/lib/saml_idp/request.rb @@ -3,7 +3,7 @@ require 'logger' module SamlIdp class Request - def self.from_deflated_request(raw) + def self.from_deflated_request(raw, external_attributes = {}) if raw decoded = Base64.decode64(raw) zstream = Zlib::Inflate.new(-Zlib::MAX_WBITS) @@ -18,18 +18,22 @@ def self.from_deflated_request(raw) else inflated = "" end - new(inflated) + new(inflated, external_attributes) end - attr_accessor :raw_xml + attr_accessor :raw_xml, :saml_request, :signature, :sig_algorithm, :relay_state delegate :config, to: :SamlIdp private :config delegate :xpath, to: :document private :xpath - def initialize(raw_xml = "") + def initialize(raw_xml = "", external_attributes = {}) self.raw_xml = raw_xml + self.saml_request = external_attributes[:saml_request] + self.relay_state = external_attributes[:relay_state] + self.sig_algorithm = external_attributes[:sig_algorithm] + self.signature = external_attributes[:signature] end def logout_request? @@ -85,7 +89,7 @@ def log(msg) end end - def valid? + def valid?(external_attributes = {}) unless service_provider? log "Unable to find service provider for issuer #{issuer}" return false @@ -96,8 +100,15 @@ def valid? return false end - unless valid_signature? - log "Signature is invalid in #{raw_xml}" + # XML embedded signature + if signature.nil? && !valid_signature? + log "Requested document signature is invalid in #{raw_xml}" + return false + end + + # URI query signature + if signature.present? && !valid_external_signature? + log "Requested URI signature is invalid in #{raw_xml}" return false end @@ -120,12 +131,29 @@ def valid_signature? # 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? - document.valid_signature?(service_provider.fingerprint) + document.valid_signature?(service_provider.cert, service_provider.fingerprint) else true end end + def valid_external_signature? + cert = OpenSSL::X509::Certificate.new(service_provider.cert) + + sha_version = sig_algorithm =~ /sha(.*?)$/i && $1.to_i + raw_signature = Base64.decode64(signature) + + signature_algorithm = case sha_version + when 256 then OpenSSL::Digest::SHA256 + when 384 then OpenSSL::Digest::SHA384 + when 512 then OpenSSL::Digest::SHA512 + else + OpenSSL::Digest::SHA1 + end + + cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string) + end + def service_provider? service_provider && service_provider.valid? end @@ -148,6 +176,13 @@ def session_index @_session_index ||= xpath("//samlp:SessionIndex", samlp: samlp).first.try(:content) end + def query_request_string + url_string = "SAMLRequest=#{CGI.escape(saml_request)}" + url_string << "&RelayState=#{CGI.escape(relay_state)}" if relay_state + url_string << "&SigAlg=#{CGI.escape(sig_algorithm)}" + end + private :query_request_string + def response_host uri = URI(response_url) if uri diff --git a/lib/saml_idp/xml_security.rb b/lib/saml_idp/xml_security.rb index 640c0348..0a223ad2 100644 --- a/lib/saml_idp/xml_security.rb +++ b/lib/saml_idp/xml_security.rb @@ -43,24 +43,29 @@ def initialize(response) extract_signed_element_id end - def validate(idp_cert_fingerprint, soft = true) + def validate(idp_base64_cert, idp_cert_fingerprint, soft = true) # get cert from response cert_element = REXML::XPath.first(self, "//ds:X509Certificate", { "ds"=>DSIG }) - raise ValidationError.new("Certificate element missing in response (ds:X509Certificate)") unless cert_element - base64_cert = cert_element.text - cert_text = Base64.decode64(base64_cert) - cert = OpenSSL::X509::Certificate.new(cert_text) - - # check cert matches registered idp cert - fingerprint = fingerprint_cert(cert) - sha1_fingerprint = fingerprint_cert_sha1(cert) - plain_idp_cert_fingerprint = idp_cert_fingerprint.gsub(/[^a-zA-Z0-9]/,"").downcase - - if fingerprint != plain_idp_cert_fingerprint && sha1_fingerprint != plain_idp_cert_fingerprint - return soft ? false : (raise ValidationError.new("Fingerprint mismatch")) + if cert_element + idp_base64_cert = cert_element.text + cert_text = Base64.decode64(idp_base64_cert) + cert = OpenSSL::X509::Certificate.new(cert_text) + + # check cert matches registered idp cert + fingerprint = fingerprint_cert(cert) + sha1_fingerprint = fingerprint_cert_sha1(cert) + plain_idp_cert_fingerprint = idp_cert_fingerprint.gsub(/[^a-zA-Z0-9]/,"").downcase + + if fingerprint != plain_idp_cert_fingerprint && sha1_fingerprint != plain_idp_cert_fingerprint + return soft ? false : (raise ValidationError.new("Fingerprint mismatch")) + end + end + + if idp_base64_cert.nil? || idp_base64_cert.empty? + raise ValidationError.new("Certificate validation is required, but it doesn't exist.") end - validate_doc(base64_cert, soft) + validate_doc(idp_base64_cert, soft) end def fingerprint_cert(cert) diff --git a/spec/lib/saml_idp/controller_spec.rb b/spec/lib/saml_idp/controller_spec.rb index 883e0dba..4d63f7a1 100644 --- a/spec/lib/saml_idp/controller_spec.rb +++ b/spec/lib/saml_idp/controller_spec.rb @@ -124,4 +124,19 @@ def params end end end + + context "Single Logout Request" do + before do + idp_configure("https://foo.example.com/saml/consume", true) + slo_request = make_saml_sp_slo_request + params[:SAMLRequest] = slo_request['SAMLRequest'] + params[:RelayState] = slo_request['RelayState'] + params[:SigAlg] = slo_request['SigAlg'] + params[:Signature] = slo_request['Signature'] + end + + it 'should successfully validate signature' do + expect(validate_saml_request).to eq(true) + end + end end diff --git a/spec/lib/saml_idp/incoming_metadata_spec.rb b/spec/lib/saml_idp/incoming_metadata_spec.rb index 7d483e0b..a966e17d 100644 --- a/spec/lib/saml_idp/incoming_metadata_spec.rb +++ b/spec/lib/saml_idp/incoming_metadata_spec.rb @@ -33,7 +33,6 @@ module SamlIdp it 'should properly set sign_assertions to false' do metadata = SamlIdp::IncomingMetadata.new(metadata_1) expect(metadata.sign_assertions).to eq(false) - expect(metadata.sign_authn_request).to eq(false) end it 'should properly set entity_id as https://test-saml.com/saml' do diff --git a/spec/lib/saml_idp/metadata_builder_spec.rb b/spec/lib/saml_idp/metadata_builder_spec.rb index c8e14765..453a8d81 100644 --- a/spec/lib/saml_idp/metadata_builder_spec.rb +++ b/spec/lib/saml_idp/metadata_builder_spec.rb @@ -6,7 +6,7 @@ module SamlIdp end it "signs valid xml" do - expect(Saml::XML::Document.parse(subject.signed).valid_signature?(Default::FINGERPRINT)).to be_truthy + expect(Saml::XML::Document.parse(subject.signed).valid_signature?("", Default::FINGERPRINT)).to be_truthy end it "includes logout element" do diff --git a/spec/lib/saml_idp/request_spec.rb b/spec/lib/saml_idp/request_spec.rb index 47711539..794fc361 100644 --- a/spec/lib/saml_idp/request_spec.rb +++ b/spec/lib/saml_idp/request_spec.rb @@ -122,36 +122,68 @@ def info(msg); end end describe "logout request" do - let(:raw_logout_request) { "http://example.comsome_name_idabc123index" } + context 'when POST binding' do + let(:raw_logout_request) { "http://example.comsome_name_idabc123index" } - subject { described_class.new raw_logout_request } + subject { described_class.new raw_logout_request } - it "has a valid request_id" do - expect(subject.request_id).to eq('_some_response_id') - end + it "has a valid request_id" do + expect(subject.request_id).to eq('_some_response_id') + end - it "should be flagged as a logout_request" do - expect(subject.logout_request?).to eq(true) - end + it "should be flagged as a logout_request" do + expect(subject.logout_request?).to eq(true) + end - it "should have a valid name_id" do - expect(subject.name_id).to eq('some_name_id') - end + it "should have a valid name_id" do + expect(subject.name_id).to eq('some_name_id') + end - it "should have a session index" do - expect(subject.session_index).to eq('abc123index') - end + it "should have a session index" do + expect(subject.session_index).to eq('abc123index') + end - it "should have a valid issuer" do - expect(subject.issuer).to eq('http://example.com') - end + it "should have a valid issuer" do + expect(subject.issuer).to eq('http://example.com') + end - it "fetches internal request" do - expect(subject.request['ID']).to eq(subject.request_id) + it "fetches internal request" do + expect(subject.request['ID']).to eq(subject.request_id) + end + + it "should return logout_url for response_url" do + expect(subject.response_url).to eq(subject.logout_url) + end end - it "should return logout_url for response_url" do - expect(subject.response_url).to eq(subject.logout_url) + 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 end end end diff --git a/spec/rails_app/app/views/saml_idp/idp/new.html.erb b/spec/rails_app/app/views/saml_idp/idp/new.html.erb index c71d85ab..01c86199 100644 --- a/spec/rails_app/app/views/saml_idp/idp/new.html.erb +++ b/spec/rails_app/app/views/saml_idp/idp/new.html.erb @@ -4,6 +4,9 @@ <%= form_tag do %> <%= hidden_field_tag("SAMLRequest", params[:SAMLRequest]) %> <%= hidden_field_tag("RelayState", params[:RelayState]) %> + <%= hidden_field_tag("SigAlg", params[:SigAlg]) %> + <%= hidden_field_tag("Signature", params[:Signature]) %> +

<%= label_tag :email %> <%= email_field_tag :email, params[:email], :autocapitalize => "off", :autocorrect => "off", :autofocus => "autofocus", :spellcheck => "false", :size => 30, :class => "email_pwd txt" %> diff --git a/spec/support/saml_request_macros.rb b/spec/support/saml_request_macros.rb index d587cf68..cd399346 100644 --- a/spec/support/saml_request_macros.rb +++ b/spec/support/saml_request_macros.rb @@ -18,6 +18,17 @@ 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) + 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) + if param_type + logout_request.create_params(saml_sp_setting, 'RelayState' => 'https://foo.example.com/home') + else + logout_request.create(saml_sp_setting, 'RelayState' => 'https://foo.example.com/home') + end + end + def generate_sp_metadata(saml_acs_url = "https://foo.example.com/saml/consume", enable_secure_options = false) sp_metadata = OneLogin::RubySaml::Metadata.new sp_metadata.generate(saml_settings(saml_acs_url, enable_secure_options), true) @@ -28,6 +39,7 @@ def saml_settings(saml_acs_url = "https://foo.example.com/saml/consume", enable_ settings.assertion_consumer_service_url = 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/slo" 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 @@ -84,7 +96,8 @@ def idp_configure(saml_acs_url = "https://foo.example.com/saml/consume", enable_ response_hosts: [URI(saml_acs_url).host], acs_url: saml_acs_url, cert: sp_x509_cert, - fingerprint: SamlIdp::Fingerprint.certificate_digest(sp_x509_cert) + fingerprint: SamlIdp::Fingerprint.certificate_digest(sp_x509_cert), + assertion_consumer_logout_service_url: 'https://foo.example.com/saml/logout' } } end diff --git a/spec/xml_security_spec.rb b/spec/xml_security_spec.rb index 7b7bf3c4..276dfcef 100644 --- a/spec/xml_security_spec.rb +++ b/spec/xml_security_spec.rb @@ -19,7 +19,7 @@ module SamlIdp end it "it raise Fingerprint mismatch" do - expect { document.validate("no:fi:ng:er:pr:in:t", false) }.to( + expect { document.validate("", "no:fi:ng:er:pr:in:t", false) }.to( raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, "Fingerprint mismatch") ) end @@ -45,10 +45,10 @@ module SamlIdp response = Base64.decode64(response_document) response.sub!(/.*<\/ds:X509Certificate>/, "") document = XMLSecurity::SignedDocument.new(response) - expect { document.validate("a fingerprint", false) }.to( + expect { document.validate("", "a fingerprint", false) }.to( raise_error( SamlIdp::XMLSecurity::SignedDocument::ValidationError, - "Certificate element missing in response (ds:X509Certificate)" + "Certificate validation is required, but it doesn't exist." ) ) end @@ -57,22 +57,26 @@ module SamlIdp describe "Algorithms" do it "validate using SHA1" do document = XMLSecurity::SignedDocument.new(fixture(:adfs_response_sha1, false)) - expect(document.validate("F1:3C:6B:80:90:5A:03:0E:6C:91:3E:5D:15:FA:DD:B0:16:45:48:72")).to be_truthy + base64cert = document.elements["//ds:X509Certificate"].text + expect(document.validate(base64cert, "F1:3C:6B:80:90:5A:03:0E:6C:91:3E:5D:15:FA:DD:B0:16:45:48:72")).to be_truthy end it "validate using SHA256" do document = XMLSecurity::SignedDocument.new(fixture(:adfs_response_sha256, false)) - expect(document.validate("28:74:9B:E8:1F:E8:10:9C:A8:7C:A9:C3:E3:C5:01:6C:92:1C:B4:BA")).to be_truthy + base64cert = document.elements["//ds:X509Certificate"].text + expect(document.validate(base64cert, "28:74:9B:E8:1F:E8:10:9C:A8:7C:A9:C3:E3:C5:01:6C:92:1C:B4:BA")).to be_truthy end it "validate using SHA384" do document = XMLSecurity::SignedDocument.new(fixture(:adfs_response_sha384, false)) - expect(document.validate("F1:3C:6B:80:90:5A:03:0E:6C:91:3E:5D:15:FA:DD:B0:16:45:48:72")).to be_truthy + base64cert = document.elements["//ds:X509Certificate"].text + expect(document.validate(base64cert, "F1:3C:6B:80:90:5A:03:0E:6C:91:3E:5D:15:FA:DD:B0:16:45:48:72")).to be_truthy end it "validate using SHA512" do document = XMLSecurity::SignedDocument.new(fixture(:adfs_response_sha512, false)) - expect(document.validate("F1:3C:6B:80:90:5A:03:0E:6C:91:3E:5D:15:FA:DD:B0:16:45:48:72")).to be_truthy + base64cert = document.elements["//ds:X509Certificate"].text + expect(document.validate(base64cert, "F1:3C:6B:80:90:5A:03:0E:6C:91:3E:5D:15:FA:DD:B0:16:45:48:72")).to be_truthy end end From f151f07122b030db61cb4bf37adf98deb222ce44 Mon Sep 17 00:00:00 2001 From: zogoo Date: Sat, 27 Jul 2024 21:12:53 +0800 Subject: [PATCH 2/5] [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 --- lib/saml_idp/request.rb | 15 ++- lib/saml_idp/service_provider.rb | 1 + spec/lib/saml_idp/controller_spec.rb | 2 +- spec/lib/saml_idp/request_spec.rb | 136 ++++++++++++++++++++++++++- spec/support/saml_request_macros.rb | 45 +++++---- spec/support/security_helpers.rb | 2 +- 6 files changed, 175 insertions(+), 26 deletions(-) diff --git a/lib/saml_idp/request.rb b/lib/saml_idp/request.rb index eb651145..9c69cc5f 100644 --- a/lib/saml_idp/request.rb +++ b/lib/saml_idp/request.rb @@ -128,9 +128,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 +136,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 @@ -232,5 +232,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/spec/lib/saml_idp/controller_spec.rb b/spec/lib/saml_idp/controller_spec.rb index 4d63f7a1..e5609f88 100644 --- a/spec/lib/saml_idp/controller_spec.rb +++ b/spec/lib/saml_idp/controller_spec.rb @@ -128,7 +128,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'] diff --git a/spec/lib/saml_idp/request_spec.rb b/spec/lib/saml_idp/request_spec.rb index 794fc361..e88feed9 100644 --- a/spec/lib/saml_idp/request_spec.rb +++ b/spec/lib/saml_idp/request_spec.rb @@ -119,6 +119,140 @@ def info(msg); end end end end + + context "when signature provided in authn request" do + let(:auth_request) { OneLogin::RubySaml::Authrequest.new } + let(:sp_setting) { saml_settings("https://foo.example.com/saml/consume", true) } + let(:raw_authn_request) { CGI.unescape(auth_request.create(sp_setting).split("=").last) } + + subject { described_class.from_deflated_request raw_authn_request } + + before do + idp_configure("http://localhost:3000/saml/consume", true) + end + + context "when AuthnRequest signature validation is enabled" do + before do + SamlIdp.configure do |config| + config.service_provider.finder = lambda { |_issuer_or_entity_id| + { + response_hosts: [URI("http://localhost:3000/saml/consume").host], + acs_url: "http://localhost:3000/saml/consume", + cert: sp_x509_cert, + fingerprint: SamlIdp::Fingerprint.certificate_digest(sp_x509_cert), + assertion_consumer_logout_service_url: 'https://foo.example.com/saml/logout', + sign_authn_request: true + } + } + end + end + + it "returns true" do + expect(subject.send(:validate_auth_request_signature?)).to be true + end + + it "validates the signature" do + allow(subject).to receive(:signature).and_return(nil) + allow(subject).to receive(:valid_signature?).and_call_original + + expect(subject.valid_signature?).to be true + end + end + + context "when AuthnRequest signature validation is disabled" do + before do + SamlIdp.configure do |config| + config.service_provider.finder = lambda { |_issuer_or_entity_id| + { + response_hosts: [URI("http://localhost:3000/saml/consume").host], + acs_url: "http://localhost:3000/saml/consume", + cert: sp_x509_cert, + fingerprint: SamlIdp::Fingerprint.certificate_digest(sp_x509_cert), + assertion_consumer_logout_service_url: 'https://foo.example.com/saml/logout', + sign_authn_request: false + } + } + end + end + + it "returns false" do + expect(subject.send(:validate_auth_request_signature?)).to be false + end + + it "does not validate the signature and return true" do + allow(subject).to receive(:signature).and_return(nil) + allow(subject).to receive(:valid_signature?).and_call_original + + expect(subject.valid_signature?).to be true + end + end + end + + context "when signature provided as external params" do + let(:auth_request) { OneLogin::RubySaml::Authrequest.new } + let(:sp_setting) { saml_settings("https://foo.example.com/saml/consume", true, security_options: { embed_sign: false }) } + let(:saml_response) { auth_request.create(sp_setting) } + let(:query_params) { CGI.parse(URI.parse(saml_response).query) } + let(:raw_authn_request) { query_params['SAMLRequest'].first } + let(:signature) { query_params['Signature'].first } + let(:sig_algorithm) { query_params['SigAlg'].first } + + before do + idp_configure("http://localhost:3000/saml/consume", true) + end + + subject do + described_class.from_deflated_request( + raw_authn_request, + saml_request: raw_authn_request, + relay_state: query_params['RelayState'].first, + sig_algorithm: sig_algorithm, + signature: signature + ) + end + + context "when AuthnRequest signature validation is enabled" do + before do + SamlIdp.configure do |config| + config.service_provider.finder = lambda { |_issuer_or_entity_id| + { + response_hosts: [URI("http://localhost:3000/saml/consume").host], + acs_url: "http://localhost:3000/saml/consume", + cert: sp_x509_cert, + fingerprint: SamlIdp::Fingerprint.certificate_digest(sp_x509_cert), + assertion_consumer_logout_service_url: 'https://foo.example.com/saml/logout', + sign_authn_request: true + } + } + end + end + + it "validate certificates and return valid" do + expect(subject.valid_external_signature?).to be true + end + end + + context "when AuthnRequest signature validation is disabled" do + before do + SamlIdp.configure do |config| + config.service_provider.finder = lambda { |_issuer_or_entity_id| + { + response_hosts: [URI("http://localhost:3000/saml/consume").host], + acs_url: "http://localhost:3000/saml/consume", + cert: sp_x509_cert, + fingerprint: SamlIdp::Fingerprint.certificate_digest(sp_x509_cert), + assertion_consumer_logout_service_url: 'https://foo.example.com/saml/logout', + sign_authn_request: false + } + } + end + end + + it "skip signature validation and return valid" do + expect(subject.valid_external_signature?).to be true + end + end + end end describe "logout request" do @@ -157,7 +291,7 @@ def info(msg); end end context 'when signature provided as external param' do - let!(:uri_query) { make_saml_sp_slo_request } + 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'] } diff --git a/spec/support/saml_request_macros.rb b/spec/support/saml_request_macros.rb index cd399346..230e408c 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 @@ -109,4 +102,16 @@ def print_pretty_xml(xml_string) doc.write(outbuf, 1) puts outbuf 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..d90bc394 100644 --- a/spec/support/security_helpers.rb +++ b/spec/support/security_helpers.rb @@ -51,7 +51,7 @@ 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 + def certificate_1 @signature1 ||= File.read(File.join(File.dirname(__FILE__), 'certificates', 'certificate1')) end From 86a2105a310b9cf1162f6ff074473b229522144c Mon Sep 17 00:00:00 2001 From: zogoo Date: Wed, 14 Aug 2024 08:10:04 +0800 Subject: [PATCH 3/5] =?UTF-8?q?[feat]=20Don=E2=80=99t=20ignore=20certifica?= =?UTF-8?q?tes=20without=20usage=20(#17)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I have tested with live SAML SP apps and it works fine * Unspecified certifciate from SP metadata --------- Co-authored-by: zogoo --- lib/saml_idp/incoming_metadata.rb | 9 +++ spec/lib/saml_idp/incoming_metadata_spec.rb | 75 +++++++++++++++++++++ 2 files changed, 84 insertions(+) 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/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 From 3879f595ed9e61d24ce1dc7bfd519a1beb0655ee Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 17 Sep 2024 22:23:26 +0200 Subject: [PATCH 4/5] Try with proper way to update helper method --- saml_idp.gemspec | 1 + spec/support/saml_request_macros.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/saml_idp.gemspec b/saml_idp.gemspec index 4c46a70d..d8346128 100644 --- a/saml_idp.gemspec +++ b/saml_idp.gemspec @@ -54,6 +54,7 @@ Gem::Specification.new do |s| s.add_development_dependency('byebug') s.add_development_dependency('capybara', '>= 2.16') s.add_development_dependency('rails', '>= 5.2') + 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') diff --git a/spec/support/saml_request_macros.rb b/spec/support/saml_request_macros.rb index 230e408c..ff6704a9 100644 --- a/spec/support/saml_request_macros.rb +++ b/spec/support/saml_request_macros.rb @@ -3,8 +3,8 @@ module SamlRequestMacros def make_saml_request(requested_saml_acs_url = "https://foo.example.com/saml/consume", enable_secure_options = false) auth_request = OneLogin::RubySaml::Authrequest.new - auth_url = auth_request.create(saml_settings(requested_saml_acs_url, enable_secure_options)) - CGI.unescape(auth_url.split("=").last) + auth_url = auth_request.create_params(saml_settings(requested_saml_acs_url, enable_secure_options)) + auth_url['SAMLRequest'] end def make_saml_logout_request(requested_saml_logout_url = 'https://foo.example.com/saml/logout') From abf2f5721254cdc50dcf67d779c8840800008cb2 Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 17 Sep 2024 22:43:09 +0200 Subject: [PATCH 5/5] Correctly decode and mock with correct REXML class --- spec/lib/saml_idp/controller_spec.rb | 2 +- spec/support/saml_request_macros.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/spec/lib/saml_idp/controller_spec.rb b/spec/lib/saml_idp/controller_spec.rb index e5609f88..287a0c00 100644 --- a/spec/lib/saml_idp/controller_spec.rb +++ b/spec/lib/saml_idp/controller_spec.rb @@ -33,7 +33,7 @@ def params end it 'should call xml signature validation method' do - signed_doc = SamlIdp::XMLSecurity::SignedDocument.new(params[:SAMLRequest]) + signed_doc = SamlIdp::XMLSecurity::SignedDocument.new(decode_saml_request(params[:SAMLRequest])) allow(signed_doc).to receive(:validate).and_return(true) allow(SamlIdp::XMLSecurity::SignedDocument).to receive(:new).and_return(signed_doc) validate_saml_request diff --git a/spec/support/saml_request_macros.rb b/spec/support/saml_request_macros.rb index ff6704a9..5efbdf29 100644 --- a/spec/support/saml_request_macros.rb +++ b/spec/support/saml_request_macros.rb @@ -103,6 +103,17 @@ def print_pretty_xml(xml_string) 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,