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

feat: stateless logout #3938

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: stateless logout #3938

wants to merge 1 commit into from

Conversation

alnr
Copy link
Collaborator

@alnr alnr commented Feb 6, 2025

Closes #3836

Completely eliminates database writes during logout, except session deletion. We can drop table hydra_oauth2_logout_request completely -- I didn't in this PR tho.

There now is no more reuse detection for logout verifiers. But double-submit use cases work as before, so I don't think there is a observable or at least no meaningful difference in behavior.

Deleted a bunch of tests, too 🥰 🔥

@alnr alnr requested a review from hperl February 6, 2025 18:13
@alnr alnr self-assigned this Feb 6, 2025
@alnr alnr requested review from aeneasr and a team as code owners February 6, 2025 18:13
@alnr alnr force-pushed the alnr/stateless-logout branch from f46ed94 to bc06ee5 Compare February 21, 2025 11:56
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Deletes a lot of code and DB = nice

Just a few comments / questions

return errorsx.WithStack(x.ErrNotFound)
} else {
return errorsx.WithStack(err)
_, err = flowctx.Decode[flow.LogoutRequest](ctx, p.r.FlowCipher(), challenge, flowctx.AsLogoutChallenge)
Copy link
Member

Choose a reason for hiding this comment

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

Rejection no longer works? This is a no-op and a breaking change unless rejection now works a different way.

Copy link
Collaborator Author

@alnr alnr Feb 26, 2025

Choose a reason for hiding this comment

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

No, the behavior is the same. If we don't recognize the logout challenge, we return x.ErrNotFound. If we do recognize it, we return nil.

Previously, on database errors, we would return 500. Those no longer appear of course.

Copy link
Member

Choose a reason for hiding this comment

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

Previously rejecting a log out flow looked like this:

  1. Create logout challenge in app that uses Google OIDC
  2. Redirect user to logout UI
  3. User needs to decide, did I actually want to log out of Google?
  4. User decides "no" i do not want to log out of google, presses no
  5. Logout challenge is marked as invalid and can no longer be used
  6. User is redirected to some endpoint

Is this flow still intact?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This flow is still intact, the only change being that we no longer mark the challenge as invalid. Since it's now never persisted, we have no way of marking it invalid after this change.

This could be added back (bringing back a single DB write when a logout is rejected). But I'm not sure we should do that.

Ultimately, just like in the auth code flow, the consent app is ultimately responsible for how user's journey will look.

Previously, when the consent app calls PUT /admin/oauth2/requests/logout/reject, we marked it in the DB as rejected and returned 204. The consent app then needs to figure out where to send the user.

Now, we still validate it and return 204. The consent app is still responsible for sending the user onwards.

I can't seem to be able to come up with a scenario where it would be important to mark the logout challenge as rejected. The user will be logged out only if the consent app directs Hydra to do so, even if someone re-used a previously rejected logout challenge. Generating a new challenge is "free" anyways 🤷

Does that make sense or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem now is that, as you say, we are not able to mark the challenge as invalid. Doesn't this mean, that I can re-request log out with a valid challenge over and over again (re-use)?

Copy link
Member

Choose a reason for hiding this comment

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

If there is no impact from re-use I think it's fine. I'm specifically wondering about front and backchannel logout and if this allows a client to re-request backchannel logout over and over again, potentially DoS'ing.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is more applicable to the logout verifier and if re-using that one can cause these type of issues, than it is to the challenge. The challenge is only solved if the user hits "yes". I think that's fine and we don't need a new challenge every time we present the screen (unless it expired).

@alnr alnr force-pushed the alnr/stateless-logout branch 2 times, most recently from 78e5e50 to bbdc516 Compare February 26, 2025 17:24
@alnr alnr force-pushed the alnr/stateless-logout branch from bbdc516 to 666b67f Compare February 26, 2025 19:05
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.

Make OP-initiated logout stateless
2 participants