From 3e58ea24348c2ba96bca3f138e728310ff8c6e82 Mon Sep 17 00:00:00 2001 From: zogoo Date: Thu, 25 Jul 2024 00:50:53 +0800 Subject: [PATCH 01/22] 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 02/22] [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 03/22] =?UTF-8?q?[feat]=20Don=E2=80=99t=20ignore=20certifi?= =?UTF-8?q?cates=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 94196fe9f25565e3c0cba1a41d043b0470135c1f Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 17 Sep 2024 23:14:16 +0200 Subject: [PATCH 04/22] 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 --- saml_idp.gemspec | 1 + spec/lib/saml_idp/controller_spec.rb | 2 +- spec/support/saml_request_macros.rb | 15 +++++++++++++-- 3 files changed, 15 insertions(+), 3 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/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 230e408c..5efbdf29 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') @@ -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, From 1071d67e29bc7256fcd487e1927760acf4a55324 Mon Sep 17 00:00:00 2001 From: zogoo Date: Thu, 26 Sep 2024 11:46:07 +0200 Subject: [PATCH 05/22] [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 --- lib/saml_idp/controller.rb | 5 +- lib/saml_idp/request.rb | 22 ++ spec/lib/saml_idp/controller_spec.rb | 23 +- spec/lib/saml_idp/request_spec.rb | 431 ++++++++++----------------- 4 files changed, 187 insertions(+), 294 deletions(-) diff --git a/lib/saml_idp/controller.rb b/lib/saml_idp/controller.rb index d5bd20c9..9d0a16c9 100644 --- a/lib/saml_idp/controller.rb +++ b/lib/saml_idp/controller.rb @@ -34,10 +34,7 @@ def acs_url def validate_saml_request(raw_saml_request = params[:SAMLRequest]) decode_request(raw_saml_request, params[:Signature], params[:SigAlg], params[:RelayState]) - return true if valid_saml_request? - - head :forbidden if defined?(::Rails) - false + valid_saml_request? end def decode_request(raw_saml_request, signature, sig_algorithm, relay_state) diff --git a/lib/saml_idp/request.rb b/lib/saml_idp/request.rb index 9c69cc5f..44b4def0 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 @@ -152,6 +171,9 @@ def valid_external_signature? end cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string) + rescue OpenSSL::X509::CertificateError => e + log e.message + collect_errors(:cert_format_error) end def service_provider? diff --git a/spec/lib/saml_idp/controller_spec.rb b/spec/lib/saml_idp/controller_spec.rb index 287a0c00..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) @@ -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/request_spec.rb b/spec/lib/saml_idp/request_spec.rb index e88feed9..eca624f7 100644 --- a/spec/lib/saml_idp/request_spec.rb +++ b/spec/lib/saml_idp/request_spec.rb @@ -1,323 +1,194 @@ 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) - end + 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'] } - it "is a authn request" do - expect(subject.authn_request?).to eq(true) + 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 "fetches internal request" do - expect(subject.request['ID']).to eq(subject.request_id) + it "should validate the request" do + expect(subject.valid_external_signature?).to be true + expect(subject.errors).to be_empty 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') + 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 - 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) } + 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) - 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 - 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 + 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) + + expect(request.valid?).to be false + expect(request.errors).to include(:empty_certificate) 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 + 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 "validate certificates and return valid" do - expect(subject.valid_external_signature?).to be true - end - 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 - 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 and logs an error" do + request = SamlIdp::Request.from_deflated_request(valid_saml_request) - it "skip signature validation and return valid" do - expect(subject.valid_external_signature?).to be true - end - end + expect(request.valid?).to be false + expect(request.errors).to include(:empty_certificate) 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 } - - 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 + 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 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 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) - end - - 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 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 + 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) + request = SamlIdp::Request.from_deflated_request(valid_saml_request) - 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 From ade27c30227fa8ddb55f3b631f096e08cbfc70cd Mon Sep 17 00:00:00 2001 From: zogoo Date: Thu, 26 Sep 2024 11:48:18 +0200 Subject: [PATCH 06/22] Support lowercase percent-encoded sequences for URL encoding (#20) Co-authored-by: zogoo --- lib/saml_idp/request.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/saml_idp/request.rb b/lib/saml_idp/request.rb index 44b4def0..bd84c5d8 100644 --- a/lib/saml_idp/request.rb +++ b/lib/saml_idp/request.rb @@ -170,7 +170,11 @@ 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) From d95a9d69297e9cd814d6613ac23c9775d588605f Mon Sep 17 00:00:00 2001 From: zogoo Date: Fri, 25 Oct 2024 14:09:05 +0200 Subject: [PATCH 07/22] [fix] Gem CI updates for latest versions (#22) * Remove duplications * Pre-conditions need to be defined in before section * Le's not test logger in here --------- Co-authored-by: zogoo --- saml_idp.gemspec | 1 - spec/lib/saml_idp/configurator_spec.rb | 17 ++++++++++++----- spec/lib/saml_idp/request_spec.rb | 1 + 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/saml_idp.gemspec b/saml_idp.gemspec index 3db940c1..1a332250 100644 --- a/saml_idp.gemspec +++ b/saml_idp.gemspec @@ -56,7 +56,6 @@ Gem::Specification.new do |s| s.add_development_dependency('rails', '>= 5.2') s.add_development_dependency('debug') s.add_development_dependency('rake') - s.add_development_dependency('debug') 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/request_spec.rb b/spec/lib/saml_idp/request_spec.rb index eca624f7..151e391a 100644 --- a/spec/lib/saml_idp/request_spec.rb +++ b/spec/lib/saml_idp/request_spec.rb @@ -185,6 +185,7 @@ 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) expect(request.valid?).to be false From fcb331b1dff5faf7d56f5977c05726b9ed8052eb Mon Sep 17 00:00:00 2001 From: zogoo Date: Fri, 25 Oct 2024 14:16:19 +0200 Subject: [PATCH 08/22] [fix] Allow IdP set reference ID for SAML response (#21) * Pass ref id as Session Index * Official Rails 8 is not released yet to RubyGem until that let's stick official older version --------- Co-authored-by: zogoo --- gemfiles/rails_dev.gemfile | 2 +- lib/saml_idp/saml_response.rb | 4 ++-- spec/lib/saml_idp/saml_response_spec.rb | 19 +++++++++++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/gemfiles/rails_dev.gemfile b/gemfiles/rails_dev.gemfile index 645a4e5d..dd5a03a3 100644 --- a/gemfiles/rails_dev.gemfile +++ b/gemfiles/rails_dev.gemfile @@ -2,7 +2,7 @@ source "https://rubygems.org" -gem "rails", github: "rails/rails", branch: "main" +gem "rails", "~> 7.2.1" gem "activeresource", github: "rails/activeresource", branch: "main" gemspec path: "../" diff --git a/lib/saml_idp/saml_response.rb b/lib/saml_idp/saml_response.rb index fdc327a5..85cf48b1 100644 --- a/lib/saml_idp/saml_response.rb +++ b/lib/saml_idp/saml_response.rb @@ -98,7 +98,7 @@ def response_builder def assertion_builder @assertion_builder ||= - AssertionBuilder.new SecureRandom.uuid, + AssertionBuilder.new(reference_id || SecureRandom.uuid, issuer_uri, principal, audience_uri, @@ -110,7 +110,7 @@ def assertion_builder encryption_opts, session_expiry, name_id_formats_opts, - asserted_attributes_opts + asserted_attributes_opts) end private :assertion_builder end diff --git a/spec/lib/saml_idp/saml_response_spec.rb b/spec/lib/saml_idp/saml_response_spec.rb index a9e82151..b79f8a2b 100644 --- a/spec/lib/saml_idp/saml_response_spec.rb +++ b/spec/lib/saml_idp/saml_response_spec.rb @@ -192,6 +192,25 @@ module SamlIdp expect(saml_resp.is_valid?).to eq(true) end + it "will pass reference_id as SessionIndex" do + expect { subject.build }.not_to raise_error + signed_encoded_xml = subject.build + resp_settings = saml_settings(saml_acs_url) + resp_settings.private_key = Default::SECRET_KEY + resp_settings.issuer = audience_uri + saml_resp = OneLogin::RubySaml::Response.new(signed_encoded_xml, settings: resp_settings) + + expect( + Nokogiri::XML(saml_resp.response).at_xpath( + "//saml:AuthnStatement/@SessionIndex", + { + "samlp" => "urn:oasis:names:tc:SAML:2.0:protocol", + "saml" => "urn:oasis:names:tc:SAML:2.0:assertion" + } + ).value + ).to eq("_#{reference_id}") + end + it "sets session expiration" do saml_resp = OneLogin::RubySaml::Response.new(subject.build) expect(saml_resp.session_expires_at).to eq Time.local(1990, "jan", 2).iso8601 From 02f3a7a406bfe8efbe9dce33751c3c78e4d5b8a3 Mon Sep 17 00:00:00 2001 From: zogoo Date: Fri, 1 Nov 2024 12:36:21 +0100 Subject: [PATCH 09/22] Support rails 8 for dev env (#23) Co-authored-by: zogoo --- gemfiles/rails_dev.gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gemfiles/rails_dev.gemfile b/gemfiles/rails_dev.gemfile index dd5a03a3..645a4e5d 100644 --- a/gemfiles/rails_dev.gemfile +++ b/gemfiles/rails_dev.gemfile @@ -2,7 +2,7 @@ source "https://rubygems.org" -gem "rails", "~> 7.2.1" +gem "rails", github: "rails/rails", branch: "main" gem "activeresource", github: "rails/activeresource", branch: "main" gemspec path: "../" From e015f6e4ac8f38f517fe1516b8518e9b8889f2e5 Mon Sep 17 00:00:00 2001 From: zogoo Date: Fri, 1 Nov 2024 23:28:41 +0100 Subject: [PATCH 10/22] Signable logic with given certificate information --- lib/saml_idp/signable.rb | 35 +++++++++++-------- lib/saml_idp/signature_builder.rb | 28 ++++++++------- lib/saml_idp/signed_info_builder.rb | 27 ++++++-------- spec/lib/saml_idp/signable_spec.rb | 15 +++++++- spec/spec_helper.rb | 1 + .../certificates/sp_encrypted_pv_key.pem | 30 ++++++++++++++++ spec/support/certificates/sp_public_cert.pem | 16 +++++++++ spec/support/security_helpers.rb | 8 +++++ 8 files changed, 115 insertions(+), 45 deletions(-) create mode 100644 spec/support/certificates/sp_encrypted_pv_key.pem create mode 100644 spec/support/certificates/sp_public_cert.pem diff --git a/lib/saml_idp/signable.rb b/lib/saml_idp/signable.rb index 0a330a18..14c923a8 100644 --- a/lib/saml_idp/signable.rb +++ b/lib/saml_idp/signable.rb @@ -5,6 +5,7 @@ # * algorithm require 'saml_idp/signed_info_builder' require 'saml_idp/signature_builder' + module SamlIdp module Signable def self.included(base) @@ -24,6 +25,8 @@ def sign(el) el << signature if sign? end + private + def generated_reference_id if reference_id fin = yield reference_id if block_given? @@ -34,12 +37,10 @@ def generated_reference_id end block_given? ? fin : ref end - private :generated_reference_id def reference_id_generator SamlIdp.config.reference_id_generator end - private :reference_id_generator def with_signature original = @sign @@ -48,7 +49,6 @@ def with_signature @sign = original end end - private :with_signature def without_signature original = @sign @@ -57,49 +57,52 @@ def without_signature @sign = original end end - private :without_signature def sign? !!@sign end - private :sign? def signature - SignatureBuilder.new(signed_info_builder).raw + SignatureBuilder.new(signed_info_builder, get_public_cert).raw end - private :signature def signed_info_builder - SignedInfoBuilder.new(get_reference_id, get_digest, get_algorithm) + SignedInfoBuilder.new(get_reference_id, get_digest, get_algorithm, get_private_key, pv_key_password) end - private :signed_info_builder def get_reference_id send(self.class.reference_id_method) end - private :get_reference_id def get_digest without_signature do send(self.class.digest_method) end end - private :get_digest def get_algorithm send(self.class.algorithm_method) end - private :get_algorithm def get_raw send(self.class.raw_method) end - private :get_raw + + def get_public_cert + send(self.class.public_cert_method) + end + + def get_private_key + send(self.class.private_key_method) + end + + def pv_key_password + send(self.class.pv_key_password_method) + end def noko_raw Nokogiri::XML::Document.parse(get_raw) end - private :noko_raw def digest # Make it check for inclusive at some point (https://github.com/onelogin/ruby-saml/blob/master/lib/xml_security.rb#L159) @@ -111,7 +114,6 @@ def digest hash = digest_algorithm.digest(canon_hashed_element) Base64.strict_encode64(hash).gsub(/\n/, '') end - private :digest module ClassMethods def self.module_method(name, default = nil) @@ -125,6 +127,9 @@ def self.module_method(name, default = nil) module_method :digest module_method :algorithm module_method :reference_id + module_method :public_cert + module_method :private_key + module_method :pv_key_password end end end diff --git a/lib/saml_idp/signature_builder.rb b/lib/saml_idp/signature_builder.rb index 83183f23..da45644c 100644 --- a/lib/saml_idp/signature_builder.rb +++ b/lib/saml_idp/signature_builder.rb @@ -1,10 +1,12 @@ require 'builder' + module SamlIdp class SignatureBuilder - attr_accessor :signed_info_builder + attr_accessor :signed_info_builder, :x509_certificate - def initialize(signed_info_builder) + def initialize(signed_info_builder, x509_certificate) self.signed_info_builder = signed_info_builder + self.x509_certificate = x509_certificate end def raw @@ -14,29 +16,31 @@ def raw signature.tag! "ds:SignatureValue", signature_value signature.KeyInfo xmlns: "http://www.w3.org/2000/09/xmldsig#" do |key_info| key_info.tag! "ds:X509Data" do |x509| - x509.tag! "ds:X509Certificate", x509_certificate + x509.tag! "ds:X509Certificate", der_certificate end end end end - def x509_certificate - SamlIdp.config.x509_certificate - .to_s - .gsub(/-----BEGIN CERTIFICATE-----/,"") - .gsub(/-----END CERTIFICATE-----/,"") - .gsub(/\n/, "") + private + + def der_certificate + certificate_formatter(x509_certificate || SamlIdp.config.x509_certificate) end - private :x509_certificate def signed_info signed_info_builder.raw end - private :signed_info def signature_value signed_info_builder.signed end - private :signature_value + + def certificate_formatter(pem_certificate) + pem_certificate + .gsub(/-----BEGIN CERTIFICATE-----/,"") + .gsub(/-----END CERTIFICATE-----/,"") + .gsub(/\n/, "") + end end end diff --git a/lib/saml_idp/signed_info_builder.rb b/lib/saml_idp/signed_info_builder.rb index 81380666..b6b5391a 100644 --- a/lib/saml_idp/signed_info_builder.rb +++ b/lib/saml_idp/signed_info_builder.rb @@ -22,11 +22,15 @@ class SignedInfoBuilder attr_accessor :reference_id attr_accessor :digest_value attr_accessor :raw_algorithm + attr_accessor :private_key + attr_accessor :pv_key_password - def initialize(reference_id, digest_value, raw_algorithm) + def initialize(reference_id, digest_value, raw_algorithm, private_key, pv_key_password) self.reference_id = reference_id self.digest_value = digest_value self.raw_algorithm = raw_algorithm + self.private_key = private_key + self.pv_key_password = pv_key_password end def raw @@ -49,40 +53,29 @@ def signed encoded.gsub(/\n/, "") end + private + def digest_method DIGEST_METHODS.fetch(clean_algorithm_name, DIGEST_METHODS["sha1"]) end - private :digest_method def signature_method SIGNATURE_METHODS.fetch(clean_algorithm_name, SIGNATURE_METHODS["sha1"]) end - private :signature_method def clean_algorithm_name algorithm_name.to_s.downcase end - private :clean_algorithm_name - - def secret_key - SamlIdp.config.secret_key - end - private :secret_key - - def password - SamlIdp.config.password - end - private :password def encoded - key = OpenSSL::PKey::RSA.new(secret_key, password) + rsa_key = private_key || SamlIdp.config.secret_key + key_secret = pv_key_password || SamlIdp.config.pv_key_password + key = OpenSSL::PKey::RSA.new(rsa_key, key_secret) Base64.strict_encode64(key.sign(algorithm.new, raw)) end - private :encoded def reference_string "#_#{reference_id}" end - private :reference_string end end diff --git a/spec/lib/saml_idp/signable_spec.rb b/spec/lib/saml_idp/signable_spec.rb index cd9a90ae..0d033780 100644 --- a/spec/lib/saml_idp/signable_spec.rb +++ b/spec/lib/saml_idp/signable_spec.rb @@ -1,7 +1,9 @@ # encoding: utf-8 require 'spec_helper' + class MockSignable include SamlIdp::Signable + include SecurityHelpers def raw builder = Builder::XmlMarkup.new @@ -21,6 +23,18 @@ def digest def algorithm OpenSSL::Digest::SHA1 end + + def private_key + sp_encrypted_pv_key[:sp_encrypted_pv_key] + end + + def pv_key_password + sp_encrypted_pv_key[:pv_key_password] + end + + def public_cert + sp_encrypted_pv_key[:sp_public_cert] + end end module SamlIdp @@ -72,6 +86,5 @@ module SamlIdp it "has a valid signed" do expect(subject.signed).to match all_regex end - end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index ffc9f9a8..61819b77 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -17,6 +17,7 @@ require 'ruby-saml' require 'saml_idp' require 'timecop' +require 'debug' Dir[File.dirname(__FILE__) + "/support/**/*.rb"].each {|f| require f} diff --git a/spec/support/certificates/sp_encrypted_pv_key.pem b/spec/support/certificates/sp_encrypted_pv_key.pem new file mode 100644 index 00000000..2fecdc96 --- /dev/null +++ b/spec/support/certificates/sp_encrypted_pv_key.pem @@ -0,0 +1,30 @@ +-----BEGIN RSA PRIVATE KEY----- +Proc-Type: 4,ENCRYPTED +DEK-Info: AES-128-CBC,89BFDC1035C5C7A2277D87B8F0763A70 + +9Tpy80djhnb8S7Xq7O94TkH6ec38gO54JWbMbC3xTtVnR8lqvX6kIeF5gLjbySUI +Q30F+1bZ1/AiT1PeJ9Mg+TUEFDxiNrRw+liS3F6mhM5XyGe96FVOGEKp+WbCEznV +ZgLzzMXXwcAzKblsVNAiBAozqyfdhyGg/BhTcPXpAXt3wxy4VfbwHs1CW5+Ar+iN +qlYXDFyg+UPh0EbKQc7FUB6NKcgZ76yuaWQlkUfAbBjx4f7zpHSwYO29Gv2Hq/E4 +REhS4GXuJDTZLSDW3n6TcOszyudVYNSNFEjDWtc88/l3gQW5ZtJVnLarNMOfQF48 +MFFQBT/ksT/9GbCIKOonr12D+XlGgHJpNJrHQ+Hzee3sU8U4tn2/4sM2ZMrjOiMh +giuJq1OiM95ohK3XcdcE5apbMtCTzBjcwumG6MaI/okQURHuHvMvdhivhp5CC1Ix +dRx9cnCmXidEdhsgKBq5vBvcqt4VJqqkaGEA0YBAge//LZbzc1fp8Ghzjdev78Sb +SeW/QnzGTPW4FX/UMLrZPuwNpx9fJ1pJdI8UOOWBxIBkvl+1dM9WOVP+07b80RO1 +rJm+QKZVEbrHZKX8Gq3+DyvI2a+FUWv0WTDxp4fcPMAGy+Kr8Xratxh/fQhvksbV +ZK6/m/8f97zfxsUU5tM0B+WwIv6/KQm3wwloeL226OgmBX8x+WKthFe+MozchVLI +UNvXCw5NeGmIbLWlcoZuzsvHHcfNJ/iDaBLl6itJxd1sNiuamTHfpTQlxoDCwaQO +OqRRMfq8pjEgHYXHMm0us3sQ235yN5W32K2N79YlTQOQUA9QYHLlpmMvVRtOSRQY +GNhIPNWqLWP7+CAtfXVA0o97p5Wd0ySWwOcv3iJuAxywBg0a6s6iYV2lkRHHkJY2 +dvDH39hbaqRakPUKFOPkCHdHvN+cM7trvdxpzURj67u9IKLPMwK64FJ3LEerqSgh +FIcwTcN8jQBLdcsi9XHNPRJEaa+4Sf7KpeUYe79yZlFNDKU6yG8DMpt2uJ6p17C7 +R2L11AdsPC0MAgxaj2yNV1/CSP3wc6Y36AMSnRJtMsUX7BjZT6TSUjOj21zPfu9R +h2aJYe1224EpT6bb31Sgfky2TcIOXAY60g8slRLniCDzmexB6u/EUvF7A4fYXFRK +pgcbWVAfYr1/GqXs9srLLAeYRbFcg6aaTD63uTe0rf/Vse5h7c9tmMZ6/Wk/rkwQ +oguzl8rXvXd2mpJdhvT/NDI2AI6jzIDcCb/F21qlyfEk8K5Rlykm8BDoRNx6+wW/ +SuUZfnXJmywovvpRSsEaGTYo/3eERZVdIZqV5nLHVJtcTRzvh3QuCm2JC8S1HNHE +PboWwv1BHZOR9/llB/voip+DJuIGkBVwbN9mDUC+uf7L/y6Xj8kOvI7g3zaJR/ac +t2YjhgL3sxORZp8lIsYlwZYHT39jGkYP4Pi1HoVWH1sKyn+P55nF8f9BrQCmhyFU +t1K+p2AwzuWxYw6YThQYy2SdiGSaZ9axGrwlu1kdh9Vq2YDctXX+yR1Bs3DytK/6 +PzdLSUAJt4HoLCND+lXG8Ii79WWTCqX9UrYWTJN4PhApCnN+/RYft2Bjd8ZGl0E2 +-----END RSA PRIVATE KEY----- diff --git a/spec/support/certificates/sp_public_cert.pem b/spec/support/certificates/sp_public_cert.pem new file mode 100644 index 00000000..02bea35a --- /dev/null +++ b/spec/support/certificates/sp_public_cert.pem @@ -0,0 +1,16 @@ +-----BEGIN CERTIFICATE----- +MIICjTCCAXWgAwIBAgIVAOTwdQ89937l4RyuaJ7Ni1K2YifEMA0GCSqGSIb3DQEB +CwUAMAAwHhcNMjQxMTAxMjIyMzQ4WhcNMzQxMTAxMjIyMzQ4WjAAMIIBIjANBgkq +hkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1Ddn+u21iDGHHcLLsThAgdAXLnvL8Pqi +aXKu+MoMu2LYu/pHPk6ZuCmecdM6Tsqdc69S1fCJGu45zSbSuQNzZnW+iLItzBYj +WWv5cgQdOTkT6WVUBsA7NU9sKKmCqsU/3DOCXbZ8OYPxnftZGRIZdydsENOG49f6 +195VJ7h3FybXUnqpaw+bO+pepl4UNl6Y6nIKv4eZnAXJ8P7sJX2iHiu5zl2y/eht +rrL9faNdiSdb2wERkcIZKDrnoGlTvxkFd7fr7sIHIIJvxCBvBpBrgSNfQrHtqjsG +STQ4IK1f5Vtwy8jVkuK+SBEzA+Ft6IlWDkTW3oTnFgVjwwOv0NQdNwIDAQABMA0G +CSqGSIb3DQEBCwUAA4IBAQC2nUkoFXCBVvmkJ9iwXnmJwTGPuDFlZh0zVAuDnVCi +DAcb5z+96GV1dPucFGP/0dtSVazHw3bpw1b9sm1844ne11XjU6PFGEkVAbAyA72Z +Vi0t+ksEVlXP044XjSvZLuWnETJU31w8MjUcoi4BUhdlPtgqYd5jWMSKTncpZCmp +zggCKpz6tSpJcBl7kBAudcxP3kCrWNku5V1OW/4Z4A27ISJLE+Gzi2Q+O/lKHydr +ebXtjQpuuCuMnNwWaLRnxQaQWmYdbVvCuXIweoC6pSPffwjbMpU4ZTpHn1UoPqUQ +9JfW50G2IUKQjq9ssjZAKcGsQIRAIwzwWLvNtl62OeoK +-----END CERTIFICATE----- diff --git a/spec/support/security_helpers.rb b/spec/support/security_helpers.rb index f6952ce7..93b57391 100644 --- a/spec/support/security_helpers.rb +++ b/spec/support/security_helpers.rb @@ -68,4 +68,12 @@ def sp_pv_key def sp_x509_cert @sp_x509_cert ||= File.read(File.join(File.dirname(__FILE__), 'certificates', 'sp_x509_cert.crt')) end + + def sp_encrypted_pv_key + @sp_encrypted_pv_key ||= { + sp_encrypted_pv_key: File.read(File.join(File.dirname(__FILE__), 'certificates', 'sp_encrypted_pv_key.pem')), + sp_public_cert: File.read(File.join(File.dirname(__FILE__), 'certificates', 'sp_public_cert.pem')), + pv_key_password: 'dccf0b7614b554fbe16b0f31' + } + end end From 38b70fd656f4be48de8c80c503b84ac05fecf801 Mon Sep 17 00:00:00 2001 From: zogoo Date: Mon, 4 Nov 2024 13:01:57 +0100 Subject: [PATCH 11/22] Update unit test with new test certificate --- spec/lib/saml_idp/signed_info_builder_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/lib/saml_idp/signed_info_builder_spec.rb b/spec/lib/saml_idp/signed_info_builder_spec.rb index 4685d39a..0ac39966 100644 --- a/spec/lib/saml_idp/signed_info_builder_spec.rb +++ b/spec/lib/saml_idp/signed_info_builder_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' + module SamlIdp describe SignedInfoBuilder do let(:reference_id) { "abc" } @@ -7,7 +8,9 @@ module SamlIdp subject { described_class.new( reference_id, digest, - algorithm + algorithm, + sp_encrypted_pv_key[:sp_encrypted_pv_key], + sp_encrypted_pv_key[:pv_key_password] ) } before do @@ -19,7 +22,7 @@ module SamlIdp end it "builds a legit digest of the XML file" do - expect(subject.signed).to eq("hKLeWLRgatHcV6N5Fc8aKveqNp6Y/J4m2WSYp0awGFtsCTa/2nab32wI3du+3kuuIy59EDKeUhHVxEfyhoHUo6xTZuO2N7XcTpSonuZ/CB3WjozC2Q/9elss3z1rOC3154v5pW4puirLPRoG+Pwi8SmptxNRHczr6NvmfYmmGfo=") + expect(subject.signed).to eq("YP2e6cTEfRj1vI1/gaaSApLAMxPBQzyuBvbvulbS99x17LCLDSKvqA6MyU4WLavmVba5qiF88e97f0XKLsse7gEGOfnF/6jaRV3fePXk6+LFaeYUHZ11u7PkZ1/ucO459ASsuPN/9P9xCY2t+jtVKvIrcSZQbomymfsWGt9P/oY83elKU712aAwqcfvINsa1N+RefZRwdAW4ATBwwcDjE3VTR6mKOyGMsPJ4HQcPrNiEmuwd1QaPH0H1LLzxtewGQGmIL2UqNE/QMe/kKiSTFZ0loBKuSEc9WBw5XuH31QxbzpLJjqM/C1qy4aykPqDUuJtQ4csx78GgfFS4uiqowg==") end end end From 6f3b8fa963dfda752dbebaff53a5c203ec4d89fa Mon Sep 17 00:00:00 2001 From: zogoo Date: Mon, 4 Nov 2024 13:50:45 +0100 Subject: [PATCH 12/22] Assertion builder with certificate attribute --- lib/saml_idp/assertion_builder.rb | 66 +++--- spec/lib/saml_idp/assertion_builder_spec.rb | 210 +++++++++++--------- 2 files changed, 149 insertions(+), 127 deletions(-) diff --git a/lib/saml_idp/assertion_builder.rb b/lib/saml_idp/assertion_builder.rb index 8e0170fa..482ab16a 100644 --- a/lib/saml_idp/assertion_builder.rb +++ b/lib/saml_idp/assertion_builder.rb @@ -18,23 +18,29 @@ class AssertionBuilder attr_accessor :session_expiry attr_accessor :name_id_formats_opts attr_accessor :asserted_attributes_opts + attr_accessor :public_cert + attr_accessor :private_key + attr_accessor :pv_key_password delegate :config, to: :SamlIdp def initialize( - reference_id, - issuer_uri, - principal, - audience_uri, - saml_request_id, - saml_acs_url, - raw_algorithm, - authn_context_classref, - expiry=60*60, - encryption_opts=nil, - session_expiry=nil, - name_id_formats_opts = nil, - asserted_attributes_opts = nil + reference_id:, + issuer_uri:, + principal:, + audience_uri:, + saml_request_id:, + saml_acs_url:, + raw_algorithm:, + authn_context_classref:, + public_cert:, + private_key:, + pv_key_password:, + expiry: 60*60, + encryption_opts: nil, + session_expiry: nil, + name_id_formats_opts: nil, + asserted_attributes_opts: nil ) self.reference_id = reference_id self.issuer_uri = issuer_uri @@ -49,6 +55,17 @@ def initialize( self.session_expiry = session_expiry.nil? ? config.session_expiry : session_expiry self.name_id_formats_opts = name_id_formats_opts self.asserted_attributes_opts = asserted_attributes_opts + self.public_cert = public_cert + self.private_key = private_key + self.pv_key_password = pv_key_password + end + + def encrypt(opts = {}) + raise "Must set encryption_opts to encrypt" unless encryption_opts + raw_xml = opts[:sign] ? signed : raw + require 'saml_idp/encryptor' + encryptor = Encryptor.new encryption_opts + encryptor.encrypt(raw_xml) end def fresh @@ -105,15 +122,8 @@ def fresh end end alias_method :raw, :fresh - private :fresh - def encrypt(opts = {}) - raise "Must set encryption_opts to encrypt" unless encryption_opts - raw_xml = opts[:sign] ? signed : raw - require 'saml_idp/encryptor' - encryptor = Encryptor.new encryption_opts - encryptor.encrypt(raw_xml) - end + private def asserted_attributes if asserted_attributes_opts.present? && !asserted_attributes_opts.empty? @@ -124,7 +134,6 @@ def asserted_attributes config.attributes end end - private :asserted_attributes def get_values_for(friendly_name, getter) result = nil @@ -141,12 +150,10 @@ def get_values_for(friendly_name, getter) end Array(result) end - private :get_values_for def name_id name_id_getter.call principal end - private :name_id def name_id_getter getter = name_id_format[:getter] @@ -156,56 +163,45 @@ def name_id_getter ->(principal) { principal.public_send getter.to_s } end end - private :name_id_getter def name_id_format @name_id_format ||= NameIdFormatter.new(name_id_formats).chosen end - private :name_id_format def name_id_formats @name_id_formats ||= (name_id_formats_opts || config.name_id.formats) end - private :name_id_formats def reference_string "_#{reference_id}" end - private :reference_string def now @now ||= Time.now.utc end - private :now def now_iso iso { now } end - private :now_iso def not_before iso { now - 5 } end - private :not_before def not_on_or_after_condition iso { now + expiry } end - private :not_on_or_after_condition def not_on_or_after_subject iso { now + 3 * 60 } end - private :not_on_or_after_subject def session_not_on_or_after iso { now + session_expiry } end - private :session_not_on_or_after def iso yield.iso8601 end - private :iso end end diff --git a/spec/lib/saml_idp/assertion_builder_spec.rb b/spec/lib/saml_idp/assertion_builder_spec.rb index 9e267464..8592cd3f 100644 --- a/spec/lib/saml_idp/assertion_builder_spec.rb +++ b/spec/lib/saml_idp/assertion_builder_spec.rb @@ -1,4 +1,6 @@ require 'spec_helper' +require 'debug' + module SamlIdp describe AssertionBuilder do let(:reference_id) { "abc" } @@ -22,16 +24,21 @@ module SamlIdp let(:session_expiry) { nil } let(:name_id_formats_opt) { nil } let(:asserted_attributes_opt) { nil } + let(:principal) { Struct.new(:email, :asserted_attributes).new('foo@example.com', { emailAddress: { getter: :email } })} + subject { described_class.new( - reference_id, - issuer_uri, - name_id, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry + reference_id: reference_id, + issuer_uri: issuer_uri, + principal: name_id, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + raw_algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: sp_encrypted_pv_key[:sp_public_cert], + private_key: sp_encrypted_pv_key[:sp_encrypted_pv_key], + pv_key_password: sp_encrypted_pv_key[:pv_key_password], + expiry: expiry ) } context "No Request ID" do @@ -70,18 +77,19 @@ module SamlIdp describe "with principal.asserted_attributes" do it "delegates attributes to principal" do - Principal = Struct.new(:email, :asserted_attributes) - principal = Principal.new('foo@example.com', { emailAddress: { getter: :email } }) builder = described_class.new( - reference_id, - issuer_uri, - principal, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry + reference_id: reference_id, + issuer_uri: issuer_uri, + principal: principal, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + raw_algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: sp_encrypted_pv_key[:sp_public_cert], + private_key: sp_encrypted_pv_key[:sp_encrypted_pv_key], + pv_key_password: sp_encrypted_pv_key[:pv_key_password], + expiry: expiry ) Timecop.travel(Time.zone.local(2010, 6, 1, 13, 0, 0)) do expect(builder.raw).to eq("http://sportngin.comfoo@example.comhttp://example.comurn:oasis:names:tc:SAML:2.0:ac:classes:Passwordfoo@example.com") @@ -91,16 +99,19 @@ module SamlIdp it "builds encrypted XML" do builder = described_class.new( - reference_id, - issuer_uri, - name_id, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry, - encryption_opts + reference_id: reference_id, + issuer_uri: issuer_uri, + principal: name_id, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + raw_algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: sp_encrypted_pv_key[:sp_public_cert], + private_key: sp_encrypted_pv_key[:sp_encrypted_pv_key], + pv_key_password: sp_encrypted_pv_key[:pv_key_password], + expiry: expiry, + encryption_opts: encryption_opts ) encrypted_xml = builder.encrypt expect(encrypted_xml).to_not match(audience_uri) @@ -118,19 +129,22 @@ module SamlIdp UserWithUniqueId = Struct.new(:unique_identifier, :email, :asserted_attributes) principal = UserWithUniqueId.new('unique_identifier_123456', 'foo@example.com', { emailAddress: { getter: :email } }) builder = described_class.new( - reference_id, - issuer_uri, - principal, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry, - encryption_opts, - session_expiry, - name_id_formats_opt, - asserted_attributes_opt + reference_id: reference_id, + issuer_uri: issuer_uri, + principal: principal, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + raw_algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: sp_encrypted_pv_key[:sp_public_cert], + private_key: sp_encrypted_pv_key[:sp_encrypted_pv_key], + pv_key_password: sp_encrypted_pv_key[:pv_key_password], + expiry: expiry, + encryption_opts: encryption_opts, + session_expiry: session_expiry, + name_id_formats_opts: name_id_formats_opt, + asserted_attributes_opts: asserted_attributes_opt ) Timecop.travel(Time.zone.local(2010, 6, 1, 13, 0, 0)) do expect(builder.raw).to eq("http://sportngin.comunique_identifier_123456http://example.comurn:oasis:names:tc:SAML:2.0:ac:classes:Passwordfoo@example.com") @@ -156,19 +170,22 @@ module SamlIdp UserWithName = Struct.new(:email, :first_name, :last_name) principal = UserWithName.new('foo@example.com', 'George', 'Washington') builder = described_class.new( - reference_id, - issuer_uri, - principal, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry, - encryption_opts, - session_expiry, - name_id_formats_opt, - asserted_attributes_opt + reference_id: reference_id, + issuer_uri: issuer_uri, + principal: principal, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + raw_algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: sp_encrypted_pv_key[:sp_public_cert], + private_key: sp_encrypted_pv_key[:sp_encrypted_pv_key], + pv_key_password: sp_encrypted_pv_key[:pv_key_password], + expiry: expiry, + encryption_opts: encryption_opts, + session_expiry: session_expiry, + name_id_formats_opts: name_id_formats_opt, + asserted_attributes_opts: asserted_attributes_opt ) Timecop.travel(Time.zone.local(2010, 6, 1, 13, 0, 0)) do expect(builder.raw).to eq("http://sportngin.comfoo@example.comhttp://example.comurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordGeorgeWashington") @@ -185,16 +202,19 @@ module SamlIdp it "sets default session_expiry from config" do builder = described_class.new( - reference_id, - issuer_uri, - name_id, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry, - encryption_opts + reference_id: reference_id, + issuer_uri: issuer_uri, + principal: name_id, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + raw_algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: sp_encrypted_pv_key[:sp_public_cert], + private_key: sp_encrypted_pv_key[:sp_encrypted_pv_key], + pv_key_password: sp_encrypted_pv_key[:pv_key_password], + expiry: expiry, + encryption_opts: encryption_opts ) expect(builder.session_expiry).to eq(8) end @@ -212,19 +232,22 @@ module SamlIdp UserWithUniqueId = Struct.new(:unique_identifier, :email, :asserted_attributes) principal = UserWithUniqueId.new('unique_identifier_123456', 'foo@example.com', { emailAddress: { getter: :email } }) builder = described_class.new( - reference_id, - issuer_uri, - principal, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry, - encryption_opts, - session_expiry, - name_id_formats_opt, - asserted_attributes_opt + reference_id: reference_id, + issuer_uri: issuer_uri, + principal: principal, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + raw_algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: sp_encrypted_pv_key[:sp_public_cert], + private_key: sp_encrypted_pv_key[:sp_encrypted_pv_key], + pv_key_password: sp_encrypted_pv_key[:pv_key_password], + expiry: expiry, + encryption_opts: encryption_opts, + session_expiry: session_expiry, + name_id_formats_opts: name_id_formats_opt, + asserted_attributes_opts: asserted_attributes_opt ) Timecop.travel(Time.zone.local(2010, 6, 1, 13, 0, 0)) do expect(builder.raw).to eq("http://sportngin.comunique_identifier_123456http://example.comurn:oasis:names:tc:SAML:2.0:ac:classes:Passwordfoo@example.com") @@ -250,19 +273,22 @@ module SamlIdp UserWithName = Struct.new(:email, :first_name, :last_name) principal = UserWithName.new('foo@example.com', 'George', 'Washington') builder = described_class.new( - reference_id, - issuer_uri, - principal, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry, - encryption_opts, - session_expiry, - name_id_formats_opt, - asserted_attributes_opt + reference_id: reference_id, + issuer_uri: issuer_uri, + principal: principal, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + raw_algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: sp_encrypted_pv_key[:sp_public_cert], + private_key: sp_encrypted_pv_key[:sp_encrypted_pv_key], + pv_key_password: sp_encrypted_pv_key[:pv_key_password], + expiry: expiry, + encryption_opts: encryption_opts, + session_expiry: session_expiry, + name_id_formats_opts: name_id_formats_opt, + asserted_attributes_opts: asserted_attributes_opt ) Timecop.travel(Time.zone.local(2010, 6, 1, 13, 0, 0)) do expect(builder.raw).to eq("http://sportngin.comfoo@example.comhttp://example.comurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordGeorgeWashington") From 686cb6a3707e5d25134d8c8cd06b8c15dd3afe11 Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 5 Nov 2024 00:42:40 +0100 Subject: [PATCH 13/22] Response builder with ceritificate --- lib/saml_idp/response_builder.rb | 26 +++++++++++++++------- spec/lib/saml_idp/response_builder_spec.rb | 15 ++++++++----- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/lib/saml_idp/response_builder.rb b/lib/saml_idp/response_builder.rb index 0686cd9b..9d076790 100644 --- a/lib/saml_idp/response_builder.rb +++ b/lib/saml_idp/response_builder.rb @@ -11,16 +11,31 @@ class ResponseBuilder attr_accessor :saml_request_id attr_accessor :assertion_and_signature attr_accessor :raw_algorithm + attr_accessor :public_cert + attr_accessor :private_key + attr_accessor :pv_key_password alias_method :reference_id, :response_id - def initialize(response_id, issuer_uri, saml_acs_url, saml_request_id, assertion_and_signature, raw_algorithm) + def initialize(response_id:, + issuer_uri:, + saml_acs_url:, + saml_request_id:, + assertion_and_signature:, + raw_algorithm:, + public_cert:, + private_key:, + pv_key_password: + ) self.response_id = response_id self.issuer_uri = issuer_uri self.saml_acs_url = saml_acs_url self.saml_request_id = saml_request_id self.assertion_and_signature = assertion_and_signature self.raw_algorithm = raw_algorithm + self.public_cert = public_cert + self.private_key = private_key + self.pv_key_password = pv_key_password end def encoded(signed_message: false, compress: false) @@ -31,15 +46,15 @@ def raw build end + private + def encode_raw_message(compress) Base64.strict_encode64(compress ? deflate(raw) : raw) end - private :encode_raw_message def encode_signed_message(compress) Base64.strict_encode64(compress ? deflate(signed) : signed) end - private :encode_signed_message def build resp_options = {} @@ -61,22 +76,17 @@ def build response << assertion_and_signature end end - private :build def response_id_string "_#{response_id}" end - alias_method :reference_id, :response_id - private :response_id_string def now_iso Time.now.utc.iso8601 end - private :now_iso def deflate(inflated) Zlib::Deflate.deflate(inflated, 9)[2..-5] end - private :deflate end end diff --git a/spec/lib/saml_idp/response_builder_spec.rb b/spec/lib/saml_idp/response_builder_spec.rb index 9105eede..ce5224e8 100644 --- a/spec/lib/saml_idp/response_builder_spec.rb +++ b/spec/lib/saml_idp/response_builder_spec.rb @@ -8,12 +8,15 @@ module SamlIdp let(:assertion_and_signature) { "http://sportngin.comstuffjon.phenow@sportngin.comhttp://example.comjon.phenow@sportngin.comurn:federation:authentication:windows" } let(:algorithm) { :sha256 } subject { described_class.new( - response_id, - issuer_uri, - saml_acs_url, - saml_request_id, - assertion_and_signature, - algorithm + response_id: response_id, + issuer_uri: issuer_uri, + saml_acs_url: saml_acs_url, + saml_request_id: saml_request_id, + assertion_and_signature: assertion_and_signature, + raw_algorithm: algorithm, + public_cert: sp_encrypted_pv_key[:sp_public_cert], + private_key: sp_encrypted_pv_key[:sp_encrypted_pv_key], + pv_key_password: sp_encrypted_pv_key[:pv_key_password], ) } before do From 03594c049c2ce71eea372e186bae681746626404 Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 5 Nov 2024 01:21:24 +0100 Subject: [PATCH 14/22] Use directly provided cert and pv key --- lib/saml_idp/controller.rb | 37 +++++----- lib/saml_idp/response_builder.rb | 3 +- lib/saml_idp/saml_response.rb | 96 +++++++++++++++---------- lib/saml_idp/signed_info_builder.rb | 2 +- spec/lib/saml_idp/saml_response_spec.rb | 75 ++++++++++--------- 5 files changed, 121 insertions(+), 92 deletions(-) diff --git a/lib/saml_idp/controller.rb b/lib/saml_idp/controller.rb index 9d0a16c9..13aed80b 100644 --- a/lib/saml_idp/controller.rb +++ b/lib/saml_idp/controller.rb @@ -57,6 +57,9 @@ def encode_authn_response(principal, opts = {}) audience_uri = opts[:audience_uri] || saml_request.issuer || saml_acs_url[/^(.*?\/\/.*?\/)/, 1] opt_issuer_uri = opts[:issuer_uri] || issuer_uri my_authn_context_classref = opts[:authn_context_classref] || authn_context_classref + public_cert = opts[:public_cert] || SamlIdp.config.pv_key_password + private_key = opts[:private_key] || SamlIdp.config.secret_key + pv_key_password = opts[:pv_key_password] || SamlIdp.config.pv_key_password acs_url = opts[:acs_url] || saml_acs_url expiry = opts[:expiry] || 60*60 session_expiry = opts[:session_expiry] @@ -70,23 +73,23 @@ def encode_authn_response(principal, opts = {}) compress_opts = opts[:compress] || false SamlResponse.new( - reference_id, - response_id, - opt_issuer_uri, - principal, - audience_uri, - saml_request_id, - acs_url, - (opts[:algorithm] || algorithm || default_algorithm), - my_authn_context_classref, - expiry, - encryption_opts, - session_expiry, - name_id_formats_opts, - asserted_attributes_opts, - signed_message_opts, - signed_assertion_opts, - compress_opts + reference_id: reference_id, + response_id: response_id, + opt_issuer_uri: opt_issuer_uri, + principal: principal, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + acs_url: acs_url, + raw_algorithm: (opts[:algorithm] || algorithm || default_algorithm), + my_authn_context_classref: my_authn_context_classref, + expiry: expiry, + encryption_opts: encryption_opts, + session_expiry: session_expiry, + name_id_formats_opts: name_id_formats_opts, + asserted_attributes_opts: asserted_attributes_opts, + signed_message_opts: signed_message_opts, + signed_assertion_opts: signed_assertion_opts, + compress_opts: compress_opts ).build end diff --git a/lib/saml_idp/response_builder.rb b/lib/saml_idp/response_builder.rb index 9d076790..fc278b98 100644 --- a/lib/saml_idp/response_builder.rb +++ b/lib/saml_idp/response_builder.rb @@ -17,7 +17,8 @@ class ResponseBuilder alias_method :reference_id, :response_id - def initialize(response_id:, + def initialize( + response_id:, issuer_uri:, saml_acs_url:, saml_request_id:, diff --git a/lib/saml_idp/saml_response.rb b/lib/saml_idp/saml_response.rb index 85cf48b1..ecda5cfe 100644 --- a/lib/saml_idp/saml_response.rb +++ b/lib/saml_idp/saml_response.rb @@ -12,8 +12,9 @@ class SamlResponse attr_accessor :saml_request_id attr_accessor :saml_acs_url attr_accessor :algorithm - attr_accessor :secret_key - attr_accessor :x509_certificate + attr_accessor :private_key + attr_accessor :pv_key_password + attr_accessor :public_cert attr_accessor :authn_context_classref attr_accessor :expiry attr_accessor :encryption_opts @@ -25,23 +26,26 @@ class SamlResponse attr_accessor :compression_opts def initialize( - reference_id, - response_id, - issuer_uri, - principal, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry = 60 * 60, - encryption_opts = nil, - session_expiry = 0, - name_id_formats_opts = nil, - asserted_attributes_opts = nil, - signed_message_opts = false, - signed_assertion_opts = true, - compression_opts = false + reference_id:, + response_id:, + issuer_uri:, + principal:, + audience_uri:, + saml_request_id:, + saml_acs_url:, + algorithm:, + authn_context_classref:, + public_cert:, + private_key:, + pv_key_password:, + expiry: 60 * 60, + encryption_opts: nil, + session_expiry: 0, + name_id_formats_opts: nil, + asserted_attributes_opts: nil, + signed_message_opts: false, + signed_assertion_opts: true, + compression_opts: false ) self.reference_id = reference_id @@ -52,8 +56,9 @@ def initialize( self.saml_request_id = saml_request_id self.saml_acs_url = saml_acs_url self.algorithm = algorithm - self.secret_key = secret_key - self.x509_certificate = x509_certificate + self.private_key = private_key + self.pv_key_password = pv_key_password + self.public_cert = public_cert self.authn_context_classref = authn_context_classref self.expiry = expiry self.encryption_opts = encryption_opts @@ -80,7 +85,8 @@ def signed_assertion assertion_builder.raw end end - private :signed_assertion + + private def encoded_message if signed_message_opts @@ -89,29 +95,41 @@ def encoded_message response_builder.encoded(signed_message: false, compress: compression_opts) end end - private :encoded_message def response_builder - ResponseBuilder.new(response_id, issuer_uri, saml_acs_url, saml_request_id, signed_assertion, algorithm) + ResponseBuilder.new( + response_id: response_id, + issuer_uri: issuer_uri, + saml_acs_url: saml_acs_url, + saml_request_id: saml_request_id, + assertion_and_signature: signed_assertion, + raw_algorithm: algorithm, + public_cert: public_cert, + private_key: private_key, + pv_key_password: pv_key_password, + ) end - private :response_builder def assertion_builder @assertion_builder ||= - AssertionBuilder.new(reference_id || SecureRandom.uuid, - issuer_uri, - principal, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry, - encryption_opts, - session_expiry, - name_id_formats_opts, - asserted_attributes_opts) + AssertionBuilder.new( + reference_id: (reference_id || SecureRandom.uuid), + issuer_uri: issuer_uri, + principal: principal, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + raw_algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: public_cert, + private_key: private_key, + pv_key_password: pv_key_password, + expiry: expiry, + encryption_opts: encryption_opts, + session_expiry: session_expiry, + name_id_formats_opts: name_id_formats_opts, + asserted_attributes_opts: asserted_attributes_opts + ) end - private :assertion_builder end end diff --git a/lib/saml_idp/signed_info_builder.rb b/lib/saml_idp/signed_info_builder.rb index b6b5391a..7708e006 100644 --- a/lib/saml_idp/signed_info_builder.rb +++ b/lib/saml_idp/signed_info_builder.rb @@ -69,7 +69,7 @@ def clean_algorithm_name def encoded rsa_key = private_key || SamlIdp.config.secret_key - key_secret = pv_key_password || SamlIdp.config.pv_key_password + key_secret = pv_key_password || SamlIdp.config.password key = OpenSSL::PKey::RSA.new(rsa_key, key_secret) Base64.strict_encode64(key.sign(algorithm.new, raw)) end diff --git a/spec/lib/saml_idp/saml_response_spec.rb b/spec/lib/saml_idp/saml_response_spec.rb index b79f8a2b..f702054c 100644 --- a/spec/lib/saml_idp/saml_response_spec.rb +++ b/spec/lib/saml_idp/saml_response_spec.rb @@ -28,43 +28,50 @@ module SamlIdp let(:unsigned_response_opts) { false } let(:signed_assertion_opts) { true } let(:compress_opts) { false } - let(:subject_encrypted) { described_class.new(reference_id, - response_id, - issuer_uri, - name_id, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry, - encryption_opts, - session_expiry, - nil, - nil, - unsigned_response_opts, - signed_assertion_opts, - compress_opts + let(:subject_encrypted) { described_class.new( + reference_id: reference_id, + response_id: response_id, + issuer_uri: issuer_uri, + principal: name_id, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: x509_certificate, + private_key: secret_key, + pv_key_password: nil, + expiry: expiry, + encryption_opts: encryption_opts, + session_expiry: session_expiry, + name_id_formats_opts: nil, + asserted_attributes_opts: nil, + signed_message_opts: unsigned_response_opts, + signed_assertion_opts: signed_assertion_opts, + compression_opts: compress_opts ) } - subject { described_class.new(reference_id, - response_id, - issuer_uri, - name_id, - audience_uri, - saml_request_id, - saml_acs_url, - algorithm, - authn_context_classref, - expiry, - nil, - session_expiry, - nil, - nil, - signed_response_opts, - signed_assertion_opts, - compress_opts + subject { described_class.new(reference_id: reference_id, + response_id: response_id, + issuer_uri: issuer_uri, + principal: name_id, + audience_uri: audience_uri, + saml_request_id: saml_request_id, + saml_acs_url: saml_acs_url, + algorithm: algorithm, + authn_context_classref: authn_context_classref, + public_cert: x509_certificate, + private_key: secret_key, + pv_key_password: nil, + expiry: expiry, + encryption_opts: nil, + session_expiry: session_expiry, + name_id_formats_opts: nil, + asserted_attributes_opts: nil, + signed_message_opts: signed_response_opts, + signed_assertion_opts: signed_assertion_opts, + compression_opts: compress_opts ) } From b1c2cc906dd7174d2692a09210021e3cb7789569 Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 5 Nov 2024 01:24:26 +0100 Subject: [PATCH 15/22] Remove config dependency from low layer logics --- lib/saml_idp/algorithmable.rb | 5 ++--- lib/saml_idp/signed_info_builder.rb | 4 +--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/saml_idp/algorithmable.rb b/lib/saml_idp/algorithmable.rb index 354dbb92..182d6114 100644 --- a/lib/saml_idp/algorithmable.rb +++ b/lib/saml_idp/algorithmable.rb @@ -1,10 +1,9 @@ module SamlIdp module Algorithmable def algorithm - algorithm_check = raw_algorithm || SamlIdp.config.algorithm - return algorithm_check if algorithm_check.respond_to?(:digest) + return raw_algorithm if raw_algorithm.respond_to?(:digest) begin - OpenSSL::Digest.const_get(algorithm_check.to_s.upcase) + OpenSSL::Digest.const_get(raw_algorithm.to_s.upcase) rescue NameError OpenSSL::Digest::SHA1 end diff --git a/lib/saml_idp/signed_info_builder.rb b/lib/saml_idp/signed_info_builder.rb index 7708e006..a858197e 100644 --- a/lib/saml_idp/signed_info_builder.rb +++ b/lib/saml_idp/signed_info_builder.rb @@ -68,9 +68,7 @@ def clean_algorithm_name end def encoded - rsa_key = private_key || SamlIdp.config.secret_key - key_secret = pv_key_password || SamlIdp.config.password - key = OpenSSL::PKey::RSA.new(rsa_key, key_secret) + key = OpenSSL::PKey::RSA.new(private_key, pv_key_password) Base64.strict_encode64(key.sign(algorithm.new, raw)) end From 4189fe141dd4ce27966566d181b4bde6953eaa41 Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 5 Nov 2024 01:31:42 +0100 Subject: [PATCH 16/22] Use correct attribute name --- lib/saml_idp/controller.rb | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/saml_idp/controller.rb b/lib/saml_idp/controller.rb index 13aed80b..89c5a13f 100644 --- a/lib/saml_idp/controller.rb +++ b/lib/saml_idp/controller.rb @@ -57,9 +57,9 @@ def encode_authn_response(principal, opts = {}) audience_uri = opts[:audience_uri] || saml_request.issuer || saml_acs_url[/^(.*?\/\/.*?\/)/, 1] opt_issuer_uri = opts[:issuer_uri] || issuer_uri my_authn_context_classref = opts[:authn_context_classref] || authn_context_classref - public_cert = opts[:public_cert] || SamlIdp.config.pv_key_password + public_cert = opts[:public_cert] || SamlIdp.config.x509_certificate private_key = opts[:private_key] || SamlIdp.config.secret_key - pv_key_password = opts[:pv_key_password] || SamlIdp.config.pv_key_password + pv_key_password = opts[:pv_key_password] || SamlIdp.config.password acs_url = opts[:acs_url] || saml_acs_url expiry = opts[:expiry] || 60*60 session_expiry = opts[:session_expiry] @@ -75,13 +75,16 @@ def encode_authn_response(principal, opts = {}) SamlResponse.new( reference_id: reference_id, response_id: response_id, - opt_issuer_uri: opt_issuer_uri, + issuer_uri: opt_issuer_uri, principal: principal, audience_uri: audience_uri, saml_request_id: saml_request_id, - acs_url: acs_url, - raw_algorithm: (opts[:algorithm] || algorithm || default_algorithm), - my_authn_context_classref: my_authn_context_classref, + saml_acs_url: acs_url, + algorithm: (opts[:algorithm] || algorithm || default_algorithm), + authn_context_classref: my_authn_context_classref, + public_cert: public_cert, + private_key: private_key, + pv_key_password: pv_key_password, expiry: expiry, encryption_opts: encryption_opts, session_expiry: session_expiry, @@ -89,7 +92,7 @@ def encode_authn_response(principal, opts = {}) asserted_attributes_opts: asserted_attributes_opts, signed_message_opts: signed_message_opts, signed_assertion_opts: signed_assertion_opts, - compress_opts: compress_opts + compression_opts: compress_opts ).build end From 57254e71dee0f77d493ccd1025fdfbbfb9dfa737 Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 5 Nov 2024 01:51:41 +0100 Subject: [PATCH 17/22] Remove config dependency from low level logics --- lib/saml_idp/controller.rb | 13 +++++++----- lib/saml_idp/logout_builder.rb | 16 +++++++++++++- lib/saml_idp/logout_request_builder.rb | 21 +++++++++++++++++-- lib/saml_idp/logout_response_builder.rb | 21 +++++++++++++++++-- lib/saml_idp/metadata_builder.rb | 10 +++++++++ .../saml_idp/logout_request_builder_spec.rb | 12 ++++++----- .../saml_idp/logout_response_builder_spec.rb | 12 ++++++----- 7 files changed, 85 insertions(+), 20 deletions(-) diff --git a/lib/saml_idp/controller.rb b/lib/saml_idp/controller.rb index 89c5a13f..804df719 100644 --- a/lib/saml_idp/controller.rb +++ b/lib/saml_idp/controller.rb @@ -98,11 +98,14 @@ def encode_authn_response(principal, opts = {}) def encode_logout_response(_principal, opts = {}) SamlIdp::LogoutResponseBuilder.new( - get_saml_response_id, - (opts[:issuer_uri] || issuer_uri), - saml_logout_url, - saml_request_id, - (opts[:algorithm] || algorithm || default_algorithm) + response_id: get_saml_response_id, + issuer_uri: (opts[:issuer_uri] || issuer_uri), + saml_slo_url: saml_logout_url, + saml_request_id: saml_request_id, + algorithm: (opts[:algorithm] || algorithm || default_algorithm), + public_cert: opts[:public_cert] || SamlIdp.config.x509_certificate, + private_key: opts[:private_key] || SamlIdp.config.secret_key, + pv_key_password: opts[:pv_key_password] || SamlIdp.config.password ).signed end diff --git a/lib/saml_idp/logout_builder.rb b/lib/saml_idp/logout_builder.rb index 7d049dd6..230e898c 100644 --- a/lib/saml_idp/logout_builder.rb +++ b/lib/saml_idp/logout_builder.rb @@ -7,12 +7,26 @@ class LogoutBuilder attr_accessor :issuer_uri attr_accessor :saml_slo_url attr_accessor :algorithm + attr_accessor :public_cert + attr_accessor :private_key + attr_accessor :pv_key_password - def initialize(response_id, issuer_uri, saml_slo_url, algorithm) + def initialize( + response_id:, + issuer_uri:, + saml_slo_url:, + algorithm:, + public_cert:, + private_key:, + pv_key_password: + ) self.response_id = response_id self.issuer_uri = issuer_uri self.saml_slo_url = saml_slo_url self.algorithm = algorithm + self.public_cert = public_cert + self.private_key = private_key + self.pv_key_password = pv_key_password end # this is an abstract base class. diff --git a/lib/saml_idp/logout_request_builder.rb b/lib/saml_idp/logout_request_builder.rb index b75f9315..87f76c89 100644 --- a/lib/saml_idp/logout_request_builder.rb +++ b/lib/saml_idp/logout_request_builder.rb @@ -3,8 +3,25 @@ module SamlIdp class LogoutRequestBuilder < LogoutBuilder attr_accessor :name_id - def initialize(response_id, issuer_uri, saml_slo_url, name_id, algorithm) - super(response_id, issuer_uri, saml_slo_url, algorithm) + def initialize( + response_id:, + issuer_uri:, + saml_slo_url:, + name_id:, + algorithm:, + public_cert:, + private_key:, + pv_key_password: nil + ) + super( + response_id: response_id, + issuer_uri: issuer_uri, + saml_slo_url: saml_slo_url, + algorithm: algorithm, + public_cert: public_cert, + private_key: private_key, + pv_key_password: pv_key_password + ) self.name_id = name_id end diff --git a/lib/saml_idp/logout_response_builder.rb b/lib/saml_idp/logout_response_builder.rb index fd14116b..ee9370cb 100644 --- a/lib/saml_idp/logout_response_builder.rb +++ b/lib/saml_idp/logout_response_builder.rb @@ -3,8 +3,25 @@ module SamlIdp class LogoutResponseBuilder < LogoutBuilder attr_accessor :saml_request_id - def initialize(response_id, issuer_uri, saml_slo_url, saml_request_id, algorithm) - super(response_id, issuer_uri, saml_slo_url, algorithm) + def initialize( + response_id:, + issuer_uri:, + saml_slo_url:, + saml_request_id:, + algorithm:, + public_cert:, + private_key:, + pv_key_password: nil + ) + super( + response_id: response_id, + issuer_uri: issuer_uri, + saml_slo_url: saml_slo_url, + algorithm: algorithm, + public_cert: public_cert, + private_key: private_key, + pv_key_password: pv_key_password + ) self.saml_request_id = saml_request_id end diff --git a/lib/saml_idp/metadata_builder.rb b/lib/saml_idp/metadata_builder.rb index 9a4afb92..044b0520 100644 --- a/lib/saml_idp/metadata_builder.rb +++ b/lib/saml_idp/metadata_builder.rb @@ -159,6 +159,16 @@ def x509_certificate .gsub(/\n/, "") end + alias_method :public_cert, :x509_certificate + + def private_key + SamlIdp.config.secret_key + end + + def pv_key_password + nil + end + %w[ support_email organization_name diff --git a/spec/lib/saml_idp/logout_request_builder_spec.rb b/spec/lib/saml_idp/logout_request_builder_spec.rb index 73f0a626..26890159 100644 --- a/spec/lib/saml_idp/logout_request_builder_spec.rb +++ b/spec/lib/saml_idp/logout_request_builder_spec.rb @@ -19,11 +19,13 @@ module SamlIdp subject do described_class.new( - response_id, - issuer_uri, - saml_slo_url, - name_id, - algorithm + response_id: response_id, + issuer_uri: issuer_uri, + saml_slo_url: saml_slo_url, + name_id: name_id, + algorithm: algorithm, + public_cert: SamlIdp::Default::X509_CERTIFICATE, + private_key: SamlIdp::Default::SECRET_KEY ) end diff --git a/spec/lib/saml_idp/logout_response_builder_spec.rb b/spec/lib/saml_idp/logout_response_builder_spec.rb index a4ca7bfd..44f730dd 100644 --- a/spec/lib/saml_idp/logout_response_builder_spec.rb +++ b/spec/lib/saml_idp/logout_response_builder_spec.rb @@ -19,11 +19,13 @@ module SamlIdp subject do described_class.new( - response_id, - issuer_uri, - saml_slo_url, - request_id, - algorithm + response_id: response_id, + issuer_uri: issuer_uri, + saml_slo_url: saml_slo_url, + saml_request_id: request_id, + algorithm: algorithm, + public_cert: SamlIdp::Default::X509_CERTIFICATE, + private_key: SamlIdp::Default::SECRET_KEY ) end From cf0c9d398bcdf0f55d9653409cf4a40759dfc280 Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 5 Nov 2024 01:54:20 +0100 Subject: [PATCH 18/22] Remove config dependency from low level logics and fix test --- lib/saml_idp/signature_builder.rb | 2 +- spec/lib/saml_idp/signature_builder_spec.rb | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/saml_idp/signature_builder.rb b/lib/saml_idp/signature_builder.rb index da45644c..fe6e4c39 100644 --- a/lib/saml_idp/signature_builder.rb +++ b/lib/saml_idp/signature_builder.rb @@ -25,7 +25,7 @@ def raw private def der_certificate - certificate_formatter(x509_certificate || SamlIdp.config.x509_certificate) + certificate_formatter(x509_certificate) end def signed_info diff --git a/spec/lib/saml_idp/signature_builder_spec.rb b/spec/lib/saml_idp/signature_builder_spec.rb index ff28575a..8fe612b2 100644 --- a/spec/lib/saml_idp/signature_builder_spec.rb +++ b/spec/lib/saml_idp/signature_builder_spec.rb @@ -5,7 +5,8 @@ module SamlIdp let(:raw_info) { "em8csGAWynywpe8S4nN64o56/4DosXi2XWMY6RJ6YfA=" } let(:signed_info) { double raw: raw_info, signed: signed } subject { described_class.new( - signed_info + signed_info, + SamlIdp::Default::X509_CERTIFICATE ) } before do From e9736049d7ee495058e664ca7291fb55e06b042a Mon Sep 17 00:00:00 2001 From: zogoo Date: Tue, 5 Nov 2024 02:10:06 +0100 Subject: [PATCH 19/22] Revert Proc approach --- README.md | 5 ----- lib/saml_idp/configurator.rb | 4 ++-- spec/lib/saml_idp/configurator_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 50f99ebe..d8c48dd7 100644 --- a/README.md +++ b/README.md @@ -78,11 +78,6 @@ KEY DATA -----END RSA PRIVATE KEY----- CERT - # x509_certificate, secret_key, and password may also be set from within a proc, for example: - # config.x509_certificate = -> { File.read("cert.pem") } - # config.secret_key = -> { SecretKeyFinder.key_for(id: 1) } - # config.password = -> { "password" } - # config.password = "secret_key_password" # config.algorithm = :sha256 # Default: sha1 only for development. # config.organization_name = "Your Organization" diff --git a/lib/saml_idp/configurator.rb b/lib/saml_idp/configurator.rb index 99a662a0..e645f912 100644 --- a/lib/saml_idp/configurator.rb +++ b/lib/saml_idp/configurator.rb @@ -25,8 +25,8 @@ class Configurator attr_accessor :logger def initialize - self.x509_certificate = -> { Default::X509_CERTIFICATE } - self.secret_key = -> { Default::SECRET_KEY } + self.x509_certificate = Default::X509_CERTIFICATE + self.secret_key = Default::SECRET_KEY self.algorithm = :sha1 self.reference_id_generator = ->() { SecureRandom.uuid } self.service_provider = OpenStruct.new diff --git a/spec/lib/saml_idp/configurator_spec.rb b/spec/lib/saml_idp/configurator_spec.rb index 80da3b14..59c399a8 100644 --- a/spec/lib/saml_idp/configurator_spec.rb +++ b/spec/lib/saml_idp/configurator_spec.rb @@ -20,11 +20,11 @@ module SamlIdp it { should respond_to :logger } it "has a valid x509_certificate" do - expect(subject.x509_certificate.call).to eq(Default::X509_CERTIFICATE) + expect(subject.x509_certificate).to eq(Default::X509_CERTIFICATE) end it "has a valid secret_key" do - expect(subject.secret_key.call).to eq(Default::SECRET_KEY) + expect(subject.secret_key).to eq(Default::SECRET_KEY) end it "has a valid algorithm" do From a00cef851cc1bcb91ed87522213d89ba1aa37af7 Mon Sep 17 00:00:00 2001 From: zogoo Date: Sun, 10 Nov 2024 19:44:34 +0100 Subject: [PATCH 20/22] Assertion flag should able switchable by application (#24) Co-authored-by: zogoo --- lib/saml_idp/controller.rb | 2 +- spec/lib/saml_idp/controller_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/saml_idp/controller.rb b/lib/saml_idp/controller.rb index 9d0a16c9..a8e689f4 100644 --- a/lib/saml_idp/controller.rb +++ b/lib/saml_idp/controller.rb @@ -66,7 +66,7 @@ def encode_authn_response(principal, opts = {}) signed_message_opts = opts[:signed_message] || false name_id_formats_opts = opts[:name_id_formats] || nil asserted_attributes_opts = opts[:attributes] || nil - signed_assertion_opts = opts[:signed_assertion] || true + signed_assertion_opts = opts[:signed_assertion].nil? ? true : opts[:signed_assertion] compress_opts = opts[:compress] || false SamlResponse.new( diff --git a/spec/lib/saml_idp/controller_spec.rb b/spec/lib/saml_idp/controller_spec.rb index 94e189e9..e33d1592 100644 --- a/spec/lib/saml_idp/controller_spec.rb +++ b/spec/lib/saml_idp/controller_spec.rb @@ -66,6 +66,16 @@ def params end end + context '#encode_authn_response' do + it 'uses default values when opts are not provided' do + saml_response = encode_authn_response(principal, { audience_uri: 'http://example.com/issuer', issuer_uri: 'http://example.com', acs_url: 'https://foo.example.com/saml/consume', signed_assertion: false }) + + response = OneLogin::RubySaml::Response.new(saml_response) + response.settings = saml_settings + expect(response.document.to_s).to_not include("") + end + end + context "solicited Response" do before(:each) do params[:SAMLRequest] = make_saml_request From e163c4d87dfcb818ed0b2c44be14c0b59dbbb32d Mon Sep 17 00:00:00 2001 From: zogoo Date: Mon, 20 Jan 2025 16:53:35 +0100 Subject: [PATCH 21/22] concurrent-ruby v1.3.5 has removed the dependency on logger (#27) Co-authored-by: zogoo --- gemfiles/rails_5.2.gemfile | 1 + gemfiles/rails_6.1.gemfile | 1 + gemfiles/rails_7.0.gemfile | 1 + 3 files changed, 3 insertions(+) diff --git a/gemfiles/rails_5.2.gemfile b/gemfiles/rails_5.2.gemfile index 92f56759..0d08dd87 100644 --- a/gemfiles/rails_5.2.gemfile +++ b/gemfiles/rails_5.2.gemfile @@ -4,5 +4,6 @@ source "https://rubygems.org" gem "rails", "~> 5.2.4" gem "activeresource", "~> 5.1.0" +gem 'concurrent-ruby', '1.3.4' gemspec path: "../" diff --git a/gemfiles/rails_6.1.gemfile b/gemfiles/rails_6.1.gemfile index 34b89b00..c22c2925 100644 --- a/gemfiles/rails_6.1.gemfile +++ b/gemfiles/rails_6.1.gemfile @@ -4,5 +4,6 @@ source "https://rubygems.org" gem "rails", "~> 6.1.0" gem "activeresource", "~> 5.1.0" +gem 'concurrent-ruby', '1.3.4' gemspec path: "../" diff --git a/gemfiles/rails_7.0.gemfile b/gemfiles/rails_7.0.gemfile index f759cc55..1e30a8f2 100644 --- a/gemfiles/rails_7.0.gemfile +++ b/gemfiles/rails_7.0.gemfile @@ -4,5 +4,6 @@ source "https://rubygems.org" gem "rails", "~> 7.0.0" gem "activeresource", "~> 6.0.0" +gem 'concurrent-ruby', '1.3.4' gemspec path: "../" From 545cf91955eb441bdf7b01bcfb5c9ef0d4de3cb6 Mon Sep 17 00:00:00 2001 From: Massimo Zappino <99500013+mzappino-noz@users.noreply.github.com> Date: Mon, 20 Jan 2025 17:01:46 +0100 Subject: [PATCH 22/22] MetadataBuilder uses custom configurator (#25) Co-authored-by: Andrea Lorenzetti <64900248+andnoz@users.noreply.github.com> --- lib/saml_idp/metadata_builder.rb | 4 +-- spec/lib/saml_idp/metadata_builder_spec.rb | 31 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/saml_idp/metadata_builder.rb b/lib/saml_idp/metadata_builder.rb index 5d77caad..246297ed 100644 --- a/lib/saml_idp/metadata_builder.rb +++ b/lib/saml_idp/metadata_builder.rb @@ -152,7 +152,7 @@ def raw_algorithm private :raw_algorithm def x509_certificate - certificate = SamlIdp.config.x509_certificate.is_a?(Proc) ? SamlIdp.config.x509_certificate.call : SamlIdp.config.x509_certificate + certificate = configurator.x509_certificate.is_a?(Proc) ? configurator.x509_certificate.call : configurator.x509_certificate certificate .to_s .gsub(/-----BEGIN CERTIFICATE-----/,"") @@ -163,7 +163,7 @@ def x509_certificate alias_method :public_cert, :x509_certificate def private_key - SamlIdp.config.secret_key + configurator.secret_key end def pv_key_password diff --git a/spec/lib/saml_idp/metadata_builder_spec.rb b/spec/lib/saml_idp/metadata_builder_spec.rb index 453a8d81..2b84c295 100644 --- a/spec/lib/saml_idp/metadata_builder_spec.rb +++ b/spec/lib/saml_idp/metadata_builder_spec.rb @@ -71,5 +71,36 @@ module SamlIdp subject.configurator.single_logout_service_redirect_location = 'https://example.com/saml/logout' expect(subject.fresh).to match('') end + + context 'with custom configurator' do + let(:certificate) {'a certificate'} + let(:configurator) do SamlIdp::Configurator.new.tap do |c| + c.secret_key = 'a private key' + c.x509_certificate = certificate + end + end + subject { described_class.new(configurator) } + + describe '.private_key' do + it 'returns the given private_key' do + expect(subject.private_key).to eq(configurator.secret_key) + end + end + + describe '.x509_certificate' do + context 'with a given certificate string' do + it 'returns the given certificate' do + expect(subject.x509_certificate).to eq('a certificate') + end + end + + context 'with a given certificate proc' do + let(:certificate) {Proc.new { "a certificate from proc"}} + it 'returns the given certificate' do + expect(subject.x509_certificate).to eq('a certificate from proc') + end + end + end + end end end