From 3e58ea24348c2ba96bca3f138e728310ff8c6e82 Mon Sep 17 00:00:00 2001 From: zogoo Date: Thu, 25 Jul 2024 00:50:53 +0800 Subject: [PATCH 1/2] 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 41aad96f30d155fb89aee4ded6c3260b246963c0 Mon Sep 17 00:00:00 2001 From: zogoo Date: Thu, 24 Oct 2024 23:18:29 +0200 Subject: [PATCH 2/2] Add explanation for external attributes of decode_request method --- README.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 7d70b75c..d8c48dd7 100644 --- a/README.md +++ b/README.md @@ -24,9 +24,13 @@ Add this to your Gemfile: Include `SamlIdp::Controller` and see the examples that use rails. It should be straightforward for you. -Basically you call `decode_request(params[:SAMLRequest])` on an incoming request and then use the value -`saml_acs_url` to determine the source for which you need to authenticate a user. How you authenticate -a user is entirely up to you. +Basically, you call `decode_request(params[:SAMLRequest])` on an incoming request and then use the value +`saml_acs_url` to determine the source for which you need to authenticate a user. +If the signature (`Signature`) and signing algorithm (`SigAlg`) are provided as external parameters in the request, +you can pass those parameters as `decode_request(params[:SAMLRequest], params[:Signature], params[:SigAlg], params[:RelayState])`. +Then, you can verify the request signature with the `valid?` method. + +How you authenticate a user is entirely up to you. Once a user has successfully authenticated on your system send the Service Provider a SAMLResponse by posting to `saml_acs_url` the parameter `SAMLResponse` with the return value from a call to