Finish fixing tests. Yay!

Author: johnnyshields

lib/ruby_saml/response.rb

@@ -819,23 +819,26 @@ def doc_to_validate
     #
     def validate_signature
       error_msg = "Invalid Signature on SAML Response"
-
       doc = doc_to_validate
-      subject_id = RubySaml::XML::SignedDocumentValidator.subject_id(document)
-      return false unless subject_id
 
-      sig_elements = document.xpath(
-        "/p:Response[@ID=$id]/ds:Signature",
-        { "p" => RubySaml::XML::NS_PROTOCOL, "ds" => RubySaml::XML::DSIG },
-        id: subject_id
-      )
+      # TODO: document vs doc is super confusing
+      subject_id = RubySaml::XML::SignedDocumentValidator.subject_id(document)
+      sig_elements = []
+      if subject_id
+        sig_elements = document.xpath(
+          "/p:Response[@ID=$id]/ds:Signature",
+          { "p" => RubySaml::XML::NS_PROTOCOL, "ds" => RubySaml::XML::DSIG },
+          id: subject_id
+        )
+      end
 
       # Check signature node inside assertion
       if sig_elements.empty?
+        subject_id2 = RubySaml::XML::SignedDocumentValidator.subject_id(doc)
         sig_elements = doc.xpath(
           "/p:Response/a:Assertion[@ID=$id]/ds:Signature",
           SAML_NAMESPACES,
-          id: subject_id
+          id: subject_id2
         )
       end
 

lib/ruby_saml/xml.rb

@@ -91,15 +91,11 @@ def safe_load_nokogiri(document, check_malformed_doc: true)
       xml
     end
 
-    # def clone_node(node)
-    #   if node.is_a?(Nokogiri::XML::Node)
-    #     node.to_xml(save_with: Nokogiri::XML::Node::SaveOptions::AS_XML)
-    #   end
-    #
-    #   Nokogiri::XML(doc_str) do |config|
-    #     config.options = NOKOGIRI_OPTIONS
-    #   end
-    # end
+    def copy_nokogiri(noko)
+      Nokogiri::XML(noko.to_xml(save_with: Nokogiri::XML::Node::SaveOptions::AS_XML)) do |config|
+        config.options = NOKOGIRI_OPTIONS
+      end
+    end
 
     # Lookup XML canonicalization algorithm.
     # @api private

lib/ruby_saml/xml/signed_document_info.rb

@@ -13,7 +13,11 @@ class SignedDocumentInfo
       # @param noko [Nokogiri::XML] The XML document to validate
       # @param check_malformed_doc [Boolean] Whether to check for malformed documents
       def initialize(noko, check_malformed_doc: true)
-        noko = RubySaml::XML.safe_load_nokogiri(noko, check_malformed_doc: check_malformed_doc) unless noko.is_a?(Nokogiri::XML::Document)
+        noko = if noko.is_a?(Nokogiri::XML::Document)
+                 RubySaml::XML.copy_nokogiri(noko)
+               else
+                 RubySaml::XML.safe_load_nokogiri(noko, check_malformed_doc: check_malformed_doc)
+               end
         @noko = noko
         @check_malformed_doc = check_malformed_doc
       end
@@ -52,16 +56,8 @@ def validate_signature(cert)
 
         # Compare digest
         calculated_digest = digest_algorithm.digest(canonicalized_subject)
-        # puts "calculated_digest: #{calculated_digest.bytes}"
-        # puts "digest_value: #{digest_value.bytes}"
-        # puts "subject" + canonicalized_subject.inspect
-        # puts "\n\n\n\n\n\n"
         raise RubySaml::ValidationError.new('Digest mismatch') unless calculated_digest == digest_value
 
-        # puts "signature_hash_algorithm: #{signature_hash_algorithm}"
-        # puts "signature_value: #{signature_value.bytes}"
-        # puts "canonicalized_signed_info: #{canonicalized_signed_info.inspect}"
-
         # Verify signature
         signature_verified = false
         begin
@@ -113,10 +109,8 @@ def reference_node
       # Get the ID of the signed element
       # @return [String] The ID of the signed element
       def subject_id
-        id = uri_from_reference_node || signature_node.parent&.[]('ID')
-        return id unless !id || id.empty?
-
-        raise RubySaml::ValidationError.new('No signed subject ID found')
+        # TODO: The error here is problematic, perhaps it can be checked elsewhere
+        @subject_id ||= extract_subject_id || (raise RubySaml::ValidationError.new('No signed subject ID found'))
       end
 
       # Get the subject node (the node being signed)
@@ -136,8 +130,10 @@ def canonicalized_subject
       # TODO: Destructive side-effect!! signature_node.remove
       # should possibly deep copy the noko object initially
       def remove_signature_node!
-        inclusive_namespaces # memoize this
-        canonicalized_signed_info # memoize this
+        # memoize various elements
+        subject_id
+        inclusive_namespaces
+        canonicalized_signed_info
 
         signature_node.remove
       end
@@ -205,6 +201,12 @@ def inclusive_namespaces
 
       private
 
+      def extract_subject_id
+        return unless reference_node
+
+        reference_node['URI'][1..] || signature_node.parent['ID']
+      end
+
       # Get the ds:Signature element from the document
       # @return [Nokogiri::XML::Element] The Signature element
       def signature_node
@@ -236,11 +238,6 @@ def canon_algorithm_from_transforms
         transform_element = transforms.reverse.detect { |el| el['Algorithm'] }
         RubySaml::XML.canon_algorithm(transform_element, default: false)
       end
-
-      def uri_from_reference_node
-        uri = reference_node&.[]('URI')&.delete_prefix('#')
-        uri unless !uri || uri.empty?
-      end
     end
   end
 end

lib/ruby_saml/xml/signed_document_validator.rb

@@ -40,21 +40,10 @@ def validate_signature(document, base64_cert, errors = [], soft: true)
       end
 
       # TODO: This is a workaround to avoid errors
-      def subject_id(noko)
-        # TODO: Should be this
-        # SignedDocumentInfo.new(document).subject_id
-        noko = RubySaml::XML.safe_load_nokogiri(noko) unless noko.is_a?(Nokogiri::XML::Document)
-        reference_element = noko.at_xpath(
-          '//ds:Signature/ds:SignedInfo/ds:Reference',
-          { 'ds' => RubySaml::XML::DSIG }
-        )
-
-        return nil if reference_element.nil?
-
-        sei = reference_element['URI'].delete_prefix('#')
-        return sei unless !sei || sei.empty?
-
-        reference_element.parent.parent.parent['ID']
+      def subject_id(document)
+        SignedDocumentInfo.new(document).subject_id
+      rescue RubySaml::ValidationError
+        # TODO: Consider removing the error in SignedDocumentInfo#subject_id
       end
 
       def subject_node(document)

test/response_test.rb

@@ -376,12 +376,11 @@ def generate_audience_error(expected, actual)
         end
 
         it "support dynamic namespace resolution on signature elements" do
-          no_signature_response = RubySaml::Response.new(fixture("no_signature_ns.xml"))
+          no_signature_response = RubySaml::Response.new(fixture("inclusive_namespaces.xml"))
           no_signature_response.stubs(:conditions).returns(nil)
           no_signature_response.stubs(:validate_subject_confirmation).returns(true)
           no_signature_response.settings = settings
-          no_signature_response.settings.idp_cert_fingerprint = "3D:C5:BC:58:60:5D:19:64:94:E3:BA:C8:3D:49:01:D5:56:34:44:65:C2:85:0A:A8:65:A5:AC:76:7E:65:1F:F7"
-          RubySaml::XML::SignedDocumentValidator.expects(:validate_signature).returns(true)
+          no_signature_response.settings.idp_cert_fingerprint = "E0:89:CF:86:E3:00:C0:C8:B9:BC:04:16:D7:F3:8D:8D:9C:8F:20:B3:FE:7C:EC:64:D5:5D:90:E3:7B:8B:5A:51"
           assert no_signature_response.is_valid?
         end
 

test/xml_test.rb

@@ -391,11 +391,12 @@ class XmlTest < Minitest::Test
 
       describe 'signature wrapping attack - concealed SAML response body' do
         let(:document_data) { read_invalid_response("response_with_concealed_signed_assertion.xml") }
+        let(:document) { RubySaml::Response.new(document_data) }
         let(:fingerprint) { '6385109dd146a45d4382799491cb2707bd1ebda3738f27a0e4a4a8159c0fe6cd' }
 
-        it 'is valid, but fails to retrieve information' do
-          assert RubySaml::XML::SignedDocumentValidator.validate_document(document, fingerprint), 'Document should be valid'
-          assert response.name_id.nil?, 'Document should expose only signed, valid details'
+        it 'is valid, but the unsigned information is ignored in favour of the signed information' do
+          assert RubySaml::XML::SignedDocumentValidator.validate_document(document.document, fingerprint), 'Document should be valid'
+          assert_equal 'someone@example.org', document.name_id, 'Document should expose only signed, valid details'
         end
       end
     end