flipboxfactory / saml-sp

SAML Service Provider (SP) Plugin for Craft CMS
https://saml-sp.flipboxfactory.com/
Other
19 stars 5 forks source link

Invalid RelayState if not base64 encoded #59

Closed thkus closed 4 years ago

thkus commented 4 years ago

One of our IDPs is not able to send a base64 encoded RelayState which then causes an invalid RelayState, because of the way the RelayState is currently handled. I didn't see a way to configure this – correct me if i'm wrong.

LoginController.php

try {
    $redirect = base64_decode($relayState);
    Saml::info('RelayState: ' . $redirect);
} catch (\Exception $e) {
     $redirect = \Craft::$app->getUser()->getReturnUrl();
}

A simple fix/workaround would be, to check if the decoded URL is valid and if not, return a fallback. So basically something like this should work:

if (filter_var($redirect, FILTER_VALIDATE_URL) === false) {   
    $redirect = \Craft::$app->getUser()->getReturnUrl();
}

Would this be feasible?

dsmrt commented 4 years ago

Hello @thkus,

There may be some confusion on what RelayState's purpose is. I would like some clarification on what you mean by the following:

One of our IDPs is not able to send a base64 encoded RelayState which then causes an invalid RelayState

Is the IdP erroring due to the RelayState being base64'd? Because that shouldn't happen.

The RelayState is state managed by the SP, so the IdP shouldn't really care if it's base64 encoded or not. This is where the RelayState is added from the plugin and you are correct, there isn't a way to override the format currently. The IdP is simply supposed to take the RelayState value and pass it back. Here is a good Stack Overflow on RelayState and here is a snippet from the saml docs (pdf):

3.1.1 Use of RelayState Some bindings define a "RelayState" mechanism for preserving and conveying state information. When such a mechanism is used in conveying a request message as the initial step of a SAML protocol, it places requirements on the selection and use of the binding subsequently used to convey the response. Namely, if a SAML request message is accompanied by RelayState data, then the SAML responder MUST return its SAML protocol response using a binding that also supports a RelayState mechanism, and it MUST place the exact RelayState data it received with the request into the corresponding RelayState parameter in the response.

The snippet you recommended on adding makes sense for further validation but, ultimately, it shouldn't be needed if the plugin is sending and receiving the url correctly. So with that, let me know if you are seeing RelayState not working correctly.

dsmrt commented 4 years ago

@thkus, are you initiating SSO from the IdP. Like, do you start the process from the IdP instead of the SP? I could see that being an issue if that is the case. Can you initiate from the SP (craft) instead? I could see a case to add a "handler" if needed.

thkus commented 4 years ago

Hi @dsmrt,

thanks for the quick reply. Yes indeed, the Request is initiated by the IDP. In this case we are not able to initiate it from the SP (Craft) because the IDP is behind some proxy / firewall. Sorry for not being more specific about that.

A handler would also be a good idea or a setting to specify (on an IDP basis) wether do decode the RelayState or not.

dsmrt commented 4 years ago

I added this setting: https://github.com/flipboxfactory/saml-sp/blob/2.1.8/src/models/Settings.php#L177

Please reference these docs to implement the saml-sp.php: https://saml-sp.flipboxfactory.com/configure/settings.html

Let me know how that works out for you!

thkus commented 4 years ago

@dsmrt, thanks for the fix. Unfortunately it won't work for us, because we are using multiple IDPs and some are returning a base64 encoded RelayState. So this setting would now cause problems with those. I think it has to be either a check wether the URL is valid after decoding it or a setting that allows us to enable or disable decoding for each individual IDP.

dsmrt commented 4 years ago

Ok. I might just add an event there so you can do what you need. I was heading in that direction at first but it was getting too complicated for the moment.

I’ll add the event, which means you have to write a little code but the IdP and SP will be on the event object, as well as the RelayState.

dsmrt commented 4 years ago

@thkus, Just added some events to 2.1.9. I also added an example here: https://saml-sp.flipboxfactory.com/configure/events.html#modify-the-redirect-after-successful-login

Let me know how this all looks!

thkus commented 4 years ago

Works like a charm, @dsmrt! Thanks a lot for the quick support.