Support for IdP initiated proxy SLO by m0ark · Pull Request #1553 · simplesamlphp/simplesamlphp

m0ark · 2022-01-06T08:44:31Z

Problem

SimpleSAMLphp was not designed to support protocol proxy setups. Fortunately it supports a lot of those use cases.
One particular feature that has always been missing in proxy scenarios is the capability to properly propagate logout (SLO) requests coming from an upstream identity provider. This is because of SimpleSAMLphp IdPs using its SPs as authentication sources. In case of an IdP initiated logout the proxy SP is notified about the logout request and terminates its session properly without knowing about components it has been used by and thus cannot propagate the logout request to.
The result of this behaviour is the users IdP receiving a logout response telling the SLO has properly finished but the proxy IdP(s) still having an active session.

This issue has been discussed in #65 and @relaxnow made a great job implementing a first draft to tackle it (relaxnow@35cffcc).
Nevertheless this implementation was way too tightly coupled to SAML components and lacked a proper differentiation between multiple identity providers.

Solution

This patch is based on the work of @relaxnow and adds a small framework for adding IdP initiated proxy SLO capabilities. It also implements the framework for SAML SPs and SAML/ADFS IdPs to support proxy SLO flows using frontend channels.
It should be possible to add proxy SLO capabilities to other IdP/SP protocol implementations (e.g. OIDC, CAS) using this framework without much of a hassle.

To not break the current behaviour proxy SLO has to be activated by setting
‘proxyLogoutEnable’ => true
in each SP configuration.

Components of the framework

• registerAsConsumerLogoutCallback
To be called by an authentication source consumer (e.g. IdP). The callback should handle everything that is needed to logout the consumer from its own perspective. In case of an IdP this could be logging out of all associated SPs; in case of a PHP application this could terminate the application local session.
• hasAsConsumerLogoutCallbacks
Check if authentication source consumer logout callbacks have been registered for this source.
• callAsConsumerLogoutCallbacks
To be called by a regular authentication source logout routine (e.g. SAML SP SLO handler) to trigger execution of the registered logout callbacks.
• handleAsConsumerLogoutCallbacks
Only used by callAsConsumerLogoutCallbacks and the returnTo page that is called in case of the callback not returning immediately (e.g. redirecting, etc).

thijskh

Hi! Thanks for the thorough PR.

I have not reviewed it line by line (yet), did add a few observations. And I notice the base branch is simplesamlphp-1.19. It seems unlikely that we'd merge such a feature there and rather target master.

lib/SimpleSAML/IdP.php

modules/saml/lib/Auth/Source/SP.php

m0ark · 2022-01-19T08:28:18Z

Thanks for your first review.
I implemented this change using the version we used in production. So I just wanted to discuss the probability of having the PR accepted first before putting in more effort and porting this change to master.

thijskh

I've checked the code and believe it to be OK. However, I don't have a lot of experience with the scenario in question nor do I have a representative environment. So I'd like to add the caveat that I have not verified it to actually work myself. If you use this in actual production I'd be quite confident to merge it though.

Did you give any consideration as to whether at least some basic tests could be added for this?

lib/SimpleSAML/Auth/Source.php

thijskh · 2022-01-21T15:35:52Z

modules/saml/www/sp/saml2-logout-resume.php

+$lr->setInResponseTo($message->getId());
+
+if ($numLoggedOut < count($sessionIndexes)) {
+    \SimpleSAML\Logger::warning('Logged out of ' . $numLoggedOut . ' of ' . count($sessionIndexes) . ' sessions.');


Why is this a warning?

It's just a copy of the working code of saml2-logout.php.
I think it is not expected to have any active session indexes left of the current SLO request after reaching this point.

m0ark · 2022-01-24T08:29:05Z

Thank you for your thorough review and your helpful annotations.

Do you have any pointers on where to add tests for this code?

For an environment just create a simple SAML proxy and trigger an IdP initiated SLO request.
Without enabling this feature the logout request will not propagate to the proxy IdP component.

tvdijen · 2022-01-25T23:50:21Z

Also closes #1565

codecov · 2022-01-26T00:41:11Z

Codecov Report

Merging #1553 (bcb357c) into master (b7e26e3) will decrease coverage by 0.24%.
The diff coverage is 1.47%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1553      +/-   ##
============================================
- Coverage     44.09%   43.85%   -0.25%     
- Complexity     3680     3696      +16     
============================================
  Files           156      156              
  Lines         10889    10951      +62     
============================================
+ Hits           4802     4803       +1     
- Misses         6087     6148      +61

tvdijen · 2022-01-26T00:40:34Z

lib/SimpleSAML/IdP.php

        $returnTo = Module::getModuleURL('core/idp/resumelogout.php', ['id' => $id]);

-        $this->authSource->logout($returnTo);
+        if (!$skipAsLogout) {


I'd like to see strict comparison here

tvdijen · 2022-01-26T00:40:43Z

lib/SimpleSAML/IdP.php

+        }

-        if ($assocId !== null) {
+        if ($assocId !== null || $skipAsLogout) {


tvdijen · 2022-01-26T00:42:07Z

I hope I didn't break anything, but I've rebased the lot against master..

thijskh · 2022-01-26T10:50:09Z

I think the rebasing was not entirely accurate, at least i've spotted the reintroduction of this code: https://github.com/simplesamlphp/simplesamlphp/pull/1553/files#diff-60baa677d9f59b02cd5b3bbd371583dc05eccd0cb0bb74f2003398031fd84f8dR109-R111

tvdijen · 2022-01-26T12:25:12Z

Good catch! I've fixed that.. Something else that needs to change is the www-script.. It should be converted to a Controller

this is used in proxy mode logout scenario to prevent logging out the authentication source twice.

tvdijen · 2023-07-19T14:19:36Z

I'm not sure how to fix the conflicts in src/SimpleSAML/IdP.php because the code has been refactored for Symfony responses

thijskh reviewed Jan 18, 2022

View reviewed changes

lib/SimpleSAML/IdP.php Outdated Show resolved Hide resolved

modules/saml/lib/Auth/Source/SP.php Outdated Show resolved Hide resolved

m0ark mentioned this pull request Jan 19, 2022

Fix: ADFS IdP loads metadata of disabled SAML IdP #1565

Merged

thijskh reviewed Jan 21, 2022

View reviewed changes

tvdijen closed this Jan 25, 2022

tvdijen reopened this Jan 25, 2022

tvdijen linked an issue Jan 25, 2022 that may be closed by this pull request

ADFS IdP loads metadata of disabled SAML IdP #1564

Closed

tvdijen changed the base branch from simplesamlphp-1.19 to master January 26, 2022 00:02

tvdijen force-pushed the proxy-logout-v1.19.1 branch from 657a09d to 683a0ea Compare January 26, 2022 00:37

tvdijen approved these changes Jan 26, 2022

View reviewed changes

tvdijen force-pushed the master branch from 37e0650 to 3543330 Compare March 17, 2022 20:30

tvdijen force-pushed the master branch from 6f142ba to 69bec7b Compare July 14, 2022 20:43

tvdijen force-pushed the master branch from d0563a7 to c32e2ed Compare August 14, 2022 08:14

m0ark and others added 11 commits September 14, 2022 16:51

add callback infrastructure to enable idp initiated / proxy mode logout

0122f06

fix missing LR destination in saml2-logout-resume.php

8fed6fc

add flag for idp logout to skip logging out of authentication source.

381bb26

this is used in proxy mode logout scenario to prevent logging out the authentication source twice.

remove code formerly used to work around the missing proxy SLO.

b494c73

move registering of asConsumerLogoutCallback to postAuth

791b5ba

add missing php doc

571f181

add missing type hints and remove type assertions

0f2546f

remove redundant check

eab3b07

optimize code structure of callback loop for better readability

296754e

Coding style

a60b8ca

Coding style

fcdd559

tvdijen force-pushed the master branch 5 times, most recently from 3b5f5ba to 96357ee Compare July 19, 2023 12:37

tvdijen force-pushed the master branch 5 times, most recently from 7587851 to d523b31 Compare August 2, 2023 11:58

tvdijen force-pushed the master branch 3 times, most recently from 8c90121 to d534e3b Compare September 5, 2023 08:09

tvdijen force-pushed the master branch 2 times, most recently from bc1c5c8 to d0a5974 Compare October 17, 2023 21:17

tvdijen force-pushed the master branch 3 times, most recently from ccb9b02 to 120a100 Compare December 1, 2023 14:34

tvdijen force-pushed the master branch 2 times, most recently from 6004a77 to 58bf8db Compare May 4, 2024 23:45

tvdijen force-pushed the master branch 2 times, most recently from 5c9fb2c to 0970efc Compare May 27, 2024 12:27

tvdijen force-pushed the master branch 2 times, most recently from c27831c to 71e49f4 Compare June 19, 2024 17:03

tvdijen force-pushed the master branch from 0849f01 to 1730f2c Compare July 1, 2025 22:07

tvdijen force-pushed the master branch 3 times, most recently from c06a17a to a52c98d Compare August 20, 2025 21:45

tvdijen force-pushed the master branch from f14a640 to f8590fa Compare January 28, 2026 06:56

Conversation

m0ark commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Components of the framework

Uh oh!

thijskh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

m0ark commented Jan 19, 2022

Uh oh!

thijskh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thijskh Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

m0ark Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

m0ark commented Jan 24, 2022

Uh oh!

tvdijen commented Jan 25, 2022

Uh oh!

codecov bot commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tvdijen Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

tvdijen Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

tvdijen commented Jan 26, 2022

Uh oh!

thijskh commented Jan 26, 2022

Uh oh!

tvdijen commented Jan 26, 2022

Uh oh!

tvdijen commented Jul 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

m0ark commented Jan 6, 2022 •

edited

Loading

codecov bot commented Jan 26, 2022 •

edited

Loading