Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Azure Logout Issues #1847

Open
JackW6809 opened this issue Aug 18, 2023 · 18 comments
Open

Azure Logout Issues #1847

JackW6809 opened this issue Aug 18, 2023 · 18 comments

Comments

@JackW6809
Copy link

JackW6809 commented Aug 18, 2023

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
Copy link
Member

tvdijen commented Aug 18, 2023

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
Copy link
Member

tvdijen commented Aug 18, 2023

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
Copy link
Author

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
Copy link
Member

tvdijen commented Aug 18, 2023

What version are you running?

@JackW6809
Copy link
Author

What version are you running?

Latest, 2.0.5.

@tvdijen
Copy link
Member

tvdijen commented Aug 18, 2023

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
Copy link
Author

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
Copy link
Member

tvdijen commented Aug 18, 2023

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
Copy link
Author

JackW6809 commented Aug 18, 2023

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
Copy link
Member

tvdijen commented Aug 18, 2023

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
Copy link
Author

JackW6809 commented Aug 18, 2023

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
Copy link
Member

tvdijen commented Aug 18, 2023

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

@JackW6809
Copy link
Author

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
Copy link
Member

tvdijen commented Aug 18, 2023

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

@JackW6809
Copy link
Author

JackW6809 commented Aug 18, 2023

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
Copy link
Member

tvdijen commented Aug 18, 2023

Maybe my fellow devs have an idea :)

@JackW6809
Copy link
Author

JackW6809 commented Aug 18, 2023

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
Copy link
Member

tvdijen commented Sep 5, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants