fix: prevent CSRF cookie accumulation on auth expiry#99
Merged
thesuperzapper merged 1 commit intoFeb 7, 2024
Merged
Conversation
Signed-off-by: Mathew Wicks <[email protected]>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #96
This PR tries to ensure that background queries do not get redirected to dex authentication (which will cause a CSRF cookie to be added to the browser), as around the cookie expiry time, we are seeing 431 errors (too large headers) due to too many CSRF cookies.
This PR makes two changes:
Acceptheaders are actually passed to oauth2-proxy during ExtAuthZ:application/jsonrequests which are unauthenticated, and instead just responds with 401. (See oauth2-proxy behavior).--api-route/api_routespatterns for all paths that deployKF makes "background requests" to (but which don't useapplication/json), so aren't hit by the first change:/api/or/apis/hits most of them, with the only significant remaining one being/ml_metadatafrom KFP.NOTE: This solution is inherently brittle, it requires us to manually keep the
api_routesup to date, and users can still manually cause too many CSRF tokens (for example, by clicking around in Jupyter after your token expires).Therefore, we will also follow up PR with a phased approach to enable the oauth2-proxy "sign-in" page, which will mean that no CSRF cookie is added until a user clicks the "sign-in" button.
The "sign-in" page change will require two steps, because it will potentially break some "automatic login scripts", like the
KFPClientManagerfrom our own docs: