Incorrect Destination in LogoutResponse when using responseUrl

[mvangeest]

I'm trying to work with an IdP that has an SLO endpoint with different URLs for LogoutRequests and LogoutResponses. That almost works by setting the responseUrl in the singleLogoutService section of the IdP settings: Auth::processSLO correctly uses Auth::getSLOResponseUrl and sends the LogoutResponse to the right URL. However, LogoutResponse::build uses the following code to set the value of Destination attribute:

Destination="{$idpData['singleLogoutService']['url']}"

As a consequence, the Destination value does not match the destination URL, which causes the IdP to reject the message.

The SAML2 standard says (saml-core-2.0-os, section 3.2.2):

Destination [Optional]
A URI reference indicating the address to which this response has been sent. [...] If it is present, the actual recipient MUST check that the URI reference identifies the location at which the message was received. If it does not, the response MUST be discarded.

I, along with the IdP, interpret this to mean Destination should be set to the responseUrl when the LogoutResponse is sent to that URL.

I could've submitted a simple PR to fix this, but I don't think it's a good idea to duplicate the Auth::getSLOResponseURL code inside the LogoutResponse class. I think the most elegant fix would be to move all URL getters (getSLOResponseURL, getSLOURL, getSSOURL) to Settings so that all consuming classes (Auth, AuthnRequest, LogoutRequest, LogoutResponse) can use them. I can probably submit a PR for that if you'd like.

[makije]

Hi @mvangeest,

We are also experiencing this issue, in connection with us integrating with uu.nl
Do you have a temporary fix?
Would you be interested in co-authoring a PR?

[mvangeest]

Our temporary fix is to add the Auth::getSLOResponseURL logic to LogoutResponse::build. (We can't call it because we can't access the Auth instance, and passing it into the method wouldn't be a good idea.) In other words, change

https://github.com/onelogin/php-saml/blob/b41ccc3c6e3386f31b7c333ca7cc45c3eddf5aec/lib/Saml2/LogoutResponse.php#L243-L249

to

        $destination = isset($idpData['singleLogoutService']['responseUrl']) ? $idpData['singleLogoutService']['responseUrl'] : $idpData['singleLogoutService']['url'];
        $logoutResponse = <<<LOGOUTRESPONSE
<samlp:LogoutResponse xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
                  xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
                  ID="{$this->id}"
                  Version="2.0"
                  IssueInstant="{$issueInstant}"
                  Destination="{$destination}"

Would you be interested in co-authoring a PR?

The changes I had in mind wouldn't be very complicated and I'll see if I can put together a PR. It's just that I'd like to hear from the maintainers if they think it's a good idea, because there are several PRs that have been open for a while.

[makije]

Exactly what I ended up doing in our fork.
Fair play, lets see how that plays out.
Would be nice to have a proper fix for this.

[pitbulk]

Sorry for the long delay,

I will take care of this fix. @mvangeest thanks for your suggestions

[mvangeest]

Thanks for fixing this! That solution is exactly what I meant.