See #545 More specific error messages for signature validation

Author: pitbulk

lib/onelogin/ruby-saml/response.rb

@@ -847,14 +847,15 @@ def validate_signature
         end
 
         if sig_elements.size != 1
-          if  sig_elements.size == 0
+          if sig_elements.size == 0
              append_error("Signed element id ##{doc.signed_element_id} is not found")
           else
              append_error("Signed element id ##{doc.signed_element_id} is found more than once")
           end
           return append_error(error_msg)
         end
 
+        old_errors = @errors.clone
 
         idp_certs = settings.get_idp_cert_multi
         if idp_certs.nil? || idp_certs[:signing].empty?
@@ -878,21 +879,27 @@ def validate_signature
           valid = false
           expired = false
           idp_certs[:signing].each do |idp_cert|
-            valid = doc.validate_document_with_cert(idp_cert)
+            valid = doc.validate_document_with_cert(idp_cert, true)
             if valid
               if settings.security[:check_idp_cert_expiration]
                 if OneLogin::RubySaml::Utils.is_cert_expired(idp_cert)
                   expired = true
                 end
               end
+
+              # At least one certificate is valid, restore the old accumulated errors
+              @errors = old_errors
               break
             end
+
           end
           if expired
             error_msg = "IdP x509 certificate expired"
             return append_error(error_msg)
           end
           unless valid
+            # Remove duplicated errors
+            @errors = @errors.uniq
             return append_error(error_msg)
           end
         end

lib/xml_security.rb

@@ -212,7 +212,7 @@ def validate_document(idp_cert_fingerprint, soft = true, options = {})
         begin
           cert = OpenSSL::X509::Certificate.new(cert_text)
         rescue OpenSSL::X509::CertificateError => _e
-          return append_error("Certificate Error", soft)
+          return append_error("Document Certificate Error", soft)
         end
 
         if options[:fingerprint_alg]
@@ -224,7 +224,6 @@ def validate_document(idp_cert_fingerprint, soft = true, options = {})
 
         # check cert matches registered idp cert
         if fingerprint != idp_cert_fingerprint.gsub(/[^a-zA-Z0-9]/,"").downcase
-          @errors << "Fingerprint mismatch"
           return append_error("Fingerprint mismatch", soft)
         end
       else
@@ -255,12 +254,12 @@ def validate_document_with_cert(idp_cert, soft = true)
         begin
           cert = OpenSSL::X509::Certificate.new(cert_text)
         rescue OpenSSL::X509::CertificateError => _e
-          return append_error("Certificate Error", soft)
+          return append_error("Document Certificate Error", soft)
         end
 
         # check saml response cert matches provided idp cert
         if idp_cert.to_pem != cert.to_pem
-          return false
+          return append_error("Certificate of the Signature element does not match provided certificate", soft)
         end
       else
         base64_cert = Base64.encode64(idp_cert.to_pem)
@@ -345,7 +344,6 @@ def validate_signature(base64_cert, soft = true)
       digest_value = Base64.decode64(OneLogin::RubySaml::Utils.element_text(encoded_digest_value))
 
       unless digests_match?(hash, digest_value)
-        @errors << "Digest mismatch"
         return append_error("Digest mismatch", soft)
       end
 

test/response_test.rb

@@ -897,6 +897,7 @@ def generate_audience_error(expected, actual)
         settings.idp_cert_fingerprint = signature_fingerprint_1
         response.settings = settings
         assert !response.send(:validate_signature)
+        assert_includes response.errors, "Fingerprint mismatch"
         assert_includes response.errors, "Invalid Signature on SAML Response"
       end
 
@@ -917,15 +918,29 @@ def generate_audience_error(expected, actual)
         assert_includes response_valid_signed.errors, "IdP x509 certificate expired"
       end
 
-      it "return false when no X509Certificate and the cert provided at settings mismatches" do
+      it "return false when X509Certificate and the cert provided at settings mismatches" do
         settings.idp_cert_fingerprint = nil
         settings.idp_cert = signature_1
         response_valid_signed_without_x509certificate.settings = settings
         assert !response_valid_signed_without_x509certificate.send(:validate_signature)
+        assert_includes response_valid_signed_without_x509certificate.errors, "Key validation error"
         assert_includes response_valid_signed_without_x509certificate.errors, "Invalid Signature on SAML Response"
       end
 
-      it "return true when no X509Certificate and the cert provided at settings matches" do
+      it "return false when X509Certificate has invalid content" do
+        settings.idp_cert_fingerprint = nil
+        settings.idp_cert = ruby_saml_cert_text
+        content = read_response('response_with_signed_message_and_assertion.xml')
+        content = content.sub(/<ds:X509Certificate>.*<\/ds:X509Certificate>/,
+                       "<ds:X509Certificate>an-invalid-certificate</ds:X509Certificate>")
+        response_invalid_x509certificate = OneLogin::RubySaml::Response.new(content)
+        response_invalid_x509certificate.settings = settings
+        assert !response_invalid_x509certificate.send(:validate_signature)
+        assert_includes response_invalid_x509certificate.errors, "Document Certificate Error"
+        assert_includes response_invalid_x509certificate.errors, "Invalid Signature on SAML Response"
+      end
+
+      it "return true when X509Certificate and the cert provided at settings matches" do
         settings.idp_cert_fingerprint = nil
         settings.idp_cert = ruby_saml_cert_text
         response_valid_signed_without_x509certificate.settings = settings
@@ -953,7 +968,7 @@ def generate_audience_error(expected, actual)
           :encryption => []
         }
         response_valid_signed.settings = settings
-        assert response_valid_signed.send(:validate_signature)
+        res = response_valid_signed.send(:validate_signature)
         assert_empty response_valid_signed.errors
       end
 
@@ -965,6 +980,7 @@ def generate_audience_error(expected, actual)
         }
         response_valid_signed.settings = settings
         assert !response_valid_signed.send(:validate_signature)
+        assert_includes response_valid_signed.errors, "Certificate of the Signature element does not match provided certificate"
         assert_includes response_valid_signed.errors, "Invalid Signature on SAML Response"
       end
     end

test/xml_security_test.rb

@@ -395,6 +395,11 @@ class XmlSecurityTest < Minitest::Test
     end
 
     describe '#validate_document_with_cert' do
+      let(:document_data) { read_response('response_with_signed_message_and_assertion.xml') }
+      let(:document) { OneLogin::RubySaml::Response.new(document_data).document }
+      let(:idp_cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text) }
+      let(:fingerprint) { '4b68c453c7d994aad9025c99d5efcf566287fe8d' }
+
       describe 'with invalid document ' do
         describe 'when certificate is invalid' do
           let(:document_data) { read_response('response_with_signed_message_and_assertion.xml')
@@ -408,13 +413,8 @@ class XmlSecurityTest < Minitest::Test
         end
       end
 
-      describe 'with valid document ' do
+      describe 'with valid document' do
         describe 'when response has cert' do
-          let(:document_data) { read_response('response_with_signed_message_and_assertion.xml') }
-          let(:document) { OneLogin::RubySaml::Response.new(document_data).document }
-          let(:idp_cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text) }
-          let(:fingerprint) { '4b68c453c7d994aad9025c99d5efcf566287fe8d' }
-
           it 'is valid' do
             assert document.validate_document_with_cert(idp_cert), 'Document should be valid'
           end
@@ -429,6 +429,43 @@ class XmlSecurityTest < Minitest::Test
           end
         end
       end
+
+      describe 'when response has no cert but you have local cert' do
+        let(:document_data) { response_document_valid_signed_without_x509certificate }
+
+        it 'is valid' do
+          assert document.validate_document_with_cert(idp_cert), 'Document should be valid'
+        end
+      end
+
+      describe 'when response cert is invalid' do
+        let(:document_data) do
+          contents = read_response('response_with_signed_message_and_assertion.xml')
+          contents.sub(/<ds:X509Certificate>.*<\/ds:X509Certificate>/,
+                       "<ds:X509Certificate>an-invalid-certificate</ds:X509Certificate>")
+        end
+
+        it 'is not valid' do
+          assert !document.validate_document_with_cert(idp_cert), 'Document should be valid'
+          assert_equal(["Document Certificate Error"], document.errors)
+        end
+      end
+
+      describe 'when response cert is different from idp cert' do
+        let(:idp_cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text2) }
+
+        it 'is not valid' do
+          exception = assert_raises(OneLogin::RubySaml::ValidationError) do
+            document.validate_document_with_cert(idp_cert, false)
+          end
+          assert_equal("Certificate of the Signature element does not match provided certificate", exception.message)
+        end
+
+        it 'is not valid (soft = true)' do
+          document.validate_document_with_cert(idp_cert)
+          assert_equal(["Certificate of the Signature element does not match provided certificate"], document.errors)
+        end
+      end
     end
   end
 end