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

SLO support using back channel #60

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

egroeper
Copy link
Contributor

@egroeper egroeper commented Oct 9, 2017

For our django project I implemented SLO support using back channel as an addition to this nice application.

Are you interested in including this in the upstream project or is this considered out of scope?

This introduces a dependency on spyne (and lxml) and needs an additional database table for mapping shibboleth sessions to django sessions (since we can't access the session of the user using the back channel).
Perhaps I could make back channel SLO and the dependencies belonging to it a configurable / optional feature?

I currently tested this on the test server of our service (using shibboleth-sp2 version 2.5.3 of Ubuntu 16.04), since I needed / wanted an IdP server to interact with.
Unfortunately there is no release of spyne, that is working with a recent Django, but with current master (b3bb2571d), it works fine.

This currently lacks tests and I'm not sure how / if that can be done.
Perhaps it is possible to simulate the IdP soap request by using the django.test.TestCase.client.

If you don't think this is generally out of scope, please review it and let's discuss what needs to be done to get this merged.

@egroeper
Copy link
Contributor Author

I'm currently working on implementing a test, which, like the tests in #61, works without the LOGOUT_SESSION_KEY stuff.
To do this I simply start a separate client instance for the SOAP request.

@bcail
Copy link
Contributor

bcail commented Oct 13, 2017

@egroeper thanks for your work. I think this work is in-scope for this package. Here are some thoughts:

  1. I think it would be great to have this feature be optional.
  2. I'd rather depend on a released version of spyne.

@egroeper
Copy link
Contributor Author

1. I think it would be great to have this feature be optional.

I will try that.

2. I'd rather depend on a released version of spyne.

I'm with you. I first tried with the latest release, but it didn't work out. Using the current master worked. By just looking through the commit history of spyne, I currently can't see a reason for that. I will reinvestigate.

@egroeper egroeper force-pushed the feature/slo_backchannel branch from a150875 to 381c4e8 Compare October 13, 2017 17:33
@egroeper
Copy link
Contributor Author

This now works with spyne 2.12.14

@egroeper egroeper force-pushed the feature/slo_backchannel branch from 381c4e8 to c885a56 Compare October 13, 2017 19:40
@egroeper
Copy link
Contributor Author

Made this feature optional and added some tests. Should be okay now.

Anything else to do?

@egroeper egroeper force-pushed the feature/slo_backchannel branch from 1f75792 to a51e433 Compare October 15, 2017 18:25
@egroeper
Copy link
Contributor Author

I now improved / finished the SOAP modelling and reactivated xml input validation.

Some documentation would be nice. Should I try to extend the Optional section of the README or do you think this would better fit in a separate file?

@bcail
Copy link
Contributor

bcail commented Oct 16, 2017

@egroeper Either way is fine with the documentation. You could try extending the Optional section of the README, and then move it to a new file if it gets too long.

@egroeper
Copy link
Contributor Author

I think we may have a problem here with Django <1.10 and first-request logins.
If session.session_key is None, then (in Django <1.10) on login the session gets created (persisted in database) and after that gets deleted immediately. Because of that inserting the SLO session mapping fails (foreign key constraint).
Seems like we can't test this with the django test framework. I will investigate further.
Currently I see the issue in Seafile, when the clients, which don't support cookie persistence, try to login with Shibboleth. But the same could be true for first-request logins.

I like the idea of using a foreign key for the session mapping table. That way we don't have to care about cleaning up our table. But this comes with the drawbacks, that we rely on the usage of the db SessionStore (which is quite common) and that the session needs to be already persisted in the database when our middleware runs.
Currently I would mention that (need for db SessionStore) in the docs and stay with using a foreign key. But of course we could simply make an independent table (giving us the burden to think about table cleanup).

The real work still needs to be done:
* store mapping shib session -> django session on login
* delete django user session on logout notifier call
In Django dashes in headers are replaced by underscores.
We have to respect that. Otherwise we will get a KeyError (as I did at
first in my test env).
See https://stackoverflow.com/a/24355709/1381638
My code is based on the example code in the master branch.
There ServiceBase was exchanged with Service.
Let's support both variants.
Backchannel SLO introduces new dependencies and is perhaps not needed /
wanted by everyone.
Disable it by default.
If the user directly accesses the shibboleth login path (session_key is
None) on Django version < 1.10, in auth.login the session will be persisted
in the database and get deleted again immediately.
This causes the insertion of the slo session mapping to fail (foreign
key constraint).
As a workaround we give him a session to delete.
@egroeper egroeper force-pushed the feature/slo_backchannel branch from 44b89e7 to 509bd91 Compare October 22, 2017 19:59
@egroeper
Copy link
Contributor Author

Seems like we can't test this with the django test framework. I will investigate further.
Currently I see the issue in Seafile, when the clients, which don't support cookie persistence, try to login with Shibboleth. But the same could be true for first-request logins.

I could now reproduce the issue using Chromium webbrowser, when calling the shibboleth login endpoint of my webapp directly (before that I deleted all site cookies and disabled the embedded discovery service).
509bd91 should fix the issue.



# store session mapping
ShibSession.objects.get_or_create(shib=request.META['Shib_Session_ID'], session_id=request.session.session_key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@egroeper can you make this DB call only happen if the SLO stuff is enabled in app_settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. It's really better to guard this as well.

@egroeper egroeper force-pushed the feature/slo_backchannel branch from ae146e6 to 9fc537a Compare October 23, 2017 22:21
@egroeper egroeper force-pushed the feature/slo_backchannel branch from 9fc537a to a55614b Compare October 23, 2017 22:24
@egroeper
Copy link
Contributor Author

Please don't merge this, yet. I'm still investigating an issue when both front-channel and back-channel logout are in place.

@egroeper
Copy link
Contributor Author

For back-channel SLO to work you need to configure service notification in the shibboleth sp.
But if this is in place, it will be used whether the logout was initiated by the IdP or through visiting /Shibboleth.sso/Logout. So if we redirect to that endpoint for SLO initiated from the django app, a SOAP call back to our webapp is made and it is tried to log the user out another time, which fails for obvious reasons.

One possible solution would be to not do the user logout (auht.logout()) in the ShibbolethLogoutView if back-channel SLO is configured, but only redirect to the SLO endpoint. Logout is then done through back-channel.

Here a new question arises: Do we want to support django project with additional authentication backends beside Shibboleth?
In that case we should only redirect the shibboleth sessions to the SLO endpoint and, of course, call auth.logout() in all other cases.
Perhaps it would then be better to have an own SHIBBOLETH_SLO_REDIRECT variable?

@bcail
Copy link
Contributor

bcail commented Oct 24, 2017

Does this package currently work with other authentication backends? If not, seems like we wouldn't need to worry about it for this feature.

@egroeper
Copy link
Contributor Author

Yes. I'm pretty sure, that it is currently possible to combine local (database) accounts, Shibboleth authentication and even other backends like LDAP.
Introducing SLO complicates things, when it comes to other authentication backends.
That's because we then need additional steps or even need to omit steps on logout.

I will try to find a solution and you can review the patch and we discuss, if something and what should be changed.

Base automatically changed from master to main January 14, 2021 16:28
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

Successfully merging this pull request may close these issues.

2 participants