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

Fix OIDC cookie related tenant id and chunk calculation issues #39850

Conversation

sberyozkin
Copy link
Member

Fixes #39849.
Fixes #39417.
Fixes #38535.

This PR fixes several issues related to the OIDC tenant resolution and the session management, all discovered while looking at the test failure as a result of the initial work with #39417.
As it happens, the initial #39417 fix exposed these problems, how the OIDC quickstart tenancy test was passing when the tenants are switched with the same session cookie path before is still a mystery to me.

So, this PR does the following:

  • Makes sure that if the tenant id has been changed after it was set by the session cookie, then, before the reauthentication, it removes now obsolete session cookie(s) belonging to the old tenant
  • Avoids calling OidcAuthenticationMechanism#resolve 3 times
  • Decreases the max session cookie value to 4056 bytes, otherwise the browsers will drop the session cookie chunks if the automatic session cookie splitting is enabled (it is by default)
  • Fixed the multi-tenant docs describing the correct sequence of the resolution
  • Replacing a totally inadequate section on how to manage web-app tenants with hopefully a more useful advice
  • Improved the existing tests, added new ones

This is a technical PR which tries to cover the grey areas related to the session cookie splitting, and provide the better advice on how to manage tenants

This comment has been minimized.

This comment has been minimized.

@sberyozkin sberyozkin force-pushed the remove_previous_oidc_tenant_session_cookie branch from 84477fa to 71861ab Compare April 3, 2024 11:31
@sberyozkin
Copy link
Member Author

Thanks @gastaldi, I added some doc text between 1., 2., 3. bullets so the doc build failed, replaced with === Step 1: ... for 3 steps.

@michalvavrik Please confirm when you get a chance that we are in agreement re the doc-ed order of tenant resolution :-)

A minor follow up optimization is also possible (OidcAuthenticationMechanism records a resolved OidcTenantConfig in the context, but so is DefaultTenantConfigResolver but using different keys for statically and dynamically resolved configs and I thought I'd rather consider looking into it separately to avoid overextending this PR)

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

LGTM

@sberyozkin
Copy link
Member Author

Thanks @michalvavrik

@pedroigor Hi Pedro, have a look if you get a chance, I'll keep it for another day, see the PR description, it is mainly about bug fixes related to the cookie max size (and the subsequent splits), making sure the old tenant's cookies are cleaned up if for some reasons and improving the multi-tenancy docs.

@sberyozkin sberyozkin force-pushed the remove_previous_oidc_tenant_session_cookie branch from 71861ab to 61529c2 Compare April 3, 2024 17:15
@sberyozkin
Copy link
Member Author

I've picked up the direct session cookie encryption update just to make sure it does not impact anything here (it must not)

Copy link

quarkus-bot bot commented Apr 3, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 61529c2.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Apr 3, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 61529c2.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@sberyozkin
Copy link
Member Author

I suppose I'll go ahead, I'm seeing some strange Keycloak 24.0.2 test errors, and this PR fixes a few session and tenant resolution issues, so may be it can help.

@sberyozkin sberyozkin merged commit 0e2c02f into quarkusio:main Apr 4, 2024
25 checks passed
@sberyozkin sberyozkin deleted the remove_previous_oidc_tenant_session_cookie branch April 4, 2024 13:54
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Apr 4, 2024
Copy link

github-actions bot commented Apr 4, 2024

🙈 The PR is closed and the preview is expired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment