Support for IdP initiated proxy SLO #1553

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

@@ -484,9 +520,11 @@ public function handleLogoutRequest(array &$state, ?string $assocId): void
        $id = Auth\State::saveState($state, 'core:Logout:afterbridge');
        $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 from 29f7b69 to 1a911ce Compare May 12, 2023 16:07

tvdijen force-pushed the master branch 3 times, most recently from c7c8357 to fdbe001 Compare June 12, 2023 14:28

tvdijen force-pushed the master branch 8 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for IdP initiated proxy SLO #1553

Support for IdP initiated proxy SLO #1553

m0ark commented Jan 6, 2022 •

edited

thijskh left a comment

m0ark commented Jan 19, 2022

thijskh left a comment

thijskh Jan 21, 2022

m0ark Jan 24, 2022

m0ark commented Jan 24, 2022

tvdijen commented Jan 25, 2022

codecov bot commented Jan 26, 2022 •

edited

tvdijen Jan 26, 2022

tvdijen Jan 26, 2022

tvdijen commented Jan 26, 2022

thijskh commented Jan 26, 2022

tvdijen commented Jan 26, 2022

tvdijen commented Jul 19, 2023


		if ($assocId !== null) {
		if ($assocId !== null \|\| $skipAsLogout) {

Support for IdP initiated proxy SLO #1553

Are you sure you want to change the base?

Support for IdP initiated proxy SLO #1553

Conversation

m0ark commented Jan 6, 2022 • edited

Problem

Solution

Components of the framework

thijskh left a comment

Choose a reason for hiding this comment

m0ark commented Jan 19, 2022

thijskh left a comment

Choose a reason for hiding this comment

thijskh Jan 21, 2022

Choose a reason for hiding this comment

m0ark Jan 24, 2022

Choose a reason for hiding this comment

m0ark commented Jan 24, 2022

tvdijen commented Jan 25, 2022

codecov bot commented Jan 26, 2022 • edited

Codecov Report

tvdijen Jan 26, 2022

Choose a reason for hiding this comment

tvdijen Jan 26, 2022

Choose a reason for hiding this comment

tvdijen commented Jan 26, 2022

thijskh commented Jan 26, 2022

tvdijen commented Jan 26, 2022

tvdijen commented Jul 19, 2023

m0ark commented Jan 6, 2022 •

edited

codecov bot commented Jan 26, 2022 •

edited