Improvement according the SAML specification

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

lib/saml_idp/controller.rb

@@ -33,15 +33,18 @@ def acs_url
     end
 
     def validate_saml_request(raw_saml_request = params[:SAMLRequest])
-      decode_request(raw_saml_request)
-      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)
+      decode_request(raw_saml_request, params[:Signature], params[:SigAlg], params[:RelayState])
+      valid_saml_request?
+    end
+
+    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

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",

lib/saml_idp/request.rb

@@ -3,7 +3,9 @@
 require 'logger'
 module SamlIdp
   class Request
-    def self.from_deflated_request(raw)
+    attr_accessor :errors
+
+    def self.from_deflated_request(raw, external_attributes = {})
       if raw
         decoded = Base64.decode64(raw)
         zstream = Zlib::Inflate.new(-Zlib::MAX_WBITS)
@@ -18,18 +20,23 @@ 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]
+      self.errors = []
     end
 
     def logout_request?
@@ -85,30 +92,53 @@ def log(msg)
       end
     end
 
-    def valid?
+    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
 
-      unless valid_signature?
-        log "Signature is invalid in #{raw_xml}"
+      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
 
@@ -117,15 +147,39 @@ def valid?
 
     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?
-        document.valid_signature?(service_provider.fingerprint)
+      if logout_request? || authn_request? && validate_auth_request_signature?
+        document.valid_signature?(service_provider.cert, service_provider.fingerprint)
       else
         true
       end
     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
+      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
+
+      result = cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string)
+      # Match all percent-encoded sequences (e.g., %20, %2B) and convert them to lowercase
+      # Upper case is recommended for consistency but some services such as MS Entra Id not follows it
+      # https://datatracker.ietf.org/doc/html/rfc3986#section-2.1
+      result || cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string.gsub(/%[A-F0-9]{2}/) { |match| match.downcase })
+    rescue OpenSSL::X509::CertificateError => e
+      log e.message
+      collect_errors(:cert_format_error)
+    end
+
     def service_provider?
       service_provider && service_provider.valid?
     end
@@ -148,6 +202,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
@@ -197,5 +258,14 @@ def service_provider_finder
       config.service_provider.finder
     end
     private :service_provider_finder
+
+    def validate_auth_request_signature?
+      # Validate signature when metadata specify AuthnRequest should be signed
+      metadata = service_provider.current_metadata
+      sign_authn_request = metadata.respond_to?(:sign_authn_request?) && metadata.sign_authn_request?
+      sign_authn_request = service_provider.sign_authn_request unless service_provider.sign_authn_request.nil? 
+      sign_authn_request
+    end
+    private :validate_auth_request_signature?
   end
 end

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

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)

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')

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
@@ -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)
@@ -124,4 +114,32 @@ 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(security_options: { embed_sign: false })
+      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
+
+    context "solicited Response" do
+      let(:principal) { double email_address: "[email protected]" }
+
+      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