Repository navigation
Remove Keycloak access token from cookie to not exceed cookie size limit - #61773
exolightor wants to merge 3 commits into
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
SameerMesiah97
left a comment
There was a problem hiding this comment.
Looks good. This will result in an extra refresh request to Keycloak but the impact is minimal.
c9a9599 to
c6ba658
Compare
|
Thanks for the review @SameerMesiah97, I adjusted the test accordingly. CI Workflow needs to be approved again and it looks like it needs review from either @vincbeck or @bugraoz93. |
vincbeck
left a comment
There was a problem hiding this comment.
I really do not understand this change. And I do not think it solves anything. When the user is logging in you no longer save the access token (which is very much needed to call Keycloak API). Then, in the refresh token flow, you refresh the token and the token is saved there. So at the end, the token is still saved in the cookie but this time it takes an extra step to generate it.
The issue you are describing (having refresh token and access token in a same cookie exceed the limit of 4K) is real, and I had it on my list but I do not think you are fixing it properly. I think the fix should be to store the access token and refresh in separate cookies. Happy to provide more details if needed.
|
Hello @vincbeck, thanks for looking at the change. With my changes, the access token is not saved in the cookie even after the refresh token flow. After refresh token flow in Splitting access token and refresh token across two cookies does not solve the problem in the long run, as the access token alone can easily exceed 4KB. With 15+ realm roles it already should be big enough. Other open source applications (e.g. superset) solve this by storing the access token server-side instead of in the cookie. This is why I changed the code to only store the access token server side, .i.e., only in the I have tested it locally and deployed with helm on OpenShift and issue #61771 is only resolved with my change. Otherwise the login does not work. In our Keycloak realm access tokens can include 15+ realm roles. |
But at the end this is saved in a cookie. It has to be saved somewhere and we do not store anything on the backend. If you look at , the token generated saved inKeycloakAuthManagerUser is serialized in then saved in the cookie.
|
Ok, thanks for the pointer. I do find it quite odd then that in my case with my changes the access token fits into the cookie after refresh token flow but not after the initial login callback. I will investigate this further if I get the chance. I forgot to copy an additional |
|
I believe this error is very similar to what I reported in #59049 In the end, we solved it by configuring our nginx and haproxy to accept more then 4K |
|
@exolightor This PR has been converted to draft because it does not yet meet our Pull Request quality criteria. Issues found:
What to do next:
Converting a PR to draft is not a rejection — it is an invitation to bring the PR up to the project's standards so that maintainer review time is spent productively. If you have questions, feel free to ask on the Airflow Slack. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
Closes: #61771
Removed access token from cookie for auth code flow in Keycloak provider.
For large access tokens containing multiple realm roles, access token + refresh token exceeds browser cookie limit of 4KB. Passing only refresh token in cookie is sufficient, as with refresh_user() the access token can be retrieved via refresh token.