Update SLO internals to match latest ruby-saml API

[md5]

This PR updates the internals for constructing OneLogin::RubySaml::Logoutresponse and OneLogin::RubySaml::SloLogoutrequest instances to match the latest recommendations for ruby-saml. These changes help avoid character set encoding issues with IdPs using something other that UTF-8.

In addition, the settings object and signature are now being passed, allowing signatures to be validated on these requests and responses. Previously, the signatures were not being checked, so these interactions could be forged to fraudulently log out a user on the SP side.

Closes #147

[coveralls]

Coverage decreased (-0.6%) to 99.422% when pulling d720762d61f9a5d2f35925dabf2faa21ebd3f45d on appropriate:slo-params into 5f716a8c370a7cbf38c3c80c35284ec84cf8ca84 on omniauth:master.

[ADolhov]

@md5 will this be merged anytime soon? Looks like it covers my case, after successful LogoutRequest

<samlp:LogoutRequest Destination='https://sso1.xxxxxxxxxxx.com/idp/SLO.saml2'
    ID='_e7e383bb-40a0-4de0-b776-548abb7fc533' IssueInstant='2018-02-21T19:31:43Z' Version='2.0'
    xmlns:saml='urn:oasis:names:tc:SAML:2.0:assertion'
    xmlns:samlp='urn:oasis:names:tc:SAML:2.0:protocol'>
    <saml:Issuer>http://www.okta.com/xxxxxxxxxxxxxxxxxx</saml:Issuer>
    <saml:NameID Format='urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress'>[email protected]</saml:NameID>
    <samlp:SessionIndex>kbcC2VIbcv5rrMv.NlBeZGRZANW</samlp:SessionIndex>
</samlp:LogoutRequest>

i am getting LogoutResponse with

...
<samlp:Status><samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Requester"/>
        <samlp:StatusMessage>Invalid signature</samlp:StatusMessage>
    </samlp:Status>

[md5]

Thanks for testing @ADolhov. I don't personally have access to an IdP that readily allows me to test SLO, so it would be great to get one or more additional positive confirmations, including at least one person using ADFS who is affected by the bug described at https://github.com/onelogin/ruby-saml/pull/418

[md5]

Also, this PR effectively switches from using request.params to using request.GET and request.query_string, so it may need to be adjusted to deal with the HTTP POST binding (in which case it's unfortunate that the ruby-saml parameter is called raw_get_params). I'm not sure how common it is to use something other than the HTTP redirect binding for SLO.

[md5]

Actually, I guess this PR still uses request.params, so it's possible for SAMLRequest to be pulled from the POST body while it attempts to resolve the raw encoding params from the query string.

[ADolhov]

applied this fix via fork and it didnt help, still getting same invalid signature error :( also i dont get purpose of strong validation of LogoutResponse, as far as whenever LogoutRequest is successful, IdP invalidates current SSO session for matching SessionIndex/NameId regardless to LogoutResponse it sends

Once i applied soft validation for logout response it started working for me

p.s. seems we have to preserve saml_session_index as well as saml_uid in session, but README doesnt have any info about that

[md5]

@ADolhov we would welcome a patch for the README.

As for the value of checking the signature on logout payloads, I see your point. I think the main "attack" that someone could do with a fraudulent SloLogoutrequest would be a denial of service whereby they keep a user from staying logged in to a service.

[md5]

NOTE: This still needs to be fixed, but I decided it wasn't a blocker for upgrading ruby-saml since both the encoding issue and the lack of signature checking are issues regardless of whether we upgrade. We upgraded to ruby-saml 1.7.0 in https://github.com/omniauth/omniauth-saml/pull/157

[nwittstruck]

@md5 I just wanted to confirm that this branch works fine. We were affected by https://github.com/onelogin/ruby-saml/pull/418 - with this branch it's working as expected and the character encoding issues are gone. The only thing missing are saml_session_index and saml_uid. What's the recommended way to preserve them for IdP initiated SLO? I'm happy to update the readme when I get this working.

Just to clarify - why would the saml_session_index and saml_uid be in the session? The code tries to retrieve it from the session, however shouldn't it come from the SloLogoutRequest, since it's IdP initiated?

[nwittstruck]

I've spend a bit more time on this issue and was able to make it work with AzureAD. AzureAD directly sends a HTTP request to the application for IdP initiated sign out, it doesn't use the front-channel and doesn't redirect the user. This explains why the current code doesn't work - it assumes there is session information there, when it isn't.

To work around this, I've added a parameter to the idp_slo_session_destroy proc, so that it receives the name_id from the logout_request, when no saml_uid is present in the session. This allows me to properly handle the sign out in the proc, even when no session information is present.

I can submit a PR that makes it work for the back-channel communication, if you are interested? I wouldn't be surprised if I got this wrong and missed a configuration parameter in AzureAD that handles this, so I might be wrong here, since I'm not an AzureAD expert.

Edit: Here is a draft that's working with AD based on this PR.

[md5]

Closing this to get it off my list of open PRs and since I'm no longer a maintainer of this gem. Please reach out if you decide to revisit this work and need my help to dust it off, assuming it's still relevant.