Support OIDC backchannel logouts

[sandhose]

Fixes #11326

Based on #11482.

Tested (and works) with Keycloak.

How to test this?

• Spin up a Keycloak instance, create a realm
• Configure Synapse to login via Keycloak: https://matrix-org.github.io/synapse/latest/openid.html#keycloak
• In Keycloak, on the client used by Synapse, add the following backchannel logout URL: {public_url}/_synapse/client/oidc/backchannel_logout
• Ensure Backchannel Logout Session Required is enabled on that client
• Login via OIDC in any Matrix client
• Log the user out of Keycloak by going to {keycloak}/auth/realms/{realms}/account/#/ (which is the self-service console) and click "Sign Out"
• The user should be logged out of Synapse

There is a Docker Compose stack here to help with testing: https://github.com/sandhose/synapse-keycloak-devstack

Additionally, there are two edge cases which can be tested (and which are covered by tests):

• when the logout happens before the client sends the login token (e.g. happens during the "Continuing will grant app.element.io access to your account." screen)
• when the logout happens during the username mapping session, so when the localpart_template is not set and the user chooses a username themselves

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[clokep]

Removing this from the review queue until #11482 gets worked out.

[clokep]

Looks pretty sensible, I left a bunch of comments but I think most are minor!

[clokep]

It makes me slightly sad to add another configuration option here, but I don't think there's much we can do here to avoid it?

The metadata seems to include whether this feature is supported or not. Can we auto-enable it in those cases?

[sandhose]

I mean, we could probably completely skip the configuration parameter. The only real benefit right now are warnings when the config enables backchannel logouts, but the OP does not advertise proper support.

Right now the behaviour is to delete the device, which does a "hard" logout. From a client perspective, it means it won't keep E2EE data, so you can't get back the deleted device. Maybe that's not the right behaviour, maybe that should be configurable.

Would it make sense to have instead a config parameter backchannel_logout_behaviour with the following values: logout (the default), soft_logout, nothing, and maybe implement soft_logout/other behaviours later?

[clokep]

Right now the behaviour is to delete the device, which does a "hard" logout. From a client perspective, it means it won't keep E2EE data, so you can't get back the deleted device. Maybe that's not the right behaviour, maybe that should be configurable.

Would it make sense to have instead a config parameter backchannel_logout_behaviour with the following values: logout (the default), soft_logout, nothing, and maybe implement soft_logout/other behaviours later?

I don't think doing a hard logout makes sense, actually. It likely makes more sense to do a soft-logout? I'm not really sure what a backchannel-logout is supposed to represent, but logging out of the IdP causing Synapse to destroy a session, causing a user to lose decryption keys sounds not great.

[sandhose]

I haven't really found a good way to soft-logout a user :(
It looks like soft-logouts are only sent when the token expired.

but logging out of the IdP causing Synapse to destroy a session, causing a user to lose decryption keys sounds not great.

I agree, but on the other hand leaving 'dead' devices on the account doesn't seem great either. Would you be fine with leaving this behaviour for now, see how it feels with whatever customer is asking for this change, and iterate on it later if it makes more sense to do a soft-logout?

[clokep]

I think this is a little more readable?

Suggested change

	This extracts the logout_token from the request and try to figure out
	from whom it is coming from. This works by matching the iss claim with
	the issuer and the aud claim with the client_id.
	This extracts the logout_token from the request and tries to figure out
	which account it is associated with. This works by matching the iss claim
	with the issuer and the aud claim with the client_id.

[sandhose]

Ha, I did not properly read your suggestion. It's not "which account it is associated with" but rather "from which IdP it is coming from"

[clokep]

Isn't this matching to a particular user though, not just the IdP?

[sandhose]

This method specifically matches the IDP, and then passes down to the OidcProvider#handle_backchannel_logout, which then finds the user

[sandhose]

Thanks for the review @clokep! I replied to most comments and will fix some things now

I'd like your thoughts on the caveat I highlighted in the PR message:

One caveat (not sure how to easily fix this): in the following scenario, the user is still logged in even if the OIDC session ended:

• start the OIDC login from an "untrusted" client
• when on the "consent" screen served by Synapse (Continuing will grant http://XXXX/ access to your account.), do not click continue yet
• instead, log out from Keycloak
• click on "continue" on the consent screen: the user is successfully logged in, even if the session ended in Keycloak

[sandhose]

I mean, we could probably completely skip the configuration parameter. The only real benefit right now are warnings when the config enables backchannel logouts, but the OP does not advertise proper support.

Right now the behaviour is to delete the device, which does a "hard" logout. From a client perspective, it means it won't keep E2EE data, so you can't get back the deleted device. Maybe that's not the right behaviour, maybe that should be configurable.

Would it make sense to have instead a config parameter backchannel_logout_behaviour with the following values: logout (the default), soft_logout, nothing, and maybe implement soft_logout/other behaviours later?

[clokep]

One caveat (not sure how to easily fix this): in the following scenario, the user is still logged in even if the OIDC session ended:

• start the OIDC login from an "untrusted" client
• when on the "consent" screen served by Synapse (Continuing will grant http://XXXX/ access to your account.), do not click continue yet
• instead, log out from Keycloak
• click on "continue" on the consent screen: the user is successfully logged in, even if the session ended in Keycloak

This seems like it is probably OK? Shouldn't keycloak be rejecting this if the session ended?

[msitruk]

Hello any news ?
Is there any other way to resolv this lack of working logout mecanism for oidc ... waiting this PR ?

[reivilibre]

What's the thinking on the state of this PR? Is it still wanted; does it need more review; is there something to be done to it?

[sandhose]

I'm getting back to the PR. I rebased on top of develop and applied a few fixes.

Still on my todo list is:

• add tests
• add documentation
• figure out what to do with the potential login/logout "race condition"
• merge some of the JWT handling code of the ID token and the logout token

[sandhose]

I haven't really found a good way to soft-logout a user :(
It looks like soft-logouts are only sent when the token expired.

but logging out of the IdP causing Synapse to destroy a session, causing a user to lose decryption keys sounds not great.

I agree, but on the other hand leaving 'dead' devices on the account doesn't seem great either. Would you be fine with leaving this behaviour for now, see how it feels with whatever customer is asking for this change, and iterate on it later if it makes more sense to do a soft-logout?

[sandhose]

This method specifically matches the IDP, and then passes down to the OidcProvider#handle_backchannel_logout, which then finds the user

[sandhose]

It should be ready to review! Now that #13844 is merged, I was able to revoke login tokens in-flight (there is a test for that), and now that #13910 is merged, I was able to write proper unit tests for everything.

It just lacks documentation, which I can add later today, or in a future PR. At least it is ready to review code-wise

[sandhose]

I spent way too much time figuring out why the tests were failing with the olddeps. Turns out, there was a bug with Twisted<20.3.0 where request.args was not filled on url encoded form bodies if the Content-Length was missing.

It might be this patch, but I'm not 100% sure: twisted/twisted#9678

I don't think it's worth fixing on the handler side, since most HTTP clients fill this header, unless you're streaming the body, which will probably not be the case for OIDC Backchannel logout notifications.

I fixed the tests by filling the Content-Length header in tests.server.make_request, see 55076e1

[clokep]

What's this change for? Is it ensuring JsonWebToken doesn't explode?

[sandhose]

we were importing authlib.jose.jwt before and it's not the case anymore. I think I did this because it was not available in all versions of authlib?

[clokep]

🤦 I failed to see where jwt was being used...

It looks like jwt has existed to be imported since at least 0.15?

I don't have a strong opinion about this though.

[clokep]

@sandhose Overall this looks reasonable. I'm sure some of the questions I asked were answered previously during our last round of reviews, but I'm afraid those have been lost to time. 😢 Sorry about that!

[clokep]

Looks good if tests pass! 🎉