[bug] Flaky test were passing somehow before

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

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,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
 
@@ -117,15 +128,32 @@ 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
+
+      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
@@ -197,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

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
@@ -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(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
+  end
 end

spec/lib/saml_idp/incoming_metadata_spec.rb

@@ -1,4 +1,5 @@
 require 'spec_helper'
+
 module SamlIdp
 
   metadata_1 = <<-eos
@@ -29,11 +30,52 @@ module SamlIdp
 </md:EntityDescriptor>
   eos
 
+  metadata_5 = <<-eos
+<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="test" entityID="https://test-saml.com/saml">
+  <md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
+    <md:KeyDescriptor>
+      <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
+        <ds:X509Data>
+          <ds:X509Certificate>MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnht3GR...</ds:X509Certificate>
+        </ds:X509Data>
+      </ds:KeyInfo>
+    </md:KeyDescriptor>
+  </md:SPSSODescriptor>
+</md:EntityDescriptor>
+  eos
+
+  metadata_6 = <<-eos
+<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="test" entityID="https://test-saml.com/saml">
+  <md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
+    <md:KeyDescriptor use="signing">
+      <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
+        <ds:X509Data>
+          <ds:X509Certificate>MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAmw6vGr...</ds:X509Certificate>
+        </ds:X509Data>
+      </ds:KeyInfo>
+    </md:KeyDescriptor>
+  </md:SPSSODescriptor>
+</md:EntityDescriptor>
+  eos
+
+  metadata_7 = <<-eos
+<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="test" entityID="https://test-saml.com/saml">
+  <md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
+    <md:KeyDescriptor use="encryption">
+      <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
+        <ds:X509Data>
+          <ds:X509Certificate>MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1dX3Gr...</ds:X509Certificate>
+        </ds:X509Data>
+      </ds:KeyInfo>
+    </md:KeyDescriptor>
+  </md:SPSSODescriptor>
+</md:EntityDescriptor>
+  eos
+
   describe IncomingMetadata do
     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
@@ -56,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

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