Dmm/add invalid sig errors

lib/saml_idp.rb

@@ -76,7 +76,7 @@ def options_have_signature(options)
 
       def valid_signature?(fingerprint, options = {})
         (signed? || options_have_signature(options)) &&
-          signed_document.validate(fingerprint, :soft, options)
+          signed_document.validate(fingerprint, true, options)
       end
 
       def signed_document

lib/saml_idp/request.rb

@@ -132,7 +132,7 @@ def valid?
 
       unless service_provider?
         log "Unable to find service provider for issuer #{issuer}"
-        errors.push(:issuer_missing_or_invald)
+        errors.push(:issuer_missing_or_invalid)
       end
 
       if authn_request? && logout_request?
@@ -141,7 +141,7 @@ def valid?
       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?} "
+        log 'One and only one of authnrequest and logout request is required. '
         errors.push(:no_auth_or_logout_request)
       end
 
@@ -150,10 +150,13 @@ def valid?
         errors.push(:no_response_url)
       end
 
-      unless service_provider? && valid_signature?
-        log "Signature is invalid in #{raw_xml}"
-        # TODO: We should get more specific errors
-        errors.push(:invalid_signature)
+      if service_provider?
+        begin
+          valid_signature?
+        rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError => e
+          log e.message
+          errors.push(e.error_code)
+        end
       end
 
       errors.blank?

lib/saml_idp/service_provider.rb

@@ -21,17 +21,33 @@ def valid?
     end
 
     def valid_signature?(doc, require_signature = false, options = {})
-      Array(certs).any? do |cert|
-        if doc.valid_signature?(fingerprint_cert(cert), options.merge(cert: cert))
-          @matching_cert = cert
-        end
+      return true unless require_signature || should_validate_signature?
+
+      if cert_array.empty?
+        raise SamlIdp::XMLSecurity::SignedDocument::ValidationError.new(
+          'No cert',
+          :no_cert_registered
+        )
+      end
+
+      @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
 
-      if require_signature || should_validate_signature?
-        !!@matching_cert
-      else
-        true
+      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
@@ -84,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

@@ -32,26 +32,40 @@
 module SamlIdp
   module XMLSecurity
     class SignedDocument < REXML::Document
-      ValidationError = Class.new(StandardError)
+      class ValidationError < StandardError
+        attr_reader :error_code
+
+        def initialize(msg=nil, error_code=nil)
+          @error_code = error_code
+          super(msg)
+        end
+      end
+
       C14N = "http://www.w3.org/2001/10/xml-exc-c14n#"
       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)
         cert        =
           begin
             OpenSSL::X509::Certificate.new(cert_text)
           rescue OpenSSL::X509::CertificateError => e
-            return soft ? false : (raise ValidationError.new("Invalid certificate"))
+            return soft ? false : (raise ValidationError.new(
+              'Invalid certificate',
+              :invalid_certificate
+            ))
           end
 
         # check cert matches registered idp cert
@@ -60,7 +74,10 @@ def validate(idp_cert_fingerprint, soft = true, options = {})
         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"))
+          return soft ? false : (raise ValidationError.new(
+            'Fingerprint mismatch',
+            :fingerprint_mismatch
+          ))
         end
 
         validate_doc(base64_cert, soft, options)
@@ -100,17 +117,25 @@ def find_base64_cert(options)
         if cert_element
           return cert_element.text unless cert_element.text.blank?
 
-          raise ValidationError.new("Certificate element present in response (ds:X509Certificate) but evaluating to nil")
+          raise ValidationError.new(
+            "Certificate element present in response (ds:X509Certificate) but evaluating to nil",
+            :present_but_nil
+          )
         elsif options[:cert]
           if options[:cert].is_a?(String)
             options[:cert]
           elsif options[:cert].is_a?(OpenSSL::X509::Certificate)
             Base64.encode64(options[:cert].to_pem)
           else
-            raise ValidationError.new("options[:cert] must be Base64-encoded String or OpenSSL::X509::Certificate")
+            raise ValidationError.new(
+              'options[:cert] must be Base64-encoded String or OpenSSL::X509::Certificate',
+              :not_base64_or_cert
+            )
           end
         else
-          raise ValidationError.new("Certificate element missing in response (ds:X509Certificate) and not provided in options[:cert]")
+          raise ValidationError.new(
+            'Certificate element missing in response (ds:X509Certificate) and not provided in options[:cert]',
+            :cert_missing)
         end
       end
 
@@ -178,7 +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"))
+            if @raise_request_errors || !soft
+              raise ValidationError.new(
+                "Digest mismatch",
+                :digest_mismatch
+              )
+            end
+            return false
           end
         end
 
@@ -195,8 +226,18 @@ 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 && @raise_request_errors
+          raise ValidationError.new(
+              'All signatures must use RSA SHA-256',
+              :require_sha256
+          )
+        end
+
         unless cert.public_key.verify(signature_algorithm.new, signature, canon_string)
-          return soft ? false : (raise ValidationError.new("Key validation error"))
+          return soft ? false : (raise ValidationError.new(
+            'Key validation error',
+            :key_validation_error
+          ))
         end
 
         return true