From 58c7561b10bc7f00d4a27e813704233a8a706f61 Mon Sep 17 00:00:00 2001
From: Kay Joosten <kay.joosten@dawn.tech>
Date: Mon, 20 Jul 2026 09:42:21 +0200
Subject: [PATCH] Log warning for SAMLRequests with IssueInstant older than 24
 hours

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.

https://github.com/OpenConext/OpenConext-engineblock/issues/1972
---
 library/EngineBlock/Corto/Module/Bindings.php | 18 ++++++-
 .../Test/Corto/Module/BindingsTest.php        | 52 +++++++++++++++++++
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/library/EngineBlock/Corto/Module/Bindings.php b/library/EngineBlock/Corto/Module/Bindings.php
index 66780d625..b52361ef4 100644
--- a/library/EngineBlock/Corto/Module/Bindings.php
+++ b/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(
diff --git a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php
index 5b8ef858c..5cc885034 100644
--- a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php
+++ b/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);
+    }
 }