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

Optional support for the OIDC session cookie dir encryption #37816

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

sberyozkin
Copy link
Member

Fixes #37785.

This PR adds an option to customize the way the session cookie is encrypted - it will continue using the current mode by default where a content encryption key (CEK) is generated (with more algorithms added as required - I'm not too keen to open it up completely to support all of them) - this mode is strongest but also produces a longer sequence and is a bit slower.

Direct dir encryption is a standard JWE option where the configured encryption key acts as CEK.

FYI, difference in the cookie size, in the 2 updated tests is 2919 (dir) vs 3043 (CEK is generated and encoded) so it is about 120 bytes which might make a difference.

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member Author

Hi Pedro @pedroigor, after looking at the issues related to the session token management, I think it makes sense to go ahead with this option, as it looks like that the OIDC session cookies, nearly all of them, are close to 4K by default, so having an option to shorten the cipher text, without having to split the session cookie length is useful.
I'll resolve the conflict and will ping you to discuss

@sberyozkin
Copy link
Member Author

@pedroigor The reason it will make a difference is that I've noticed that with the max cookie value set to 4048 is not enough to avoid the split which is not great, so I've tuned it to 4056 for now, but with this option, decreasing the cookie size by 120 bytes on average will make the split unnecessary in many cases

Copy link
Contributor

@pedroigor pedroigor left a comment

Choose a reason for hiding this comment

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

LGTM

@quarkus-bot
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 15d61a5.

✅ 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

Thanks @pedroigor, it should help Keycloak users in particular and those work work with providers like Azure which can produce massive ID tokens, to avoid splitting the cookies. I'll consider making it a default option if a strong encryption key is available, but for now the CEK will be generated which produces varying sequences for the same text.

@sberyozkin sberyozkin merged commit 07c9712 into quarkusio:main Apr 3, 2024
22 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Apr 3, 2024
@sberyozkin sberyozkin deleted the oidc_session_dir_encrypton branch April 3, 2024 16:09
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using direct JWE encryption of the OIDC session cookie
2 participants