From 4b0d08cb638c3d5b5b28616d73e194858c3ce953 Mon Sep 17 00:00:00 2001 From: Ioannis Igoumenos Date: Fri, 10 Apr 2026 03:17:40 +0000 Subject: [PATCH 1/3] Fix pairwise-id SP entityID extraction + tests --- phpunit.xml | 38 +++++----- src/Auth/Process/PairwiseID.php | 53 +++++++++++-- tests/src/Auth/Process/PairwiseIDTest.php | 90 +++++++++++++++++++---- 3 files changed, 144 insertions(+), 37 deletions(-) diff --git a/phpunit.xml b/phpunit.xml index 61b2e07..27f0559 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,4 +1,4 @@ - + - - - ./tests - - + failOnWarning="false"> + + + ./tests + + - - - ./src - - - - - - - + + + + + + + + + + + ./src + + diff --git a/src/Auth/Process/PairwiseID.php b/src/Auth/Process/PairwiseID.php index 7cf56af..d387f27 100644 --- a/src/Auth/Process/PairwiseID.php +++ b/src/Auth/Process/PairwiseID.php @@ -86,10 +86,7 @@ public function process(array &$state): void throw new \InvalidArgumentException('Missing or invalid attribute array in state.'); } - if (!isset($state['Source']['entityid']) || !is_string($state['Source']['entityid'])) { - throw new \InvalidArgumentException('Missing or invalid Source/entityid in state.'); - } - + $spEntityId = $this->extractSpEntityId($state); $secretSalt = $this->getSecretSalt(); if (empty($secretSalt)) { @@ -99,7 +96,7 @@ public function process(array &$state): void $pairwiseId = $this->generatePairwiseId( $state['Attributes'], $this->attribute, - (string)$state['Source']['entityid'], + $spEntityId, $secretSalt, $this->algorithm, $this->scope, @@ -215,4 +212,50 @@ private function computeShibbolethStyleHmacSha256Reference( } return $b32; } + + /** + * Determine which SP entityID to bind a pairwise identifier to. + * + * Applies when: + * - This code runs in an IdP-side processing context where the current relying party (SP) is represented in the + * state (typically via `core:SP`), and + * - You want pairwise IDs to be stable per *ultimate/original* requester in proxy scenarios, not per intermediary. + * If the request has been proxied, SimpleSAMLphp may populate `saml:RequesterID` with a requester chain; when + * present, we take index 0 as the "original requester" entityID. + * + * Produces the "correct" pairwise ID only if: + * - `core:SP` contains the SP entityID for non-proxied flows, and + * - `saml:RequesterID[0]` is in fact the entityID you intend to pair against (commonly the original downstream SP). + * + * Will NOT create/construct the intended pairwise ID when: + * - Your deployment orders `saml:RequesterID` differently (e.g., nearest-first), so `[0]` refers to a proxy rather + * than the downstream SP you mean to target. + * - A hub/broker intentionally omits or rewrites requester information; `saml:RequesterID` may be absent or may + * identify only the broker, making downstream-SP pairwise IDs impossible or misleading. + * - Your intended semantics are "pairwise per immediate requester/proxy" (policy choice). In that case, preferring + * `saml:RequesterID[0]` would be wrong; you would bind to the direct requester instead. + * - This runs in a context where neither `core:SP` nor `saml:RequesterID` are populated (different pipeline/entry + * point), in which case we throw. + * + * @param array $state The SimpleSAMLphp state array for the current request. + * @return string The selected SP entityID. + */ + private function extractSpEntityId(array $state): string + { + if ( + isset($state['saml:RequesterID']) && + is_array($state['saml:RequesterID']) && + isset($state['saml:RequesterID'][0]) && + is_string($state['saml:RequesterID'][0]) && + $state['saml:RequesterID'][0] !== '' + ) { + return $state['saml:RequesterID'][0]; + } + + if (isset($state['core:SP']) && is_string($state['core:SP']) && $state['core:SP'] !== '') { + return $state['core:SP']; + } + + throw new \InvalidArgumentException('Missing SP entityID (core:SP or saml:RequesterID[0]).'); + } } diff --git a/tests/src/Auth/Process/PairwiseIDTest.php b/tests/src/Auth/Process/PairwiseIDTest.php index f1c5db1..9d2708b 100644 --- a/tests/src/Auth/Process/PairwiseIDTest.php +++ b/tests/src/Auth/Process/PairwiseIDTest.php @@ -20,14 +20,9 @@ class PairwiseIDTest extends TestCase 'Attributes' => [ 'uid' => ['774333'] ], - 'Destination' => - [ - 'entityid' => 'https://somesp.edugain.example.edu/sp' - ], - 'Source' => - [ - 'entityid' => 'https://idp.example.edu/shibboleth' - ] + // Pairwise ID is computed per SP; in SSP state this is typically + // `core:SP` (or `saml:RequesterID[0]` for proxied). + 'core:SP' => 'https://somesp.edugain.example.edu/sp', ]; public function testNoConfigOptions(): void @@ -40,8 +35,8 @@ public function testNoConfigOptions(): void $pairwiseId->process($localState); $localState = $this->state; - unset($localState['Source']); - $this->expectExceptionMessage('Missing or invalid Source/entityid in state.'); + unset($localState['core:SP']); + $this->expectExceptionMessage('Missing SP entityID (core:SP or saml:RequesterID[0]).'); $pairwiseId->process($localState); } @@ -77,7 +72,7 @@ public function testDefaultConfig(): void * @throws Exception * @throws \Exception */ - public function testPairwiseID() + public function testPairwiseID(): void { $localState = $this->state; @@ -89,7 +84,7 @@ public function testPairwiseID() $localState['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0], ); - $expectedPairwiseId = 'XEFBRGW7UTQJ6EKRXF6Q4K6FOQG32ZX3@example.com'; + $expectedPairwiseId = 'B7VDEFQKNFXREJWWRDH3FKXBU4S3YGOY@example.com'; $this->assertEquals($expectedPairwiseId, $localState['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0]); } @@ -149,7 +144,7 @@ public function testGeneratePairwiseIdHmacSha256MatchesReference(): void /** * @throws \Exception */ - public function testPairwiseIDFailOnEmptyAttribute() + public function testPairwiseIDFailOnEmptyAttribute(): void { $pairwiseId = new PairwiseID($this->config, null); $localState = $this->state; @@ -213,7 +208,7 @@ public function testMissingAlgorithmConfigurationThrows(): void $this->expectException(\UnexpectedValueException::class); $this->expectExceptionMessage("Could not retrieve the required option 'algorithm'"); - $pairwise = new PairwiseID($config, null); + new PairwiseID($config, null); } public function testAlgorithmsProduceDifferentOutputs(): void @@ -231,4 +226,71 @@ public function testAlgorithmsProduceDifferentOutputs(): void $this->assertNotSame($sha1, $hmac, 'SHA-1 and HMAC-SHA256 outputs must differ'); } + + public function testMissingSpEntityIdThrows(): void + { + $pairwise = new PairwiseID($this->config, null); + + $localState = $this->state; + unset($localState['core:SP']); + unset($localState['saml:RequesterID']); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Missing SP entityID (core:SP or saml:RequesterID[0]).'); + $pairwise->process($localState); + } + + public function testRequesterIdTakesPrecedenceOverCoreSp(): void + { + $pairwise = $this->getMockBuilder(PairwiseID::class) + ->setConstructorArgs([$this->config, null]) + ->onlyMethods(['getSecretSalt']) + ->getMock(); + + $pairwise->method('getSecretSalt')->willReturn('secretsalt'); + + $localState = $this->state; + $localState['core:SP'] = 'https://core-sp.example.org/sp'; + $localState['saml:RequesterID'] = ['https://requester-sp.example.org/sp']; + + $pairwise->process($localState); + + $expected = $pairwise->generatePairwiseId( + ['uid' => ['774333']], + 'uid', + 'https://requester-sp.example.org/sp', + 'secretsalt', + 'sha1', + 'example.com', + ); + + $this->assertSame($expected, $localState['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0]); + } + + public function testMalformedRequesterIdFallsBackToCoreSp(): void + { + $pairwise = $this->getMockBuilder(PairwiseID::class) + ->setConstructorArgs([$this->config, null]) + ->onlyMethods(['getSecretSalt']) + ->getMock(); + + $pairwise->method('getSecretSalt')->willReturn('secretsalt'); + + $localState = $this->state; + $localState['core:SP'] = 'https://core-sp.example.org/sp'; + $localState['saml:RequesterID'] = 'not-an-array'; + + $pairwise->process($localState); + + $expected = $pairwise->generatePairwiseId( + ['uid' => ['774333']], + 'uid', + 'https://core-sp.example.org/sp', + 'secretsalt', + 'sha1', + 'example.com', + ); + + $this->assertSame($expected, $localState['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0]); + } } From 083532ca198794b489b7021e753cd58259b37ac3 Mon Sep 17 00:00:00 2001 From: Ioannis Igoumenos Date: Tue, 14 Apr 2026 16:27:48 +0000 Subject: [PATCH 2/3] Use Destination.entityid for pairwise-id SP binding and assert pairwise-id changes when Destination entityid changes --- src/Auth/Process/PairwiseID.php | 41 ++++---------- tests/src/Auth/Process/PairwiseIDTest.php | 68 ++++++++++++----------- 2 files changed, 48 insertions(+), 61 deletions(-) diff --git a/src/Auth/Process/PairwiseID.php b/src/Auth/Process/PairwiseID.php index d387f27..8b23e32 100644 --- a/src/Auth/Process/PairwiseID.php +++ b/src/Auth/Process/PairwiseID.php @@ -216,26 +216,12 @@ private function computeShibbolethStyleHmacSha256Reference( /** * Determine which SP entityID to bind a pairwise identifier to. * - * Applies when: - * - This code runs in an IdP-side processing context where the current relying party (SP) is represented in the - * state (typically via `core:SP`), and - * - You want pairwise IDs to be stable per *ultimate/original* requester in proxy scenarios, not per intermediary. - * If the request has been proxied, SimpleSAMLphp may populate `saml:RequesterID` with a requester chain; when - * present, we take index 0 as the "original requester" entityID. + * Product position: + * - Pairwise IDs MUST be generated for the SP that is directly integrated with us (the immediate relying party), + * not for an "ultimate/original requester" in proxied/hub scenarios. * - * Produces the "correct" pairwise ID only if: - * - `core:SP` contains the SP entityID for non-proxied flows, and - * - `saml:RequesterID[0]` is in fact the entityID you intend to pair against (commonly the original downstream SP). - * - * Will NOT create/construct the intended pairwise ID when: - * - Your deployment orders `saml:RequesterID` differently (e.g., nearest-first), so `[0]` refers to a proxy rather - * than the downstream SP you mean to target. - * - A hub/broker intentionally omits or rewrites requester information; `saml:RequesterID` may be absent or may - * identify only the broker, making downstream-SP pairwise IDs impossible or misleading. - * - Your intended semantics are "pairwise per immediate requester/proxy" (policy choice). In that case, preferring - * `saml:RequesterID[0]` would be wrong; you would bind to the direct requester instead. - * - This runs in a context where neither `core:SP` nor `saml:RequesterID` are populated (different pipeline/entry - * point), in which case we throw. + * In SimpleSAMLphp state, that direct SP is represented as: + * - `$state['Destination']['entityid']` * * @param array $state The SimpleSAMLphp state array for the current request. * @return string The selected SP entityID. @@ -243,19 +229,14 @@ private function computeShibbolethStyleHmacSha256Reference( private function extractSpEntityId(array $state): string { if ( - isset($state['saml:RequesterID']) && - is_array($state['saml:RequesterID']) && - isset($state['saml:RequesterID'][0]) && - is_string($state['saml:RequesterID'][0]) && - $state['saml:RequesterID'][0] !== '' + isset($state['Destination']['entityid']) + && is_array($state['Destination']) + && is_string($state['Destination']['entityid']) + && $state['Destination']['entityid'] !== '' ) { - return $state['saml:RequesterID'][0]; - } - - if (isset($state['core:SP']) && is_string($state['core:SP']) && $state['core:SP'] !== '') { - return $state['core:SP']; + return $state['Destination']['entityid']; } - throw new \InvalidArgumentException('Missing SP entityID (core:SP or saml:RequesterID[0]).'); + throw new \InvalidArgumentException('Missing SP entityID (Destination[entityid]).'); } } diff --git a/tests/src/Auth/Process/PairwiseIDTest.php b/tests/src/Auth/Process/PairwiseIDTest.php index 9d2708b..9e90cf5 100644 --- a/tests/src/Auth/Process/PairwiseIDTest.php +++ b/tests/src/Auth/Process/PairwiseIDTest.php @@ -13,15 +13,18 @@ class PairwiseIDTest extends TestCase private array $config = [ 'attribute' => 'uid', 'scope' => 'example.com', - 'algorithm' => 'sha1' + 'algorithm' => 'sha1', ]; private array $state = [ 'Attributes' => [ 'uid' => ['774333'] ], - // Pairwise ID is computed per SP; in SSP state this is typically - // `core:SP` (or `saml:RequesterID[0]` for proxied). + // Pairwise ID is computed per *directly integrated* SP; in SSP IdP state this is Destination[entityid]. + 'Destination' => [ + 'entityid' => 'https://somesp.edugain.example.edu/sp', + ], + // Kept only for legacy/back-compat state shape; no longer used for pairwise-id selection. 'core:SP' => 'https://somesp.edugain.example.edu/sp', ]; @@ -35,8 +38,8 @@ public function testNoConfigOptions(): void $pairwiseId->process($localState); $localState = $this->state; - unset($localState['core:SP']); - $this->expectExceptionMessage('Missing SP entityID (core:SP or saml:RequesterID[0]).'); + unset($localState['Destination']); + $this->expectExceptionMessage('Missing SP entityID (Destination[entityid]).'); $pairwiseId->process($localState); } @@ -74,9 +77,9 @@ public function testDefaultConfig(): void */ public function testPairwiseID(): void { + $pairwiseId = new PairwiseID($this->config, null); $localState = $this->state; - $pairwiseId = new PairwiseID($this->config, null); $pairwiseId->process($localState); $this->assertArrayHasKey(PairwiseID::PAIRWISEID_ATTR_NAME, $localState['Attributes']); $this->assertStringEndsWith( @@ -198,7 +201,7 @@ public function testInvalidAlgorithmThrows(): void $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage("Invalid algorithm. Allowed: 'sha1', 'hmac-sha256'."); - $pairwise->generatePairwiseId($attributes, 'uid', $sp, $salt, $scope, 'md5'); + $pairwise->generatePairwiseId($attributes, 'uid', $sp, $salt, 'md5', $scope); } public function testMissingAlgorithmConfigurationThrows(): void @@ -206,7 +209,7 @@ public function testMissingAlgorithmConfigurationThrows(): void $config = $this->config; unset($config['algorithm']); - $this->expectException(\UnexpectedValueException::class); + $this->expectException(\Exception::class); $this->expectExceptionMessage("Could not retrieve the required option 'algorithm'"); new PairwiseID($config, null); } @@ -227,20 +230,7 @@ public function testAlgorithmsProduceDifferentOutputs(): void $this->assertNotSame($sha1, $hmac, 'SHA-1 and HMAC-SHA256 outputs must differ'); } - public function testMissingSpEntityIdThrows(): void - { - $pairwise = new PairwiseID($this->config, null); - - $localState = $this->state; - unset($localState['core:SP']); - unset($localState['saml:RequesterID']); - - $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('Missing SP entityID (core:SP or saml:RequesterID[0]).'); - $pairwise->process($localState); - } - - public function testRequesterIdTakesPrecedenceOverCoreSp(): void + public function testDestinationEntityIdIsUsedEvenIfCoreSpOrRequesterIdPresent(): void { $pairwise = $this->getMockBuilder(PairwiseID::class) ->setConstructorArgs([$this->config, null]) @@ -250,6 +240,7 @@ public function testRequesterIdTakesPrecedenceOverCoreSp(): void $pairwise->method('getSecretSalt')->willReturn('secretsalt'); $localState = $this->state; + $localState['Destination']['entityid'] = 'https://destination-sp.example.org/sp'; $localState['core:SP'] = 'https://core-sp.example.org/sp'; $localState['saml:RequesterID'] = ['https://requester-sp.example.org/sp']; @@ -258,7 +249,7 @@ public function testRequesterIdTakesPrecedenceOverCoreSp(): void $expected = $pairwise->generatePairwiseId( ['uid' => ['774333']], 'uid', - 'https://requester-sp.example.org/sp', + 'https://destination-sp.example.org/sp', 'secretsalt', 'sha1', 'example.com', @@ -267,7 +258,7 @@ public function testRequesterIdTakesPrecedenceOverCoreSp(): void $this->assertSame($expected, $localState['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0]); } - public function testMalformedRequesterIdFallsBackToCoreSp(): void + public function testPairwiseIdChangesWhenDestinationEntityIdChanges(): void { $pairwise = $this->getMockBuilder(PairwiseID::class) ->setConstructorArgs([$this->config, null]) @@ -276,21 +267,36 @@ public function testMalformedRequesterIdFallsBackToCoreSp(): void $pairwise->method('getSecretSalt')->willReturn('secretsalt'); - $localState = $this->state; - $localState['core:SP'] = 'https://core-sp.example.org/sp'; - $localState['saml:RequesterID'] = 'not-an-array'; + $stateOne = $this->state; + $stateOne['Destination']['entityid'] = 'https://destination-one.example.org/sp'; + $pairwise->process($stateOne); + $valueOne = $stateOne['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0]; - $pairwise->process($localState); + $stateTwo = $this->state; + $stateTwo['Destination']['entityid'] = 'https://destination-two.example.org/sp'; + $pairwise->process($stateTwo); + $valueTwo = $stateTwo['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0]; - $expected = $pairwise->generatePairwiseId( + $this->assertNotSame($valueOne, $valueTwo, 'Pairwise ID must change when Destination[entityid] changes'); + + $expectedOne = $pairwise->generatePairwiseId( ['uid' => ['774333']], 'uid', - 'https://core-sp.example.org/sp', + 'https://destination-one.example.org/sp', + 'secretsalt', + 'sha1', + 'example.com', + ); + $expectedTwo = $pairwise->generatePairwiseId( + ['uid' => ['774333']], + 'uid', + 'https://destination-two.example.org/sp', 'secretsalt', 'sha1', 'example.com', ); - $this->assertSame($expected, $localState['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0]); + $this->assertSame($expectedOne, $valueOne); + $this->assertSame($expectedTwo, $valueTwo); } } From 7cdcd90a250e90a3ff2d53767a11f3240bfa6a21 Mon Sep 17 00:00:00 2001 From: Ioannis Igoumenos Date: Tue, 14 Apr 2026 16:32:11 +0000 Subject: [PATCH 3/3] Remove redendant test --- tests/src/Auth/Process/PairwiseIDTest.php | 28 ----------------------- 1 file changed, 28 deletions(-) diff --git a/tests/src/Auth/Process/PairwiseIDTest.php b/tests/src/Auth/Process/PairwiseIDTest.php index 9e90cf5..ae21808 100644 --- a/tests/src/Auth/Process/PairwiseIDTest.php +++ b/tests/src/Auth/Process/PairwiseIDTest.php @@ -230,34 +230,6 @@ public function testAlgorithmsProduceDifferentOutputs(): void $this->assertNotSame($sha1, $hmac, 'SHA-1 and HMAC-SHA256 outputs must differ'); } - public function testDestinationEntityIdIsUsedEvenIfCoreSpOrRequesterIdPresent(): void - { - $pairwise = $this->getMockBuilder(PairwiseID::class) - ->setConstructorArgs([$this->config, null]) - ->onlyMethods(['getSecretSalt']) - ->getMock(); - - $pairwise->method('getSecretSalt')->willReturn('secretsalt'); - - $localState = $this->state; - $localState['Destination']['entityid'] = 'https://destination-sp.example.org/sp'; - $localState['core:SP'] = 'https://core-sp.example.org/sp'; - $localState['saml:RequesterID'] = ['https://requester-sp.example.org/sp']; - - $pairwise->process($localState); - - $expected = $pairwise->generatePairwiseId( - ['uid' => ['774333']], - 'uid', - 'https://destination-sp.example.org/sp', - 'secretsalt', - 'sha1', - 'example.com', - ); - - $this->assertSame($expected, $localState['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0]); - } - public function testPairwiseIdChangesWhenDestinationEntityIdChanges(): void { $pairwise = $this->getMockBuilder(PairwiseID::class)