Merge pull request #255 from onelogin/ds_namespace_at_root

Refactor validate signature.
SAML-Toolkits · Oct 8, 2015 · 9a92281 · 9a92281
2 parents 096de59 + 275512a
commit 9a92281
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 37 deletions.
diff --git a/lib/xml_security.rb b/lib/xml_security.rb
@@ -227,42 +227,48 @@ def validate_document(idp_cert_fingerprint, soft = true, options = {})
     end
 
     def validate_signature(base64_cert, soft = true)
-      # validate references
-
-      # check for inclusive namespaces
-      inclusive_namespaces = extract_inclusive_namespaces
 
       document = Nokogiri.parse(self.to_s) do |options|
         options = XMLSecurity::BaseDocument::NOKOGIRI_OPTIONS
       end
 
-      # create a working copy so we don't modify the original
+      # create a rexml document
       @working_copy ||= REXML::Document.new(self.to_s).root
 
-      # store and remove signature node
-      @sig_element ||= begin
-        element = REXML::XPath.first(
+      # get signature node
+      sig_element = REXML::XPath.first(
           @working_copy,
           "//ds:Signature",
           {"ds"=>DSIG}
-        )
-        element.remove
-      end
+      )
 
-      # verify signature
-      signed_info_element = REXML::XPath.first(
-        @sig_element,
-        "//ds:SignedInfo",
+      # signature method
+      sig_alg_value = REXML::XPath.first(
+        sig_element,
+        "./ds:SignedInfo/ds:SignatureMethod",
         {"ds"=>DSIG}
       )
-      noko_sig_element = document.at_xpath('//ds:Signature', 'ds' => DSIG)
-      noko_signed_info_element = noko_sig_element.at_xpath('./ds:SignedInfo', 'ds' => DSIG)
+      signature_algorithm = algorithm(sig_alg_value)
+
+      # get signature
+      base64_signature = REXML::XPath.first(
+        sig_element,
+        "./ds:SignatureValue",
+        {"ds" => DSIG}
+      ).text
+      signature = Base64.decode64(base64_signature)
+
+      # canonicalization method
       canon_algorithm = canon_algorithm REXML::XPath.first(
-        @sig_element,
-        '//ds:CanonicalizationMethod',
+        sig_element,
+        './ds:SignedInfo/ds:CanonicalizationMethod',
         'ds' => DSIG
       )
 
+      noko_sig_element = document.at_xpath('//ds:Signature', 'ds' => DSIG)
+      noko_signed_info_element = noko_sig_element.at_xpath('./ds:SignedInfo', 'ds' => DSIG)
+
+      # Handle when no URI
       noko_signed_info_reference_element_uri_attr = noko_signed_info_element.at_xpath('./ds:Reference', 'ds' => DSIG).attributes["URI"]
       if (noko_signed_info_reference_element_uri_attr.value.empty?)
         noko_signed_info_reference_element_uri_attr.value = "##{document.root.attribute('ID')}"
@@ -271,8 +277,11 @@ def validate_signature(base64_cert, soft = true)
       canon_string = noko_signed_info_element.canonicalize(canon_algorithm)
       noko_sig_element.remove
 
+      # get inclusive namespaces
+      inclusive_namespaces = extract_inclusive_namespaces
+
       # check digests
-      REXML::XPath.each(@sig_element, "//ds:Reference", {"ds"=>DSIG}) do |ref|
+      REXML::XPath.each(sig_element, "//ds:Reference", {"ds"=>DSIG}) do |ref|
         uri = ref.attributes.get_attribute("URI").value
 
         hashed_element = uri.empty? ? document : document.at_xpath("//*[@ID=$uri]", nil, { 'uri' => uri[1..-1] })
@@ -303,26 +312,11 @@ def validate_signature(base64_cert, soft = true)
         end
       end
 
-      base64_signature = REXML::XPath.first(
-        @sig_element,
-        "//ds:SignatureValue",
-        {"ds" => DSIG}
-      ).text
-
-      signature = Base64.decode64(base64_signature)
-
       # get certificate object
       cert_text = Base64.decode64(base64_cert)
       cert = OpenSSL::X509::Certificate.new(cert_text)
 
-      # signature method
-      sig_alg_value = REXML::XPath.first(
-        signed_info_element,
-        "//ds:SignatureMethod",
-        {"ds"=>DSIG}
-      )
-      signature_algorithm = algorithm(sig_alg_value)
-
+      # verify signature
       unless cert.public_key.verify(signature_algorithm.new, signature, canon_string)
         @errors << "Key validation error"
         return soft ? false : (raise OneLogin::RubySaml::ValidationError.new("Key validation error"))
@@ -347,7 +341,7 @@ def extract_signed_element_id
       return nil if reference_element.nil?
 
       sei = reference_element.attribute("URI").value[1..-1] 
-      self.signed_element_id = sei.nil? ? self.root.attribute("ID") : sei
+      sei.nil? ? self.root.attribute("ID") : sei
     end
 
     def extract_inclusive_namespaces

diff --git a/test/response_test.rb b/test/response_test.rb
@@ -11,6 +11,7 @@ class RubySamlTest < Minitest::Test
     let(:response_without_attributes) { OneLogin::RubySaml::Response.new(response_document_without_attributes) }
     let(:response_without_reference_uri) { OneLogin::RubySaml::Response.new(response_document_without_reference_uri) }
     let(:response_with_signed_assertion) { OneLogin::RubySaml::Response.new(response_document_with_signed_assertion) }
+    let(:response_with_ds_namespace_at_the_root) { OneLogin::RubySaml::Response.new(response_document_with_ds_namespace_at_the_root)}
     let(:response_unsigned) { OneLogin::RubySaml::Response.new(response_document_unsigned) }
     let(:response_wrapped) { OneLogin::RubySaml::Response.new(response_document_wrapped) }
     let(:response_multiple_attr_values) { OneLogin::RubySaml::Response.new(fixture(:response_with_multiple_attribute_values)) }
@@ -673,6 +674,13 @@ class RubySamlTest < Minitest::Test
         assert_empty response_valid_signed.errors
       end
 
+      it "return true when the signature is valid and ds namespace is at the root" do
+        settings.idp_cert_fingerprint = '5614657ab692b960480389723a36446a5fe1f7ec'
+        response_with_ds_namespace_at_the_root.settings = settings
+        assert response_with_ds_namespace_at_the_root.send(:validate_signature)
+        assert_empty response_with_ds_namespace_at_the_root.errors
+      end
+
       it "return false when no fingerprint" do
         settings.idp_cert_fingerprint = nil
         settings.idp_cert = nil

diff --git a/test/responses/response_with_ds_namespace_at_the_root.xml.base64 b/test/responses/response_with_ds_namespace_at_the_root.xml.base64
@@ -0,0 +1 @@
+PHNhbWxwOlJlc3BvbnNlIHhtbG5zOnNhbWxwPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6cHJvdG9jb2wiDQp4bWxuczpkcz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC8wOS94bWxkc2lnIyIgeG1sbnM6c2FtbD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOmFzc2VydGlvbiIgSUQ9Il84ZThkYzVmNjlhOThjYzRjMWZmMzQyN2U1Y2UzNDYwNmZkNjcyZjkxZTYiIFZlcnNpb249IjIuMCIgSXNzdWVJbnN0YW50PSIyMDE0LTA3LTE3VDAxOjAxOjQ4WiIgRGVzdGluYXRpb249Imh0dHA6Ly9zcC5leGFtcGxlLmNvbS9kZW1vMS9pbmRleC5waHA/YWNzIiBJblJlc3BvbnNlVG89Ik9ORUxPR0lOXzRmZWUzYjA0NjM5NWM0ZTc1MTAxMWU5N2Y4OTAwYjUyNzNkNTY2ODUiPg0KICA8c2FtbDpJc3N1ZXI+aHR0cDovL2lkcC5leGFtcGxlLmNvbS9tZXRhZGF0YS5waHA8L3NhbWw6SXNzdWVyPg0KICA8c2FtbHA6U3RhdHVzPg0KICAgIDxzYW1scDpTdGF0dXNDb2RlIFZhbHVlPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6c3RhdHVzOlN1Y2Nlc3MiLz4NCiAgPC9zYW1scDpTdGF0dXM+DQogIDxzYW1sOkFzc2VydGlvbiB4bWxuczp4c2k9Imh0dHA6Ly93d3cudzMub3JnLzIwMDEvWE1MU2NoZW1hLWluc3RhbmNlIiB4bWxuczp4cz0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEiIElEPSJwZng2YzViMTVjYy02YzY4LTljMWYtY2QzNy05ZTFmYTFkMmU3YTAiIFZlcnNpb249IjIuMCIgSXNzdWVJbnN0YW50PSIyMDE0LTA3LTE3VDAxOjAxOjQ4WiI+DQogICAgPHNhbWw6SXNzdWVyPmh0dHA6Ly9pZHAuZXhhbXBsZS5jb20vbWV0YWRhdGEucGhwPC9zYW1sOklzc3Vlcj48ZHM6U2lnbmF0dXJlPg0KICA8ZHM6U2lnbmVkSW5mbz48ZHM6Q2Fub25pY2FsaXphdGlvbk1ldGhvZCBBbGdvcml0aG09Imh0dHA6Ly93d3cudzMub3JnLzIwMDEvMTAveG1sLWV4Yy1jMTRuIyIvPg0KICAgIDxkczpTaWduYXR1cmVNZXRob2QgQWxnb3JpdGhtPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwLzA5L3htbGRzaWcjcnNhLXNoYTEiLz4NCiAgPGRzOlJlZmVyZW5jZSBVUkk9IiNwZng2YzViMTVjYy02YzY4LTljMWYtY2QzNy05ZTFmYTFkMmU3YTAiPjxkczpUcmFuc2Zvcm1zPjxkczpUcmFuc2Zvcm0gQWxnb3JpdGhtPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwLzA5L3htbGRzaWcjZW52ZWxvcGVkLXNpZ25hdHVyZSIvPjxkczpUcmFuc2Zvcm0gQWxnb3JpdGhtPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxLzEwL3htbC1leGMtYzE0biMiLz48L2RzOlRyYW5zZm9ybXM+PGRzOkRpZ2VzdE1ldGhvZCBBbGdvcml0aG09Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvMDkveG1sZHNpZyNzaGExIi8+PGRzOkRpZ2VzdFZhbHVlPmlHdENJcGpyZ09Hc2FDY0lFOEtwSWEzVzZnVT08L2RzOkRpZ2VzdFZhbHVlPjwvZHM6UmVmZXJlbmNlPjwvZHM6U2lnbmVkSW5mbz48ZHM6U2lnbmF0dXJlVmFsdWU+T0RIU2tzUkl1ZEc3b2s0KzRUZitPRnA0UFJ4Ukc3d3BERGdyVDZwQk5ZbUFKNUN2dFk5UlczRldHYlJtZ2NoSjdhYW9sQ3ViekQ5Qkk4Y01nb3V6ZGErdjcrZ1k4THloVUNmV2RSd0gvSzZlVmYvZUtldjk3aUQ1YVA1TTU1MCtCZ0NVaHVWNFg0MlpUbFhkdjBVKzRDQ1VYWnFuLzRjczBxNXhkeTBoM0JzPTwvZHM6U2lnbmF0dXJlVmFsdWU+DQo8ZHM6S2V5SW5mbz48ZHM6WDUwOURhdGE+PGRzOlg1MDlDZXJ0aWZpY2F0ZT5NSUlDYWpDQ0FkT2dBd0lCQWdJQkFEQU5CZ2txaGtpRzl3MEJBUTBGQURCU01Rc3dDUVlEVlFRR0V3SjFjekVUTUJFR0ExVUVDQXdLUTJGc2FXWnZjbTVwWVRFVk1CTUdBMVVFQ2d3TVQyNWxiRzluYVc0Z1NXNWpNUmN3RlFZRFZRUUREQTV6Y0M1bGVHRnRjR3hsTG1OdmJUQWVGdzB4TkRBM01UY3hOREV5TlRaYUZ3MHhOVEEzTVRjeE5ERXlOVFphTUZJeEN6QUpCZ05WQkFZVEFuVnpNUk13RVFZRFZRUUlEQXBEWVd4cFptOXlibWxoTVJVd0V3WURWUVFLREF4UGJtVnNiMmRwYmlCSmJtTXhGekFWQmdOVkJBTU1Ebk53TG1WNFlXMXdiR1V1WTI5dE1JR2ZNQTBHQ1NxR1NJYjNEUUVCQVFVQUE0R05BRENCaVFLQmdRRFp4K09ONElVb0lXeGd1a1RiMXRPaVgzYk1ZellRaXdXUFVOTXArRnE4MnhvTm9nc28yYnlrWkcweWlKbTVvOHp2L3NkNnBHb3VheU1na3gvMkZTT2RjMzZUMGpHYkNIdVJTYnRpYTBQRXpOSVJ0bVZpTXJ0M0Flb1dCaWRSWG1ac3hDTkx3Z0lWNmRuMldwdUU1QXowYkhncFpuUXhUS0ZlazBCTUtVL2Q4d0lEQVFBQm8xQXdUakFkQmdOVkhRNEVGZ1FVR0h4WXFaWXlYN2NUeEtWT0RWZ1p3U1RkQ253d0h3WURWUjBqQkJnd0ZvQVVHSHhZcVpZeVg3Y1R4S1ZPRFZnWndTVGRDbnd3REFZRFZSMFRCQVV3QXdFQi96QU5CZ2txaGtpRzl3MEJBUTBGQUFPQmdRQnlGT2wraE1GSUNiZDNESmZucDJSZ2QvZHF0dHNaRy90eWhJTFd2RXJiaW8vREVlOThtWHBvd2hUa0MwNEVOcHJPeVhpN1piVXFpaWNGODl1QUd5dDFvcWdUVUNEMVZzTGFocUljbXJ6Z3VtTnlUd0xHV28xN1dEQWExL3VzRGhldFdBTWhnekYvQ25mNWVrMG5LMDBtMFlaR3ljNEx6Z0QwQ1JPTUFTVFdOZz09PC9kczpYNTA5Q2VydGlmaWNhdGU+PC9kczpYNTA5RGF0YT48L2RzOktleUluZm8+PC9kczpTaWduYXR1cmU+DQogICAgPHNhbWw6U3ViamVjdD4NCiAgICAgIDxzYW1sOk5hbWVJRCBTUE5hbWVRdWFsaWZpZXI9Imh0dHA6Ly9zcC5leGFtcGxlLmNvbS9kZW1vMS9tZXRhZGF0YS5waHAiIEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6dHJhbnNpZW50Ij5fY2UzZDI5NDhiNGNmMjAxNDZkZWUwYTBiM2RkNmY2OWI2Y2Y4NmY2MmQ3PC9zYW1sOk5hbWVJRD4NCiAgICAgIDxzYW1sOlN1YmplY3RDb25maXJtYXRpb24gTWV0aG9kPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6Y206YmVhcmVyIj4NCiAgICAgICAgPHNhbWw6U3ViamVjdENvbmZpcm1hdGlvbkRhdGEgTm90T25PckFmdGVyPSIyMDI0LTAxLTE4VDA2OjIxOjQ4WiIgUmVjaXBpZW50PSJodHRwOi8vc3AuZXhhbXBsZS5jb20vZGVtbzEvaW5kZXgucGhwP2FjcyIgSW5SZXNwb25zZVRvPSJPTkVMT0dJTl80ZmVlM2IwNDYzOTVjNGU3NTEwMTFlOTdmODkwMGI1MjczZDU2Njg1Ii8+DQogICAgICA8L3NhbWw6U3ViamVjdENvbmZpcm1hdGlvbj4NCiAgICA8L3NhbWw6U3ViamVjdD4NCiAgICA8c2FtbDpDb25kaXRpb25zIE5vdEJlZm9yZT0iMjAxNC0wNy0xN1QwMTowMToxOFoiIE5vdE9uT3JBZnRlcj0iMjAyNC0wMS0xOFQwNjoyMTo0OFoiPg0KICAgICAgPHNhbWw6QXVkaWVuY2VSZXN0cmljdGlvbj4NCiAgICAgICAgPHNhbWw6QXVkaWVuY2U+aHR0cDovL3NwLmV4YW1wbGUuY29tL2RlbW8xL21ldGFkYXRhLnBocDwvc2FtbDpBdWRpZW5jZT4NCiAgICAgIDwvc2FtbDpBdWRpZW5jZVJlc3RyaWN0aW9uPg0KICAgIDwvc2FtbDpDb25kaXRpb25zPg0KICAgIDxzYW1sOkF1dGhuU3RhdGVtZW50IEF1dGhuSW5zdGFudD0iMjAxNC0wNy0xN1QwMTowMTo0OFoiIFNlc3Npb25Ob3RPbk9yQWZ0ZXI9IjIwMjQtMDctMTdUMDk6MDE6NDhaIiBTZXNzaW9uSW5kZXg9Il9iZTk5NjdhYmQ5MDRkZGNhZTNjMGViNDE4OWFkYmUzZjcxZTMyN2NmOTMiPg0KICAgICAgPHNhbWw6QXV0aG5Db250ZXh0Pg0KICAgICAgICA8c2FtbDpBdXRobkNvbnRleHRDbGFzc1JlZj51cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6YWM6Y2xhc3NlczpQYXNzd29yZDwvc2FtbDpBdXRobkNvbnRleHRDbGFzc1JlZj4NCiAgICAgIDwvc2FtbDpBdXRobkNvbnRleHQ+DQogICAgPC9zYW1sOkF1dGhuU3RhdGVtZW50Pg0KICAgIDxzYW1sOkF0dHJpYnV0ZVN0YXRlbWVudD4NCiAgICAgIDxzYW1sOkF0dHJpYnV0ZSBOYW1lPSJ1aWQiIE5hbWVGb3JtYXQ9InVybjpvYXNpczpuYW1lczp0YzpTQU1MOjIuMDphdHRybmFtZS1mb3JtYXQ6YmFzaWMiPg0KICAgICAgICA8c2FtbDpBdHRyaWJ1dGVWYWx1ZSB4c2k6dHlwZT0ieHM6c3RyaW5nIj50ZXN0PC9zYW1sOkF0dHJpYnV0ZVZhbHVlPg0KICAgICAgPC9zYW1sOkF0dHJpYnV0ZT4NCiAgICAgIDxzYW1sOkF0dHJpYnV0ZSBOYW1lPSJtYWlsIiBOYW1lRm9ybWF0PSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6YXR0cm5hbWUtZm9ybWF0OmJhc2ljIj4NCiAgICAgICAgPHNhbWw6QXR0cmlidXRlVmFsdWUgeHNpOnR5cGU9InhzOnN0cmluZyI+dGVzdEBleGFtcGxlLmNvbTwvc2FtbDpBdHRyaWJ1dGVWYWx1ZT4NCiAgICAgIDwvc2FtbDpBdHRyaWJ1dGU+DQogICAgICA8c2FtbDpBdHRyaWJ1dGUgTmFtZT0iZWR1UGVyc29uQWZmaWxpYXRpb24iIE5hbWVGb3JtYXQ9InVybjpvYXNpczpuYW1lczp0YzpTQU1MOjIuMDphdHRybmFtZS1mb3JtYXQ6YmFzaWMiPg0KICAgICAgICA8c2FtbDpBdHRyaWJ1dGVWYWx1ZSB4c2k6dHlwZT0ieHM6c3RyaW5nIj51c2Vyczwvc2FtbDpBdHRyaWJ1dGVWYWx1ZT4NCiAgICAgICAgPHNhbWw6QXR0cmlidXRlVmFsdWUgeHNpOnR5cGU9InhzOnN0cmluZyI+ZXhhbXBsZXJvbGUxPC9zYW1sOkF0dHJpYnV0ZVZhbHVlPg0KICAgICAgPC9zYW1sOkF0dHJpYnV0ZT4NCiAgICA8L3NhbWw6QXR0cmlidXRlU3RhdGVtZW50Pg0KICA8L3NhbWw6QXNzZXJ0aW9uPg0KPC9zYW1scDpSZXNwb25zZT4=
diff --git a/test/test_helper.rb b/test/test_helper.rb
@@ -79,6 +79,10 @@ def response_document_with_signed_assertion_2
     @response_document_with_signed_assertion_2 ||= read_response("response_with_signed_assertion_2.xml.base64")
   end
 
+  def response_document_with_ds_namespace_at_the_root
+    @response_document_with_ds_namespace_at_the_root ||= read_response("response_with_ds_namespace_at_the_root.xml.base64")
+  end
+
   def response_document_unsigned
     @response_document_unsigned ||= read_response("response_unsigned_xml_base64")
   end