Merge pull request #112 from 18F/dmm/fix-namespace-bug-for-all-requests

Include namespace bug fix for all validations

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    saml_idp (0.21.7.pre.18f)
+    saml_idp (0.21.8.pre.18f)
       activesupport
       builder
       faraday

lib/saml_idp/request.rb

@@ -171,7 +171,7 @@ def matching_cert
       Array(service_provider.certs).find do |cert|
         document.valid_signature?(
           fingerprint(cert),
-          options.merge(cert:, digest_method_fix_enabled: true)
+          options.merge(cert:)
         )
       end
     end
@@ -189,7 +189,7 @@ def cert_errors
       Array(service_provider.certs).map do |cert|
         document.gather_errors(
           fingerprint(cert),
-          options.merge(cert:, digest_method_fix_enabled: true)
+          options.merge(cert:)
         )
       end
     end

lib/saml_idp/version.rb

@@ -1,3 +1,3 @@
 module SamlIdp
-  VERSION = '0.21.7-18f'.freeze
+  VERSION = '0.21.8-18f'.freeze
 end

lib/saml_idp/xml_security.rb

@@ -83,8 +83,7 @@ def validate_doc(base64_cert, soft = true, options = {})
         else
           validate_doc_embedded_signature(
             base64_cert,
-            soft,
-            digest_method_fix_enabled: options[:digest_method_fix_enabled]
+            soft
           )
         end
       end
@@ -172,8 +171,7 @@ def validate_doc_params_signature(base64_cert, soft = true, params)
 
       def validate_doc_embedded_signature(
         base64_cert,
-        soft = true,
-        digest_method_fix_enabled: false
+        soft = true
       )
         # check for inclusive namespaces
         inclusive_namespaces = extract_inclusive_namespaces
@@ -204,10 +202,7 @@ def validate_doc_embedded_signature(
             inclusive_namespaces
           )
 
-          digest_algorithm = digest_method_algorithm(
-            ref,
-            digest_method_fix_enabled
-          )
+          digest_algorithm = digest_method_algorithm(ref)
 
           hash = digest_algorithm.digest(canon_hashed_element)
 
@@ -233,13 +228,8 @@ def validate_doc_embedded_signature(
         verify_signature(base64_cert, sig_alg, signature, canon_string, soft)
       end
 
-      def digest_method_algorithm(ref, digest_method_fix_enabled)
-        digest_method = ref.at_xpath('//ds:DigestMethod | //DigestMethod', DS_NS)
-        if digest_method_fix_enabled || digest_method.namespace&.prefix.present?
-          algorithm(digest_method)
-        else
-          algorithm(nil)
-        end
+      def digest_method_algorithm(ref)
+        algorithm(ref.at_xpath('//ds:DigestMethod | //DigestMethod', DS_NS))
       end
 
       def verify_signature(base64_cert, sig_alg, signature, canon_string, soft)

spec/xml_security_spec.rb

@@ -63,31 +63,6 @@ module SamlIdp
           end
         end
       end
-
-      describe 'options[:digest_method_fix_enabled]' do
-        let(:xml_string) do
-          SamlIdp::Request.from_deflated_request(
-            signed_auth_request
-          ).raw_xml
-        end
-
-        let(:digest_method_fix_enabled) { true }
-        let(:options) { { digest_method_fix_enabled: } }
-
-        context 'digest_method_fix_enabled is set to true' do
-          it 'validates the doc successfully' do
-            expect(subject.validate_doc(base64cert, true, options)).to be true
-          end
-        end
-
-        context 'digest_method_fix_enabled is set to false' do
-          let(:digest_method_fix_enabled) { false }
-
-          it 'validates the doc successfully' do
-            expect(subject.validate_doc(base64cert, true, options)).to be true
-          end
-        end
-      end
     end
 
     describe '#validate' do
@@ -147,114 +122,25 @@ module SamlIdp
           sig_element.at_xpath('//ds:Reference | //Reference', ds_namespace)
         end
 
-        context 'digest_method_fix_enabled is true' do
-          let(:digest_method_fix_enabled) { true }
-
-          context 'document does not have ds namespace for Signature elements' do
-            it 'returns the value in the DigestMethod node' do
-              expect(subject.send(
-                       :digest_method_algorithm,
-                       ref,
-                       digest_method_fix_enabled
-                     )).to eq OpenSSL::Digest::SHA256
-            end
-
-            describe 'when the DigestMethod node does not exist' do
-              before do
-                ref.at_xpath('//ds:DigestMethod | //DigestMethod', ds_namespace).remove
-              end
-
-              it 'returns the default algorithm type' do
-                expect(subject.send(
-                         :digest_method_algorithm,
-                         ref,
-                         digest_method_fix_enabled
-                       )).to eq OpenSSL::Digest::SHA1
-              end
-            end
-          end
 
-          context 'document does have ds namespace for Signature elements' do
-            let(:xml_string) do
-              SamlIdp::Request.from_deflated_request(
-                signed_auth_request
-              ).raw_xml
-            end
-
-            it 'returns the value in the DigestMethod node' do
-              expect(subject.send(
-                       :digest_method_algorithm,
-                       ref,
-                       digest_method_fix_enabled
-                     )).to eq OpenSSL::Digest::SHA256
-            end
-
-            describe 'when the DigestMethod node does not exist' do
-              before do
-                ref.at_xpath('//ds:DigestMethod | //DigestMethod', ds_namespace).remove
-              end
-
-              it 'returns the default algorithm type' do
-                expect(subject.send(
-                         :digest_method_algorithm,
-                         ref,
-                         digest_method_fix_enabled
-                       )).to eq OpenSSL::Digest::SHA1
-              end
-            end
+        context 'when document does not have ds namespace for Signature elements' do
+          let(:xml_string) { fixture('valid_no_ns.xml', path: 'requests') }
+
+
+          it 'returns the value in the DigestMethod node' do
+            expect(subject.send(:digest_method_algorithm, ref)).to eq OpenSSL::Digest::SHA256
           end
         end
 
-        context 'digest_method_fix_enabled is false' do
-          let(:digest_method_fix_enabled) { false }
-
-          context 'document does not have ds namespace for Signature elements' do
-            let(:xml_string) { fixture('valid_no_ns.xml', path: 'requests') }
-
-            it 'returns the default algorithm type' do
-              expect(subject.send(
-                       :digest_method_algorithm,
-                       ref,
-                       digest_method_fix_enabled
-                     )).to eq OpenSSL::Digest::SHA1
-            end
-
-            describe 'when the namespace hash is not defined' do
-              it 'returns the default algorithm type' do
-                expect(subject.send(
-                         :digest_method_algorithm,
-                         ref,
-                         digest_method_fix_enabled
-                       )).to eq OpenSSL::Digest::SHA1
-              end
-            end
+        context 'document does have ds namespace for Signature elements' do
+          let(:xml_string) do
+            SamlIdp::Request.from_deflated_request(
+              signed_auth_request
+            ).raw_xml
           end
 
-          context 'document does have ds namespace for Signature elements' do
-            let(:xml_string) do
-              SamlIdp::Request.from_deflated_request(
-                signed_auth_request
-              ).raw_xml
-            end
-
-            it 'returns the value in the DigestMethod node' do
-              expect(subject.send(
-                       :digest_method_algorithm,
-                       ref,
-                       digest_method_fix_enabled
-                     )).to eq OpenSSL::Digest::SHA256
-            end
-
-            describe 'when the namespace hash is not defined' do
-              it 'returns the value in the DigestMethod node' do
-                # in this scenario, the undefined namespace hash is ignored
-                expect(subject.send(
-                         :digest_method_algorithm,
-                         ref,
-                         digest_method_fix_enabled
-                       )).to eq OpenSSL::Digest::SHA256
-              end
-            end
+          it 'returns the value in the DigestMethod node' do
+            expect(subject.send(:digest_method_algorithm, ref)).to eq OpenSSL::Digest::SHA256
           end
         end
       end