search

imsweb / django-saml-sp

A Django application for running one or more SAML service providers (SP)
BSD 3-Clause "New" or "Revised" License
15 stars 10 forks source link

SLO not working by default. #25

Closed noyessie closed 10 months ago

noyessie commented 1 year ago

Hi

I came across what I see as a bug. The logout endpoint does not pass the nameid to saml therefore preventing me to logout.

Here is the request sent:

<samlp:LogoutRequest
  xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
  xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
  ID="xxx"
  Version="2.0"
  IssueInstant="2023-05-05T20:20:12Z"
  Destination="https://sso.nyumc.org/oamfed/idp/samlv20">
    <saml:Issuer>https://127.0.0.1:8000/my/sso/localhosts/</saml:Issuer>
    <saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity"><url of the idp></saml:NameID>

</samlp:LogoutRequest>

As you can see, the library is sending the Idp url as nameid. That's not normal. Therefore the SLO feature is not working. To fix that, I had to change the code and pass the nameid and the name_id_format myself. But this is just a temporary fix because if the code is deployed elsewhere, those change made in the package are lost.

Here are the changes I made to the logout logout endpoint in sp.views.logout to this:

// ... other lines
// change the line : saml.logout(redir) to this line
redirect(saml.logout(redir ,name_id=get_session_nameid(request), name_id_format="urn:oasis:names:tc:SAML:2.0:nameid-format:kerberos"))
//... rest of the code

When I change the endpoint to this, everything is working well.

So I wonder why this is not the behavior by default ? Why are you not sending the nameid and the name_id_format of the current user by defaut ?

dcwatson commented 1 year ago

No particular reason other than I haven't needed it. SLO has been working for my uses - likely the IdP not needing it in order to kill the browser cookie/session. One question: is just passing name_id sufficient to make it work for you? Or do you need to specify both? I ask because it looks like the library should pull the NameIDFormat from the SP settings if not specified, which seems reasonable.

https://github.com/SAML-Toolkits/python3-saml/blob/24a8f095b06cb15cb43aac3abae2aefc183c73a4/src/onelogin/saml2/logout_request.py#L74-L76

noyessie commented 1 year ago

No particular reason other than I haven't needed it. SLO has been working for my uses - likely the IdP not needing it in order to kill the browser cookie/session. One question: is just passing name_id sufficient to make it work for you? Or do you need to specify both? I ask because it looks like the library should pull the NameIDFormat from the SP settings if not specified, which seems reasonable.

https://github.com/SAML-Toolkits/python3-saml/blob/24a8f095b06cb15cb43aac3abae2aefc183c73a4/src/onelogin/saml2/logout_request.py#L74-L76

I needed to pass both.

The NameIDFormat fetched by the library was incorrect. I expect this to be the same as the one coming from the IDP. But in my case, it was not the same. I have to manually override it.

dcwatson commented 10 months ago

I just pushed out 0.8.0 that sends nameid/nameid_format along with the logout request.