simplesamlphp / simplesamlphp

SimpleSAMLphp is an application written in native PHP that deals with authentication.
https://simplesamlphp.org
GNU Lesser General Public License v2.1
1.05k stars 674 forks source link

Azure Logout Issues #1847

Open JackW6809 opened 10 months ago

JackW6809 commented 10 months ago

Describe the bug I have setup a custom SAML Enterprise Application on Azure Active Directory. I am trying to use SimpleSAML to authenticate through Azure for some local applications. However I am having issues with the logout section. It logs out fine however in my logged_out.php it errors out:

Array ( [ReturnTo] => /logged_out.php [ReturnStateParam] => LogoutState [ReturnStateStage] => MyLogoutState [\SimpleSAML\Auth\State.id] => _d77c5a769f2c403c06c2e6eba820fcee3d19558621 [\SimpleSAML\Auth\State.stage] => MyLogoutState )
Logout status information is not available for this type of application.

Here is my logout.php file for reference:

<?php
ini_set('display_errors', 1);
error_reporting(E_ALL);
require_once('/var/www/simplesamlphp/lib/_autoload.php');
$as = new \SimpleSAML\Auth\Simple('jackoxi-sp');

$as->logout([
    'ReturnTo' => '/logged_out.php',
    'ReturnStateParam' => 'LogoutState',
    'ReturnStateStage' => 'MyLogoutState',
]);
\SimpleSAML\Session::getSessionFromRequest()->cleanup();

And here is my logged_out.php for reference:

<?php
ini_set('display_errors', 1);
error_reporting(E_ALL);
require_once('/var/www/simplesamlphp/lib/_autoload.php');
$as = new \SimpleSAML\Auth\State;

try {
    if ($_REQUEST['LogoutState']) {
        $state = $as::loadState((string)$_REQUEST['LogoutState'], 'MyLogoutState');
    } else {
        echo "Were you logged in?";
        exit;
    }
} catch (Exception $e) {
    echo 'Caught exception: ',  $e->getMessage(), "\n";
    exit;
}

print_r($state);
echo '</br>';

if (isset($state['saml:sp:LogoutStatus'])) {
    $ls = $state['saml:sp:LogoutStatus'];
    if ($ls['Code'] === 'urn:oasis:names:tc:SAML:2.0:status:Success' && !isset($ls['SubCode'])) {
        // Successful logout.
        echo "You have been logged out.";
    } else {
        // Logout failed. Tell the user to close the browser.
        echo "We were unable to log you out of all your sessions. To be completely sure that you are logged out, you need to close your web browser.";
    }
} else {
    // Handle the case where the 'saml:sp:LogoutStatus' key is not present (not applicable to your use case).
    echo "Logout status information is not available for this type of application.";
}

To Reproduce Steps to reproduce the behaviour:

  1. Setup an Azure Active Directory
  2. Create a Custom Enterprise App
  3. Go to Single sign-on and enable SAML.
  4. Setup accordingly on SimpleSAML and Azure.
  5. Try to use the logout function with the code above and you should receive the same error.

Expected behaviour Tell me that I have successfully logged out

Additional context Just note that Azure does log me out, it just does not pass the correct information it seems. If there is anything I am missing please let me know so I can send it in.

This is what I was originally getting: Undefined array key "saml:sp:LogoutStatus" in /var/www/sso/logged_out.php

tvdijen commented 10 months ago

What happens if in logged_out.php you replace $as::loadState with \SimpleSAML\Auth\State::loadState? I think maybe because you're instantiating a new State-object, it will not be able to find the previous state?

tvdijen commented 10 months ago

Just note that Azure does log me out, it just does not pass the correct information it seems.

Azure doesn't pass anything.. What you're doing here is restoring the state that you had before the $as->logout() method redirected you to Azure to log you out. It's information saved in your SSP session and not something that travels with you from/to Azure.

JackW6809 commented 10 months ago

What happens if in logged_out.php you replace $as::loadState with \SimpleSAML\Auth\State::loadState? I think maybe because you're instantiating a new State-object, it will not be able to find the previous state?

I did think that, just tried to do it again and unfortunately the same issue persists.

tvdijen commented 10 months ago

What version are you running?

JackW6809 commented 10 months ago

What version are you running?

Latest, 2.0.5.

tvdijen commented 10 months ago

OK thanks, I was confused because you use the lib/ path instead of the src/ path.. lib/ only exists for legacy reasons and is just a symlink... Not related to this issue at all though.. It's going to be really tough for us to debug this, since pretty much nobody uses SingleLogout and I don't have access to AAD..

JackW6809 commented 10 months ago

OK thanks, I was confused because you use the lib/ path instead of the src/ path.. lib/ only exists for legacy reasons and is just a symlink... Not related to this issue at all though.. It's going to be really tough for us to debug this, since pretty much nobody uses SingleLogout and I don't have access to AAD..

To be honest, I might need to write it again or something. I had this SimpleSAML project lying around which was pretty out of date so I updated it to 2.0.5 and tried to integrate into AAD. Had some teething issues but seemed ok mostly.

I have updated it from lib to src just to be sure that wasnt causing any issues.

By SingleLogout, what do you mean? Sorry aha, if there is a better logout function I would be more than happy to hear it. I was just looking at the docs which had this method.

tvdijen commented 10 months ago

The functionality you're trying to use is called SingleLogout in SAML2. It will log you out everywhere you had an active session. The concept however is broken by design and gives a false sense of security, because there is absolutely no guarantee that you're logged out at all service providers connected to the IdP.

Anyway, I'm thinking maybe the state isn't saved before the saml:sp:LogoutStatus variable is set.. You could try the following:

diff --git a/modules/saml/src/Controller/ServiceProvider.php b/modules/saml/src/Controller/ServiceProvider.php
index 167fbaabe..02d11a11d 100644
--- a/modules/saml/src/Controller/ServiceProvider.php
+++ b/modules/saml/src/Controller/ServiceProvider.php
@@ -510,6 +510,7 @@ class ServiceProvider

             $state = $this->authState::loadState($relayState, 'saml:slosent');
             $state['saml:sp:LogoutStatus'] = $message->getStatus();
+            $this->authState::saveState($state);
             return $source::completeLogout($state);
         } elseif ($message instanceof LogoutRequest) {
             Logger::debug('module/saml2/sp/logout: Request from ' . $idpEntityId);
JackW6809 commented 10 months ago

Ignore my deleted message - I added that line in but same error unfortunately. Also not sure if this is relevant, but in the URL on the logged_out.php page it only has the LogoutState passed.

tvdijen commented 10 months ago

Curious to know if $this->authState::saveState($state, 'LogoutState'); makes a difference.. If not, I'm out of quick ideas and this has to properly debugged

JackW6809 commented 10 months ago

Unfortunately leads to the same issue. Let me know what I can do to help with debugging if that is something you need, please. :)

tvdijen commented 10 months ago

More hours in a day basically :) All of us have day-jobs

JackW6809 commented 10 months ago

Aha yeah if only lol. Just came back from work to see if I could find any glaring issues but seems ok. Just ping whenever those magical extra hours appear haha.

tvdijen commented 10 months ago

Deleted a message again? Because that seemed like it did what it's supposed to do..

JackW6809 commented 10 months ago

Sorry, yes it was deleted... I turned off my debugging messaged lol. Unless all this is happening because of that buggy SingleLogout which it shouldnt be from what was mentioned.

logged_out.php

<?php
ini_set('display_errors', 1);
error_reporting(E_ALL);
require_once('/var/www/simplesamlphp/src/_autoload.php');

try {
    if ($_REQUEST['LogoutState']) {
        $state = \SimpleSAML\Auth\State::loadState((string)$_REQUEST['LogoutState'], 'MyLogoutState');
    } else {
        echo "Were you logged in?";
        exit;
    }
} catch (Exception $e) {
    echo 'Caught exception: ',  $e->getMessage(), "\n";
    exit;
}

$ls = $state['saml:sp:LogoutStatus']; /* Only works for SAML SP */
if ($ls['Code'] === 'urn:oasis:names:tc:SAML:2.0:status:Success' && !isset($ls['SubCode'])) {
    // Successful logout.
    echo("You have been logged out.");
} else {
    // Logout failed. Tell the user to close the browser.
    echo("We were unable to log you out of all your sessions. To be completely sure that you are logged out, you need to close your web browser.");
}

Browser Page:


Warning: Undefined array key "saml:sp:LogoutStatus" in /var/www/sso/logged_out.php on line 18

Warning: Undefined array key "saml:sp:LogoutStatus" in /var/www/sso/logged_out.php on line 19

Warning: Trying to access array offset on value of type null in /var/www/sso/logged_out.php on line 20
We were unable to log you out of all your sessions. To be completely sure that you are logged out, you need to close your web browser.
tvdijen commented 10 months ago

Maybe my fellow devs have an idea :)

JackW6809 commented 10 months ago

Hmm, a change in behaviour again... I just was messing around with that ServiceProvider and there was a few discrepancies. My return was different. So this is the new behaviour. Seems like the Warning: Trying to access array offset on value of type null in /var/www/sso/logged_out.php on line 20 issue is gone.


Warning: Undefined array key "saml:sp:LogoutStatus" in /var/www/sso/logged_out.php on line 18

Warning: Trying to access array offset on value of type null in /var/www/sso/logged_out.php on line 19
We were unable to log you out of all your sessions. To be completely sure that you are logged out, you need to close your web browser.
            $state = $this->authState::loadState($relayState, 'saml:slosent');
            $state['saml:sp:LogoutStatus'] = $message->getStatus();
            $this->authState::saveState($state, 'LogoutState');
            return $source::completeLogout($state);
        } elseif ($message instanceof LogoutRequest) {
            Logger::debug('module/saml2/sp/logout: Request from ' . $idpEntityId);
tvdijen commented 9 months ago

OK, I think I understand what's happening here. I can reproduce the issue when the IdP has no SingleLogoutService in it's metadata. Can you verify that this is the case for the Azure IdP metadata you have in saml20-idp-remote.php @JackW6809 ?

If there is no SLO-endpoint available, the user will be immediately redirected to logged_out.php, but the state will obviously not contain all the variables.