Log warning for SAMLRequests with IssueInstant older than 24 hours (#2061)

Why is this change needed?
Prior to this change, EngineBlock only logged a generic clock-skew
notice when an incoming message's IssueInstant differed from server
time by more than 30 seconds, in either direction, for both
AuthnRequests and Responses. There was no way to distinguish an SP
sending severely stale AuthnRequests (>24h old, possibly indicating
replay or a badly out-of-sync clock) from routine minor clock drift.

How does it address the issue?
This change adds a dedicated warning for severely stale AuthnRequests,
taking priority over the existing minor-drift notice so only the
single most-applicable message fires per request. The check is scoped
to AuthnRequests only, since Responses are handled differently and
weren't part of the reported issue. This is logging only, no
requests are blocked.

#1972

Author: kayjoosten

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);
+    }
 }