-
Notifications
You must be signed in to change notification settings - Fork 174
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
Cookie compression and splitting for JWT #1651
Cookie compression and splitting for JWT #1651
Conversation
Sorry for the review requests all - I meant to submit this as a draft. The draft status should be correct now. |
0966b27
to
b6c6c0a
Compare
034db8c
to
ce5b30e
Compare
Signed-off-by: Jochen Kressin <[email protected]>
Signed-off-by: Jochen Kressin <[email protected]>
Signed-off-by: Jochen Kressin <[email protected]>
Signed-off-by: Jochen Kressin <[email protected]>
Signed-off-by: Jochen Kressin <[email protected]>
Signed-off-by: Jochen Kressin <[email protected]>
Signed-off-by: Jochen Kressin <[email protected]>
Signed-off-by: Jochen Kressin <[email protected]>
…mports Signed-off-by: Jochen Kressin <[email protected]>
ce5b30e
to
0f39953
Compare
Signed-off-by: Jochen Kressin <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1651 +/- ##
=======================================
Coverage 67.09% 67.09%
=======================================
Files 94 94
Lines 2404 2404
Branches 318 318
=======================================
Hits 1613 1613
Misses 713 713
Partials 78 78 ☔ View full report in Codecov by Sentry. |
Another thing I found while testing this was that the logout doesn't work for JWT. |
Hi @jochen-kressin, for the logout issue--do you think you could open an issue or separate PR to fix it? Thank you! |
Thank you for this PR @jochen-kressin! While I was reviewing this issue I noticed it happened because the token was too large. This PR should fix that. |
@cwperks Will do! While testing I also noticed that the SAML logout throws an error. Will check again if I configured something wrong, otherwise I'll add an issue for that as well. |
Signed-off-by: Jochen Kressin <[email protected]>
While adding the logout issue, I realized I had forgotten something related to this issue. I added #1710 for the logout issue. I considered fixing the issue within this PR, but then thought it best to keep it separate so that we can get this PR merged. The SAML logout issue I mentioned: I believe that was a misconfiguration on my part, so I'm not adding an issue for that. |
Signed-off-by: Jochen Kressin <[email protected]>
Signed-off-by: Jochen Kressin <[email protected]> Co-authored-by: Craig Perkins <[email protected]> (cherry picked from commit 7cad47c)
Signed-off-by: Jochen Kressin <[email protected]> Co-authored-by: Craig Perkins <[email protected]> (cherry picked from commit 7cad47c) Co-authored-by: Jochen Kressin <[email protected]>
Description
This PR will port the cookie compression and splitting behaviour as implemented already for OIDC and SAML to also be used for the JWT authentication type.
This is done to support token payloads that are too large to store in a single cookie.
Category
Enhancement
Why these changes are required?
When the user authenticates with a JWT, the token is stored in a cookie. However, if the JWT is too large to store in a single cookie, the authentication will fail. To remedy this, we implemented the same approach as we've previously done for SAML and OpenId: compression and cookie splitting.
What is the old behavior before changes and new behavior after changes?
The old behaviour was that large JWTs would cause the authentication to fail. Now, we support larger tokens by splitting it up in multiple cookies, the number of which can be defined in the configuration.
For end users that are already authenticated, existing cookies should continue to work.
Issues Resolved
#1638
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Manual testing:
I've tested with large JWTs, and I've tested that having an existing cookie still works after the splitting was implemented.
I did start testing multi auth, but then quickly noticed that JWT isn't supported in multi auth
Unit tests:
I had to edit the existing JWT unit tests a bit. I did not change any logic, but only extended certain "mocks" with properties required by the new implementation.
Integration tests:
Since there already were Selenium tests for JWT, I stuck with that and added a test with a JWT that is too large for one cookie.
Additional notes
Even though we now support large tokens in the cookies, the user may still run into a problem with a "Too long http header exception".
This can however be configured in opensearch.yml, e.g. with
http.max_header_size: 16kb
I have not investigated if this change has any negative side effects.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.