Skip to content

fix: prevent CSRF cookie accumulation on auth expiry#99

Merged
thesuperzapper merged 1 commit into
deployKF:mainfrom
thesuperzapper:fix-csrf-cookie-accumulation
Feb 7, 2024
Merged

fix: prevent CSRF cookie accumulation on auth expiry#99
thesuperzapper merged 1 commit into
deployKF:mainfrom
thesuperzapper:fix-csrf-cookie-accumulation

Conversation

@thesuperzapper

@thesuperzapper thesuperzapper commented Feb 7, 2024

Copy link
Copy Markdown
Member

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:

  1. Ensures that Accept headers are actually passed to oauth2-proxy during ExtAuthZ:
    • This is because oauth2-proxy will not redirect application/json requests which are unauthenticated, and instead just responds with 401. (See oauth2-proxy behavior).
  2. Defines --api-route / api_routes patterns for all paths that deployKF makes "background requests" to (but which don't use application/json), so aren't hit by the first change:
    • As far as I can tell, just checking for the presence of /api/ or /apis/ hits most of them, with the only significant remaining one being /ml_metadata from KFP.

NOTE: This solution is inherently brittle, it requires us to manually keep the api_routes up 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 KFPClientManager from our own docs:

  1. PHASE 1: Make enabling the sign-in page an optional value:
    • Disabled by default
    • Warn users to test it with their scripts
  2. PHASE 2: Enable sign-in page by default.

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.

HTTP ERROR 431 reauthentication error

1 participant