Only raise request errors

lib/saml_idp.rb

@@ -75,9 +75,8 @@ def options_have_signature(options)
       private :options_have_signature
 
       def valid_signature?(fingerprint, options = {})
-        soft_fail = options[:soft].nil? ? true : options[:soft]
         (signed? || options_have_signature(options)) &&
-          signed_document.validate(fingerprint, soft_fail, options)
+          signed_document.validate(fingerprint, soft = true, options)
       end
 
       def signed_document

lib/saml_idp/request.rb

@@ -152,11 +152,9 @@ def valid?
 
       if service_provider?
         begin
-          unless valid_signature?
-            log "Signature is invalid in #{raw_xml}"
-            errors.push(:invalid_signature)
-          end
+          valid_signature?
         rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError => e
+          log e.message
           errors.push(e.error_code)
         end
       end
@@ -171,7 +169,7 @@ def signed?
     def valid_signature?
       # Force signatures for logout requests because there is no other
       # protection against a cross-site DoS.
-      service_provider.valid_signature?(document, logout_request?, options.merge(soft: false))
+      service_provider.valid_signature?(document, logout_request?, options)
     end
 
     def service_provider?

lib/saml_idp/service_provider.rb

@@ -21,23 +21,33 @@ def valid?
     end
 
     def valid_signature?(doc, require_signature = false, options = {})
-      @matching_cert = Array(certs).find do |cert|
-        doc.valid_signature?(fingerprint_cert(cert), options.merge(cert: cert))
-      end
+      return true unless require_signature || should_validate_signature?
 
-      no_raising_errors = options[:soft].nil? ? true : options[:soft]
+      if cert_array.empty?
+        raise SamlIdp::XMLSecurity::SignedDocument::ValidationError.new(
+          'No cert',
+          :no_cert_registered
+        )
+      end
 
-      if require_signature || should_validate_signature?
-        if certs.blank? && !no_raising_errors
-          raise SamlIdp::XMLSecurity::SignedDocument::ValidationError.new(
-            'No certificate registered',
-            :no_cert_registered
+      @matching_cert = cert_array.find do |cert|
+        doc.valid_signature?(fingerprint_cert(cert), options.merge(
+          {
+            cert: cert,
+            # we only want to raise request errors, not individual certificate errors
+            raise_request_errors: true}
           )
-        end
-        !!@matching_cert
-      else
-        true
+        )
+      end
+
+      if @matching_cert.nil?
+        # if no ValidationErrors, but no matching certs, the request was signed with an
+        # unregistered certificate
+        raise SamlIdp::XMLSecurity::SignedDocument::ValidationError.new(
+          'No matching cert',
+          :no_matching_cert)
       end
+      true
     end
 
     # @param [OpenSSL::X509::Certificate] ssl_cert
@@ -90,5 +100,11 @@ def request_metadata
       metadata_url.present? ? Faraday.get(metadata_url).body : ""
     end
     private :request_metadata
+
+    private
+
+    def cert_array
+      Array(certs)
+    end
   end
 end

lib/saml_idp/xml_security.rb

@@ -45,13 +45,16 @@ def initialize(msg=nil, error_code=nil)
       DSIG = "http://www.w3.org/2000/09/xmldsig#"
 
       attr_accessor :signed_element_id
+      attr_accessor :raise_request_errors
 
       def initialize(response)
         super(response)
         extract_signed_element_id
       end
 
       def validate(idp_cert_fingerprint, soft = true, options = {})
+        @raise_request_errors = options[:raise_request_errors]
+
         log 'Validate the fingerprint'
         base64_cert = find_base64_cert(options)
         cert_text   = Base64.decode64(base64_cert)
@@ -132,8 +135,7 @@ def find_base64_cert(options)
         else
           raise ValidationError.new(
             'Certificate element missing in response (ds:X509Certificate) and not provided in options[:cert]',
-            :cert_missing
-          )
+            :cert_missing)
         end
       end
 
@@ -201,10 +203,13 @@ def validate_doc_embedded_signature(base64_cert, soft = true)
           digest_value                  = Base64.decode64(REXML::XPath.first(ref, "//ds:DigestValue", sig_namespace_hash).text)
 
           unless digests_match?(hash, digest_value)
-            return soft ? false : (raise ValidationError.new(
-              "Digest mismatch",
-              :digest_mismatch
-            ))
+            if @raise_request_errors || !soft
+              raise ValidationError.new(
+                "Digest mismatch",
+                :digest_mismatch
+              )
+            end
+            return false
           end
         end
 
@@ -221,10 +226,10 @@ def verify_signature(base64_cert, sig_alg, signature, canon_string, soft)
         cert                = OpenSSL::X509::Certificate.new(cert_text)
         signature_algorithm = algorithm(sig_alg)
 
-        if signature_algorithm != OpenSSL::Digest::SHA256 && !soft
-         raise ValidationError.new(
-            'All signatures must use RSA SHA-256',
-            :require_sha256
+        if signature_algorithm != OpenSSL::Digest::SHA256 && @raise_request_errors
+          raise ValidationError.new(
+              'All signatures must use RSA SHA-256',
+              :require_sha256
           )
         end
 

spec/lib/saml_idp/request_spec.rb

@@ -364,23 +364,13 @@ module SamlIdp
               it 'is not valid' do
                 expect(subject.valid?).to eq false
               end
-
-              it 'adds an error to request object' do
-                subject.valid?
-                expect(subject.errors.include?(:invalid_certificate)).to be true
-              end
             end
 
             describe 'none of the service provider certs match the signed document' do
               let(:registered_cert) { OpenSSL::X509::Certificate.new(cloudhsm_idp_x509_cert) }
               it 'is not valid' do
                 expect(subject.valid?).to eq false
               end
-
-              it 'adds a key validation error to request object' do
-                subject.valid?
-                expect(subject.errors.include?(:key_validation_error)).to be true
-              end
             end
 
             describe 'service provider has no certificate registered' do
@@ -398,12 +388,7 @@ module SamlIdp
 
             describe 'fingerprint mismatch' do
               before do
-                allow(subject.service_provider).to receive(:valid_signature?).and_raise(
-                  SamlIdp::XMLSecurity::SignedDocument::ValidationError.new(
-                    'Fingerprint mismatch',
-                    :fingerprint_mismatch
-                  )
-                )
+                expect(Base64).to receive(:decode64).with(expected_cert) { cloudhsm_idp_x509_cert }
               end
 
               it 'is not valid' do
@@ -412,7 +397,7 @@ module SamlIdp
 
               it 'adds a fingerprint mismatch error' do
                 subject.valid?
-                expect(subject.errors.include?(:fingerprint_mismatch)).to be true
+                expect(subject.errors.include?(:no_matching_cert)).to be true
               end
             end
 
@@ -461,13 +446,10 @@ module SamlIdp
             end
 
             describe 'digest mismatch' do
+              let(:options) { {} }
+              let(:request_saml) { signed_auth_request }
               before do
-                allow(subject.service_provider).to receive(:valid_signature?).and_raise(
-                  SamlIdp::XMLSecurity::SignedDocument::ValidationError.new(
-                    'Digest mismatch',
-                    :digest_mismatch
-                  )
-                )
+                allow(OpenSSL::Digest::SHA256).to receive(:digest) { 'incorrect_hash' }
               end
 
               it 'is not valid' do

spec/lib/saml_idp/service_provider_spec.rb

@@ -48,16 +48,22 @@ module SamlIdp
         before { subject.validate_signature = true }
 
         describe 'a cert is not present in the document' do
+          let(:raise_request_errors) { nil }
           let(:raw_xml) do
             SamlIdp::Request.from_deflated_request(
               signed_auth_request_options['SAMLRequest']
             ).raw_xml
           end
-          let(:options) { {get_params: signed_auth_request_options}.with_indifferent_access }
+          let(:options) do
+            {
+              get_params: signed_auth_request_options,
+              raise_request_errors: raise_request_errors
+            }.with_indifferent_access
+          end
 
           describe 'the service provider has no certs' do
-            it 'returns false' do
-              expect(subject.valid_signature?(doc, true, options)).to be false
+            it 'raises an error' do
+              expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError,  'No cert')
             end
           end
 
@@ -72,17 +78,18 @@ module SamlIdp
 
             describe 'one invalid cert' do
               before { subject.certs = [invalid_cert]}
-
-              it 'returns false' do
-                expect(subject.valid_signature?(doc, true, options)).to be false
+              it 'raises an error' do
+                expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError,  'No matching cert')
               end
             end
 
             describe 'multiple certs' do
               before { subject.certs = [invalid_cert, cert] }
 
-              it 'returns true' do
-                expect(subject.valid_signature?(doc, true, options)).to be true
+              describe "one is valid" do
+                it 'returns true' do
+                  expect(subject.valid_signature?(doc, true, options)).to be true
+                end
               end
             end
           end
@@ -96,14 +103,8 @@ module SamlIdp
           end
 
           describe 'the service provider has no certs' do
-            it 'returns false' do
-              expect(subject.valid_signature?(doc, true)).to be false
-            end
-
-            describe 'the requirement is passed through the method' do
-              it 'returns false' do
-                expect(subject.valid_signature?(doc, true)).to be false
-              end
+            it 'raises an error' do
+              expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError,  'No cert')
             end
           end
 
@@ -119,8 +120,8 @@ module SamlIdp
             describe 'one invalid cert' do
               before { subject.certs = [invalid_cert]}
 
-              it 'returns false' do
-                expect(subject.valid_signature?(doc, true)).to be false
+              it 'raises an error' do
+                expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError,  'No matching cert')
               end
             end
 
@@ -145,13 +146,8 @@ module SamlIdp
               describe 'the valid cert is not registered in the idp' do
                 before { subject.certs = [other_cert, invalid_cert] }
 
-                it 'returns false' do
-                  expect(subject.valid_signature?(doc, true)).to be false
-                end
-
-                it 'matches the right cert' do
-                  subject.valid_signature?(doc)
-                  expect(subject.matching_cert).to be nil
+                it 'raises an error' do
+                  expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError,  'No matching cert')
                 end
               end
             end

spec/xml_security_spec.rb

@@ -41,15 +41,19 @@ module SamlIdp
           )
         end
 
-        it 'raises require SHA256 error' do
-          response = Base64.decode64(response_document)
-          response.sub!('<ds:DigestValue>pJQ7MS/ek4KRRWGmv/H43ReHYMs=</ds:DigestValue>',
-                        '<ds:DigestValue>b9xsAXLsynugg3Wc1CI3kpWku+0=</ds:DigestValue>')
-          document = XMLSecurity::SignedDocument.new(response)
-          base64cert = document.elements['//ds:X509Certificate'].text
-          expect { document.validate_doc(base64cert, false) }.to(
-            raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'All signatures must use RSA SHA-256')
-          )
+        describe 'when raising request errors' do
+          it 'raises require SHA256 error' do
+            response = Base64.decode64(response_document)
+            response.sub!('<ds:DigestValue>pJQ7MS/ek4KRRWGmv/H43ReHYMs=</ds:DigestValue>',
+                          '<ds:DigestValue>b9xsAXLsynugg3Wc1CI3kpWku+0=</ds:DigestValue>')
+            document = XMLSecurity::SignedDocument.new(response)
+            base64cert = document.elements['//ds:X509Certificate'].text
+
+            document.raise_request_errors = true
+            expect { document.validate_doc(base64cert, false, {raise_request_errors: true}) }.to(
+              raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'All signatures must use RSA SHA-256')
+            )
+          end
         end
 
         it 'raises Key validation error' do