Merge pull request #605 from johnnyshields/fix-clock-drift

:allowed_clock_drift should be bidrectional

Author: pitbulk

lib/onelogin/ruby-saml/response.rb

@@ -337,9 +337,9 @@ def audiences
       end
 
       # returns the allowed clock drift on timing validation
-      # @return [Integer]
+      # @return [Float]
       def allowed_clock_drift
-        return options[:allowed_clock_drift].to_f
+        options[:allowed_clock_drift].to_f.abs + Float::EPSILON
       end
 
       # Checks if the SAML Response contains or not an EncryptedAssertion element
@@ -692,13 +692,13 @@ def validate_conditions
 
         now = Time.now.utc
 
-        if not_before && (now_with_drift = now + allowed_clock_drift) < not_before
-          error_msg = "Current time is earlier than NotBefore condition (#{now_with_drift} < #{not_before})"
+        if not_before && now < (not_before - allowed_clock_drift)
+          error_msg = "Current time is earlier than NotBefore condition (#{now} < #{not_before}#{" - #{allowed_clock_drift.ceil}s" if allowed_clock_drift > 0})"
           return append_error(error_msg)
         end
 
-        if not_on_or_after && now >= (not_on_or_after_with_drift = not_on_or_after + allowed_clock_drift)
-          error_msg = "Current time is on or after NotOnOrAfter condition (#{now} >= #{not_on_or_after_with_drift})"
+        if not_on_or_after && now >= (not_on_or_after + allowed_clock_drift)
+          error_msg = "Current time is on or after NotOnOrAfter condition (#{now} >= #{not_on_or_after}#{" + #{allowed_clock_drift.ceil}s" if allowed_clock_drift > 0})"
           return append_error(error_msg)
         end
 
@@ -740,7 +740,7 @@ def validate_session_expiration(soft = true)
         return true if session_expires_at.nil?
 
         now = Time.now.utc
-        unless (session_expires_at + allowed_clock_drift) > now
+        unless now < (session_expires_at + allowed_clock_drift)
           error_msg = "The attributes have expired, based on the SessionNotOnOrAfter of the AuthnStatement of this Response"
           return append_error(error_msg)
         end
@@ -778,8 +778,8 @@ def validate_subject_confirmation
 
           attrs = confirmation_data_node.attributes
           next if (attrs.include? "InResponseTo" and attrs['InResponseTo'] != in_response_to) ||
-                  (attrs.include? "NotOnOrAfter" and (parse_time(confirmation_data_node, "NotOnOrAfter") + allowed_clock_drift) <= now) ||
-                  (attrs.include? "NotBefore" and parse_time(confirmation_data_node, "NotBefore") > (now + allowed_clock_drift)) ||
+                  (attrs.include? "NotBefore" and now < (parse_time(confirmation_data_node, "NotBefore") - allowed_clock_drift)) ||
+                  (attrs.include? "NotOnOrAfter" and now >= (parse_time(confirmation_data_node, "NotOnOrAfter") + allowed_clock_drift)) ||
                   (attrs.include? "Recipient" and !options[:skip_recipient_check] and settings and attrs['Recipient'] != settings.assertion_consumer_service_url)
 
           valid_subject_confirmation = true

lib/onelogin/ruby-saml/slo_logoutrequest.rb

@@ -130,6 +130,12 @@ def session_indexes
 
       private
 
+      # returns the allowed clock drift on timing validation
+      # @return [Float]
+      def allowed_clock_drift
+        options[:allowed_clock_drift].to_f.abs + Float::EPSILON
+      end
+
       # Hard aux function to validate the Logout Request
       # @param collect_errors [Boolean] Stop validation when first error appears or keep validating. (if soft=true)
       # @return [Boolean] TRUE if the Logout Request is valid
@@ -180,15 +186,17 @@ def validate_version
         true
       end
 
-      # Validates the time. (If the logout request was initialized with the :allowed_clock_drift option, the timing validations are relaxed by the allowed_clock_drift value)
+      # Validates the time. (If the logout request was initialized with the :allowed_clock_drift
+      # option, the timing validations are relaxed by the allowed_clock_drift value)
       # If fails, the error is added to the errors array
       # @return [Boolean] True if satisfies the conditions, otherwise False if soft=True
       # @raise [ValidationError] if soft == false and validation fails
       #
       def validate_not_on_or_after
         now = Time.now.utc
-        if not_on_or_after && now >= (not_on_or_after + (options[:allowed_clock_drift] || 0))
-          return append_error("Current time is on or after NotOnOrAfter (#{now} >= #{not_on_or_after})")
+
+        if not_on_or_after && now >= (not_on_or_after + allowed_clock_drift)
+          return append_error("Current time is on or after NotOnOrAfter (#{now} >= #{not_on_or_after}#{" + #{allowed_clock_drift.ceil}s" if allowed_clock_drift > 0})")
         end
 
         true

test/response_test.rb

@@ -1107,44 +1107,74 @@ def generate_audience_error(expected, actual)
         end
       end
 
-      it "optionally allows for clock drift" do
+      it "optionally allows for clock drift on NotBefore" do
+        settings.soft = true
+
         # The NotBefore condition in the document is 2011-06-14T18:21:01.516Z
         Timecop.freeze(Time.parse("2011-06-14T18:21:01Z")) do
-          settings.soft = true
           special_response_with_saml2_namespace = OneLogin::RubySaml::Response.new(
             response_document_with_saml2_namespace,
             :allowed_clock_drift => 0.515,
             :settings => settings
           )
           assert !special_response_with_saml2_namespace.send(:validate_conditions)
-        end
 
-        Timecop.freeze(Time.parse("2011-06-14T18:21:01Z")) do
           special_response_with_saml2_namespace = OneLogin::RubySaml::Response.new(
             response_document_with_saml2_namespace,
             :allowed_clock_drift => 0.516
           )
           assert special_response_with_saml2_namespace.send(:validate_conditions)
-        end
 
-        Timecop.freeze(Time.parse("2011-06-14T18:21:01Z")) do
-          settings.soft = true
           special_response_with_saml2_namespace = OneLogin::RubySaml::Response.new(
             response_document_with_saml2_namespace,
             :allowed_clock_drift => '0.515',
             :settings => settings
           )
           assert !special_response_with_saml2_namespace.send(:validate_conditions)
-        end
 
-        Timecop.freeze(Time.parse("2011-06-14T18:21:01Z")) do
           special_response_with_saml2_namespace = OneLogin::RubySaml::Response.new(
             response_document_with_saml2_namespace,
             :allowed_clock_drift => '0.516'
           )
           assert special_response_with_saml2_namespace.send(:validate_conditions)
         end
       end
+
+      it "optionally allows for clock drift on NotOnOrAfter" do
+        # Java Floats behave differently than MRI
+        java = defined?(RUBY_ENGINE) && %w[jruby truffleruby].include?(RUBY_ENGINE)
+
+        settings.soft = true
+
+        # The NotBefore condition in the document is 2011-06-1418:31:01.516Z
+        Timecop.freeze(Time.parse("2011-06-14T18:31:02Z")) do
+          special_response_with_saml2_namespace = OneLogin::RubySaml::Response.new(
+              response_document_with_saml2_namespace,
+              :allowed_clock_drift => 0.483,
+              :settings => settings
+          )
+          assert !special_response_with_saml2_namespace.send(:validate_conditions)
+
+          special_response_with_saml2_namespace = OneLogin::RubySaml::Response.new(
+              response_document_with_saml2_namespace,
+              :allowed_clock_drift => java ? 0.485 : 0.484
+          )
+          assert special_response_with_saml2_namespace.send(:validate_conditions)
+
+          special_response_with_saml2_namespace = OneLogin::RubySaml::Response.new(
+              response_document_with_saml2_namespace,
+              :allowed_clock_drift => '0.483',
+              :settings => settings
+          )
+          assert !special_response_with_saml2_namespace.send(:validate_conditions)
+
+          special_response_with_saml2_namespace = OneLogin::RubySaml::Response.new(
+              response_document_with_saml2_namespace,
+              :allowed_clock_drift => java ? '0.485' : '0.484'
+          )
+          assert special_response_with_saml2_namespace.send(:validate_conditions)
+        end
+      end
     end
 
     describe "#attributes" do

test/slo_logoutrequest_test.rb

@@ -109,7 +109,7 @@ class RubySamlTest < Minitest::Test
       end
     end
 
-   describe "#not_on_or_after" do
+    describe "#not_on_or_after" do
       it "extract the value of the NotOnOrAfter attribute" do
         time_value = '2014-07-17T01:01:48Z'
         assert_nil logout_request.not_on_or_after
@@ -158,25 +158,52 @@ class RubySamlTest < Minitest::Test
       it "return true when the logout request has a valid NotOnOrAfter or does not contain any" do
         assert logout_request.send(:validate_not_on_or_after)
         assert_empty logout_request.errors
-        Timecop.freeze Time.parse('2011-06-14T18:25:01.516Z') do
-          time_value = '2014-07-17T01:01:48Z'
-          logout_request.document.root.attributes['NotOnOrAfter'] = time_value
+
+        Timecop.freeze Time.parse('2014-07-17T01:01:47Z') do
+          logout_request.document.root.attributes['NotOnOrAfter'] = '2014-07-17T01:01:48Z'
           assert logout_request.send(:validate_not_on_or_after)
           assert_empty logout_request.errors
         end
       end
 
       it "return false when the logout request has an invalid NotOnOrAfter" do
-        logout_request.document.root.attributes['NotOnOrAfter'] = '2014-07-17T01:01:48Z'
-        assert !logout_request.send(:validate_not_on_or_after)
-        assert /Current time is on or after NotOnOrAfter/.match(logout_request.errors[0])
+        Timecop.freeze Time.parse('2014-07-17T01:01:49Z') do
+          logout_request.document.root.attributes['NotOnOrAfter'] = '2014-07-17T01:01:48Z'
+          assert !logout_request.send(:validate_not_on_or_after)
+          assert /Current time is on or after NotOnOrAfter/.match(logout_request.errors[0])
+        end
       end
 
       it "raise when the logout request has an invalid NotOnOrAfter" do
-        logout_request.document.root.attributes['NotOnOrAfter'] = '2014-07-17T01:01:48Z'
-        logout_request.soft = false
-        assert_raises(OneLogin::RubySaml::ValidationError, "Current time is on or after NotOnOrAfter") do
-          logout_request.send(:validate_not_on_or_after)
+        Timecop.freeze Time.parse('2014-07-17T01:01:49Z') do
+          logout_request.document.root.attributes['NotOnOrAfter'] = '2014-07-17T01:01:48Z'
+          logout_request.soft = false
+          assert_raises(OneLogin::RubySaml::ValidationError, "Current time is on or after NotOnOrAfter") do
+            logout_request.send(:validate_not_on_or_after)
+          end
+        end
+      end
+
+      it "optionally allows for clock drift" do
+        # Java Floats behave differently than MRI
+        java = defined?(RUBY_ENGINE) && %w[jruby truffleruby].include?(RUBY_ENGINE)
+
+        logout_request.soft = true
+        logout_request.document.root.attributes['NotOnOrAfter'] = '2011-06-14T18:31:01.516Z'
+
+        # The NotBefore condition in the document is 2011-06-1418:31:01.516Z
+        Timecop.freeze(Time.parse("2011-06-14T18:31:02Z")) do
+          logout_request.options[:allowed_clock_drift] = 0.483
+          assert !logout_request.send(:validate_not_on_or_after)
+
+          logout_request.options[:allowed_clock_drift] = java ? 0.485 : 0.484
+          assert logout_request.send(:validate_not_on_or_after)
+
+          logout_request.options[:allowed_clock_drift] = '0.483'
+          assert !logout_request.send(:validate_not_on_or_after)
+
+          logout_request.options[:allowed_clock_drift] = java ? '0.485' : '0.484'
+          assert logout_request.send(:validate_not_on_or_after)
         end
       end
     end

test/xml_security_test.rb

@@ -302,7 +302,7 @@ class XmlSecurityTest < Minitest::Test
       let (:response) { OneLogin::RubySaml::Response.new(fixture(:starfield_response)) }
 
       before do
-        response.settings = OneLogin::RubySaml::Settings.new( :idp_cert_fingerprint => "8D:BA:53:8E:A3:B6:F9:F1:69:6C:BB:D9:D8:BD:41:B3:AC:4F:9D:4D")
+        response.settings = OneLogin::RubySaml::Settings.new(:idp_cert_fingerprint => "8D:BA:53:8E:A3:B6:F9:F1:69:6C:BB:D9:D8:BD:41:B3:AC:4F:9D:4D")
       end
 
       it "be able to validate a good response" do
@@ -320,19 +320,19 @@ class XmlSecurityTest < Minitest::Test
           time_2 = 'Tue Nov 20 17:55:00 UTC 2012 < Wed Nov 28 17:53:45 UTC 2012'
 
           errors = [time_1, time_2].map do |time|
-            "Current time is earlier than NotBefore condition (#{time})"
+            "Current time is earlier than NotBefore condition (#{time} - 1s)"
           end
 
-          assert_predicate response.errors & errors, :any?
+          assert_predicate(response.errors & errors, :any?)
         end
       end
 
       it "fail after response expires" do
         Timecop.freeze Time.parse('2012-11-30 17:55:00 UTC') do
           assert !response.is_valid?
 
-          contains_expected_error = response.errors.include? "Current time is on or after NotOnOrAfter condition (2012-11-30 17:55:00 UTC >= 2012-11-28 18:33:45 UTC)"
-          contains_expected_error ||= response.errors.include? "Current time is on or after NotOnOrAfter condition (Fri Nov 30 17:55:00 UTC 2012 >= Wed Nov 28 18:33:45 UTC 2012)"
+          contains_expected_error = response.errors.include?("Current time is on or after NotOnOrAfter condition (2012-11-30 17:55:00 UTC >= 2012-11-28 18:33:45 UTC + 1s)")
+          contains_expected_error ||= response.errors.include?("Current time is on or after NotOnOrAfter condition (Fri Nov 30 17:55:00 UTC 2012 >= Wed Nov 28 18:33:45 UTC 2012 + 1s)")
           assert contains_expected_error
         end
       end