Log warning for SAMLRequests with IssueInstant older than 24 hours

library/EngineBlock/Corto/Module/Bindings.php

@@ -49,6 +49,8 @@ class EngineBlock_Corto_Module_Bindings extends EngineBlock_Corto_Module_Abstrac
 
     const SAML_STATUS_MESSAGE_EMPTY = '(No message provided)';
 
+    const OLD_ISSUE_INSTANT_THRESHOLD_SECONDS = 86400; // 24 hours
+
     protected static $ASSERTION_SEQUENCE = array(
         'saml:Issuer',
         'ds:Signature',
@@ -598,10 +600,22 @@ protected function _verifyKnownIdP($messageIssuer, $destination = '')
      */
     protected function _checkIssueInstant($issueInstant, $type, $entityid)
     {
-        // check the IssueInstant against our own time to see if the SP's clock is getting out of sync
-        // Ssp has a hard-coded limit of 60 seconds; use 30 here to catch an IdP's drifting clock early
         $time = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer()->getTimeProvider()->time();
         $timeDelta = $time - $issueInstant;
+
+        if ($type === 'SP' && $timeDelta > self::OLD_ISSUE_INSTANT_THRESHOLD_SECONDS) {
+            $this->_logger->warning(
+                sprintf(
+                    'IssueInstant of SAMLRequest from SP "%s" is %d seconds (more than 24 hours) in the past; possible replay or severely out-of-sync SP clock',
+                    $entityid,
+                    $timeDelta
+                )
+            );
+            return;
+        }
+
+        // check the IssueInstant against our own time to see if the SP's clock is getting out of sync
+        // Ssp has a hard-coded limit of 60 seconds; use 30 here to catch an IdP's drifting clock early
         if (abs($timeDelta) > 30) {
             $this->_logger->notice(
                 sprintf(

tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php

@@ -238,4 +238,56 @@ public function testAzureDomainHintWhrParamIsCorrectlyUrlEncoded()
         $this->assertArrayHasKey('whr', $params, 'whr query parameter must be present');
         $this->assertSame('example.nl', $params['whr']);
     }
+
+    public function testOldSpIssueInstantLogsWarningOnly()
+    {
+        $logger = $this->mockLogger();
+        $logger->shouldReceive('warning')->once()->with(m::pattern('/is \d+ seconds \(more than 24 hours\) in the past/'));
+        $logger->shouldNotReceive('notice');
+
+        $this->checkIssueInstant(time() - 90000, 'SP', 'https://sp.example.edu');
+    }
+
+    public function testRecentSpIssueInstantLogsNoticeOnly()
+    {
+        $logger = $this->mockLogger();
+        $logger->shouldReceive('notice')->once()->with(m::pattern('/clock synchronization issues/'));
+        $logger->shouldNotReceive('warning');
+
+        $this->checkIssueInstant(time() - 600, 'SP', 'https://sp.example.edu');
+    }
+
+    public function testOldIdpIssueInstantLogsGenericNoticeNotWarning()
+    {
+        $logger = $this->mockLogger();
+        $logger->shouldReceive('notice')->once()->with(m::pattern('/clock synchronization issues/'));
+        $logger->shouldNotReceive('warning');
+
+        $this->checkIssueInstant(time() - 90000, 'IdP', 'https://idp.example.edu');
+    }
+
+    public function testFreshSpIssueInstantLogsNothing()
+    {
+        $logger = $this->mockLogger();
+        $logger->shouldNotReceive('warning');
+        $logger->shouldNotReceive('notice');
+
+        $this->checkIssueInstant(time(), 'SP', 'https://sp.example.edu');
+    }
+
+    private function mockLogger()
+    {
+        $logger = m::mock();
+
+        $loggerProperty = new ReflectionProperty(EngineBlock_Corto_Module_Bindings::class, '_logger');
+        $loggerProperty->setValue($this->bindings, $logger);
+
+        return $logger;
+    }
+
+    private function checkIssueInstant($issueInstant, $type, $entityId)
+    {
+        $method = new ReflectionMethod(EngineBlock_Corto_Module_Bindings::class, '_checkIssueInstant');
+        $method->invoke($this->bindings, $issueInstant, $type, $entityId);
+    }
 }