resolve logout token subject:sessions for the idp backchannel logout

[dragonchaser]

Description

the backchannel logout only considers the logout token sessionId, as mentioned by the spec,
the sessionId is optional if the subject exists.

The current implementation ignores the subject and fails if the sessionId is not part of the logoutToken.
A more detailed description of the problem and how to reproduce it can be found here.

Related Issue

• part of https://github.com/opencloud-eu/internal/issues/246

Motivation and Context

from the spec:

A Logout Token MUST contain either a sub or a sid Claim, and MAY contain both. If a sid Claim is not present, the intent is that all sessions at the RP for the End-User identified by the iss and sub Claims be logged out.

Known side-effects

• keyCloak "Sign out all active sessions" does not send a backchannel logout request,
  as the devs mention, this may lead to thousands of backchannel logout requests,
  therefore, they recommend a short token lifetime.
  OIDC: Backchannel logout not being called when using Sign out all Session in Keycloak  keycloak/keycloak#27342 (comment)

• keyCloak user self-service portal, "Sign out all devices" may not send a backchannel
  logout request for each session, it's not mentionex explicitly,
  but maybe the reason for that is the same as for "Sign out all active sessions"
  to prevent a flood of backchannel logout requests.

• if the keyCloak setting "Backchannel logout session required" is disabled (or the token has no session id),
  we resolve the session by the subject which can lead to multiple session records (subject.*),
  we then send a logout event (sse) to each connected client and delete our stored cache record (subject.session & claim).
  all sessions besides the one that triggered the backchannel logout continue to exist in the identity provider,
  so the user will not be fully logged out until all sessions are logged out or expired.
  this leads to the situation that web renders the logout view even if the instance is not fully logged out yet.

Things to pay special attention to during the review

• services/proxy/pkg/middleware/oidc_auth.go
  • format of the cache key
    • {key: ".sessionId"} => used for *.sessionId
    • {key: "subject.sessionId"} => used for subject.*
• services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go
  • suffix key matching and lookup
  • prefix key matching and lookup
  • suspicious query detection
• services/proxy/pkg/staticroutes/backchannellogout.go
  • error handling (all cases and return values are ok?)

ToDos:

• the spec mentions by the iss and sub Claims ..., we don't do that?
• we use the memory cache store by default, should we switch it to nats?

Questions

• why don't we just store the claim records in the cache and use it for everything?

How Has This Been Tested?

• unit tests
• local keyCloak installation & multiple clients & multiple cache stores (memory, nats-kv)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation added

[fschade]

toDo:

• using nats kv by default is ok?
• what should we do if the token contains a session id and a subject
• check comment toDos

[fschade]

A few minor things here and there, but it should be fine for now.

logout.mp4

[fschade]

i updated the issue description, hope it makes sense

[rhafer]

I am not quite sure this PR is actually fixing the bug. If we receive a logout with just a sub claim. We still need to invalidate all cached userinfo claims for that user, don't we?

(We might to be able to trigger the Logout Event though)

[rhafer]

What is the behavior if the logout token contains both, a session and a subject?

Currently we just seem to account for the sesssion and basically ignore the subject? Maybe that's fine the spec isn't really clear about that.

[fschade]

if the session is given, we use only the session and ignore the subject... wich results in 1 logout... any better idea?

[fschade]

I am not quite sure this PR is actually fixing the bug. If we receive a logout with just a sub claim. We still need to invalidate all cached userinfo claims for that user, don't we?

(We might to be able to trigger the Logout Event though)

thanks for your detailed review, you found some good flaws, yes if only a subkect is given, we invalidate all records that belong to that subject.

[sonarqubecloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.4% Duplication on New Code

See analysis details on SonarQube Cloud