[improvements] Return error to application and support SLO request va…

…lidation (#224) * Squash commits for saml_idp gem * [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 <[email protected]> * [feat] Don’t ignore certificates without usage (#17) I have tested with live SAML SP apps and it works fine * Unspecified certifciate from SP metadata --------- Co-authored-by: zogoo <[email protected]> * wip add error collector * Fix type and rewrite request with proper validation test cases * 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 <[email protected]> Co-authored-by: zogoo <[email protected]> * Lead error render decision to gem user * Validate the certificate's existence before verifying the signature. * [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 <[email protected]> * Support lowercase percent-encoded sequences for URL encoding (#20) Co-authored-by: zogoo <[email protected]> * Remove duplications * Pre-conditions need to be defined in before section * Le's not test logger in here * Let's not break anything for now * Rename correctly --------- Co-authored-by: zogoo <[email protected]> Co-authored-by: Mathieu Jobin <[email protected]>
saml-idp · Oct 29, 2024 · 4b7e4c8 · 4b7e4c8
1 parent 12416c4
commit 4b7e4c8
Show file tree

Hide file tree

Showing 10 changed files with 344 additions and 192 deletions.
diff --git 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/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
 
@@ -128,16 +147,16 @@ 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
       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
@@ -151,7 +170,14 @@ 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)
     end
 
     def service_provider?
@@ -232,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
diff --git 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/saml_idp.gemspec b/saml_idp.gemspec
@@ -54,8 +54,8 @@ Gem::Specification.new do |s|
   s.add_development_dependency('appraisal')
   s.add_development_dependency('capybara', '>= 2.16')
   s.add_development_dependency('rails', '>= 5.2')
-  s.add_development_dependency('rake')
   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')
   s.add_development_dependency('simplecov')

diff --git 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/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)
@@ -128,7 +118,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']
@@ -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: "[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
diff --git 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
 </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)
@@ -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