fix(auth): harden OIDC login flow and redirect handling - #6994
fix(auth): harden OIDC login flow and redirect handling#6994snehasaisneha wants to merge 11 commits into
Conversation
claude
Bot
added
website
snehasaisneha
added
preview
snehasaisneha
changed the title
|
Claude finished @snehasaisneha's task in 5m 9s —— View job Review completeTodo list
SummaryThis is a well-designed and carefully implemented hardening of the OIDC login flow. I focused review effort on the security-critical paths: Security fundamentals check out:
Two minor, actionable points left as inline comments:
No blocking issues found. Nice work — the documentation ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude - changes have been pushed to resolve your previous comments. Please re-review. |
|
Claude finished @snehasaisneha's task in 51s —— View job Re-review completeTodo list
SummaryBoth points from the previous review have been fully resolved in
No other files changed outside these two fixes (verified via |
|
|
||
| The browser is redirected to Keycloak to enter credentials. Keycloak then returns an authorization code to the fixed `/auth/callback` endpoint on the Loculus website. Astro processes that callback on the server before the final page is rendered. | ||
|
|
||
| The requested destination is stored as `returnTo`. Loculus accepts only destinations on the same origin as the website, preventing the login flow from being used to redirect a user to an arbitrary external site. |
There was a problem hiding this comment.
Loculus accepts only destinations on the same origin as the website, preventing the login flow from being used to redirect a user to an arbitrary external site.
just for my understanding which part of the flow does this?
|
|
||
| - **State** is a random, single-use value that links the callback to the browser that started the login. It protects the login flow against cross-site request forgery. | ||
| - **Nonce** is a random value included in the authentication request and checked in the returned identity token. It prevents a response from another login transaction from being accepted. | ||
| - **PKCE** creates a one-time secret verifier and sends only its derived challenge with the initial request. The verifier is required when the website exchanges the authorization code, so an intercepted code is not sufficient to complete the login. |
There was a problem hiding this comment.
Is it currently actually possible to use the intercepted code? I think the malicious user would need access to the website client_secret which astro doesnt expose (not against using PKCE as it is good practice just trying to understand our setup)
|
|
||
| Loculus stores these values, together with `returnTo`, in an authenticated and encrypted HTTP-only cookie. The cookie: | ||
|
|
||
| - is valid for five minutes; |
There was a problem hiding this comment.
Not a huge deal, but this seems like bad UX (if I understand correctly). I may hit "Login", then I get a buzz on my phone and spend 5 mins sending a whatsapp message, then I fill in the login form and I'll presumably get an error message? I'd make this an hour or more
There was a problem hiding this comment.
In conversation with claude it pointed out that for the registration path - involving an email verification - this is especially short
There was a problem hiding this comment.
Indeed, I think the current UX is not great. If staying at the login screen for more than 5 minutes, one gets afterwards:
I think that the time should be extended. If one exceeds the time, one should get redirected back to the website with a useful message (like "Your session has expired, please click on login again.") - although what I find confusing here is that the authentication, at that point, will have been successfully performed by Keycloak already, so if one clicks on login, one gets directly logged in without needing to enter any credentials, that seems confusing and it's not clear to me how much security the time limt adds.
theosanderson
left a comment
theosanderson
left a comment
There was a problem hiding this comment.
The exact logout redirect restriction exposes a compatibility issue with the website’s current logout URL construction.
| ], | ||
| "attributes": { | ||
| "pkce.code.challenge.method": "S256", | ||
| "post.logout.redirect.uris": "https://{{$.Values.host}}/logout{{ if $.Values.insecureCookies }}##http://{{$.Values.host}}/logout##http://localhost:3000/logout{{ end }}" |
There was a problem hiding this comment.
[below comment is from Codex]
The exact allowlist here is incompatible with how UserPage.astro currently constructs post_logout_redirect_uri: it copies Astro.request.url and only replaces .pathname, so any query string on the account page is preserved. For example, visiting /user?foo=bar produces https://<host>/logout?foo=bar, which does not match this exact https://<host>/logout entry and Keycloak will reject the redirect.
Could we construct the logout URL from the origin instead (for example, new URL(routes.logout(), Astro.url.origin)) or explicitly clear search and hash before passing it to endSessionUrl? A regression test using a query-bearing /user URL would also be useful.
There was a problem hiding this comment.
To clarify what "Could we construct the logout URL from the origin instead" means: do you mean adapting UserPage.astro to not attach any ?foo=bar? Or do you mean adapting the keycloak config map here? (Do we have any use case for /logout?foo=bar?)
There was a problem hiding this comment.
Yes I think Codex means "adapting UserPage.astro to not attach any ?foo=bar"
Afaik, we don't have a use case for /logout?foo=bar
| secretKeyRef: | ||
| name: backend-keycloak-client-secret | ||
| key: backendKeycloakClientSecret | ||
| {{- if eq .name "loculus-website-config" }} |
There was a problem hiding this comment.
The conditionality here is a different pattern to what's used for everything else - everything else is also only relevant in certain situations but we just include everything. I can see some argument for conditionality to minimise exposure - (although I think practically it's OK, since the secrets are contained to the config processor) - but if we wanted to change this I'd leave it until a future issue that changes all of them as otherwise I was left asking why this special one needs to be wrapped. (But nbd of course)
There was a problem hiding this comment.
The whole pattern of putting secrets into config files should be avoided when we can. It turns out there's a new Astro feature which means we could: https://github.com/loculus-project/loculus/pull/7055/changes
|
|
||
| logger.debug(`Redirecting to auth with redirect url: ${redirectUrl}`); | ||
| const authUrl = await getAuthUrl(redirectUrl); | ||
| const authUrl = getLoginUrl(redirectUrl); |
There was a problem hiding this comment.
this was identified by claude but I verified it https://state-nonce.loculus.org/user gives 500 when not logged in
getLoginUrl returns a relative path (/auth/login?returnTo=…), but authUrl flows into createRedirectWithModifiableHeaders, which calls Response.redirect() — and that throws on a relative URL. The old getAuthUrl returned an absolute Keycloak URL, which is why the comment removed from it read // Beware: relative url does not work with Redirect.response(). That warning went away along with the behaviour that satisfied it.
Driving the middleware directly with a logged-out context for /user:
TypeError: Invalid URL
❯ Proxy.redirect .../Response.ts:392:14
❯ createRedirectWithModifiableHeaders src/middleware/authMiddleware.ts:308:31
❯ redirectToAuth src/middleware/authMiddleware.ts:320:12
❯ Module.<anonymous> src/middleware/authMiddleware.ts:116:16
This hits every enforced-login route — /user, /<organism>/user, /<organism>/my_sequences — so a logged-out visit, or a session expiring while a user sits on "My sequences", returns a 500 instead of redirecting to login.
One-line fix:
| const authUrl = getLoginUrl(redirectUrl); | |
| const authUrl = new URL(getLoginUrl(redirectUrl), context.url.origin).toString(); |
The e2e suite misses this because AuthPage.login() always starts from / and clicks the nav Login link, which goes to /auth/login directly; the only test that visits /user (integration-tests/tests/pages/group.page.ts:89) is already authenticated. Worth adding coverage for the logged-out case.
| "redirectUris": [ | ||
| "https://{{$.Values.host}}/*", | ||
| "http://{{$.Values.host}}/*", | ||
| "http://localhost:3000/*" | ||
| ] | ||
| "https://{{$.Values.host}}/auth/callback"{{ if $.Values.insecureCookies }}, | ||
| "http://{{$.Values.host}}/auth/callback", | ||
| "http://localhost:3000/auth/callback"{{ end }} | ||
| ], | ||
| "attributes": { | ||
| "pkce.code.challenge.method": "S256", | ||
| "post.logout.redirect.uris": "https://{{$.Values.host}}/logout{{ if $.Values.insecureCookies }}##http://{{$.Values.host}}/logout##http://localhost:3000/logout{{ end }}" | ||
| } |
There was a problem hiding this comment.
Drafted with the help of Claude.
Worth calling out in the PR description: the realm config map is only ever applied when the realm doesn't yet exist, so none of these Keycloak-side changes reach an instance that has already been deployed.
keycloak-deployment.yaml:113 starts Keycloak with --import-realm, which reads /opt/keycloak/data/import/ at boot but skips any realm already present in the database. That applies to the whole loculus realm definition, not just this client block — every change made to keycloak-config-map.yaml since a given instance's first boot is inert on that instance. This PR is just the first time that has security consequences. (The valuesHash pod annotation doesn't change it: it's gated on runDevelopmentKeycloakDatabase && !developmentDatabasePersistence, so it only covers throwaway dev databases, and a restart wouldn't re-import anyway.)
Concretely, on an existing instance backend-client keeps https://<host>/* and http://<host>/*, and gains neither pkce.code.challenge.method nor the post-logout allowlist.
Nothing breaks, which is what makes this easy to miss. The old wildcard still covers /auth/callback, and Keycloak honours a code_challenge even when the client doesn't mandate one, so state, nonce and PKCE all keep working end to end. What's silently absent is the server-side half — the narrowed redirect-URI allowlist and the exact post-logout destination, which is the part that addresses security-issues#20. Someone reading the PR description would reasonably assume that upgrading applies it.
Rather than adding an import-strategy override or a partial-import job to this PR, I'd suggest noting the caveat in the description and adding a short section admins can follow if they want the restriction on an existing realm. for-administrators/user-administration.md already covers reaching the admin console and selecting the loculus realm, so it could pick up from there.
E.g.
Per-client, via the admin console — Clients → backend-client:
- Settings → Valid redirect URIs: replace the wildcards with
https://<host>/auth/callback - Settings → Valid post logout redirect URIs:
https://<host>/logout - Advanced → Proof Key for Code Exchange Code Challenge Method:
S256
| - **SILO(s):** [SILO](https://github.com/GenSpectrum/LAPIS-SILO) is an open-source query engine for genetic sequences optimized for high performance and supporting alignment-specific queries such as mutation searches. It regularly pulls data from the backend server and indexes them. By default, SILO is not exposed to the users but accessed via LAPIS. For each [organism](../../reference/glossary#organism) of a Loculus [instance](../../reference/glossary#instance), there is a separate instance of SILO. | ||
| - **LAPIS(es):** [LAPIS](https://github.com/GenSpectrum/LAPIS) provides a convenient interface to SILO, offering a lightweight web API and additional data and compression formats. For each SILO instance, there is a corresponding LAPIS instance. | ||
| - **Website:** The frontend application of Loculus accesses the APIs of the backend server and LAPIS. It uses the backend server for everything related to data submission and LAPIS for searching and downloading released data. For logins and registrations, users are redirected to Keycloak. | ||
| - **Website:** The frontend application of Loculus accesses the APIs of the backend server and LAPIS. It uses the backend server for everything related to data submission and LAPIS for searching and downloading released data. For browser login and registration, the website starts an OpenID Connect transaction and redirects the user to Keycloak. The website then validates Keycloak's response before establishing the session. See the [browser authentication flow](../../reference/browser-authentication-flow/) for details. |
There was a problem hiding this comment.
I'm not sure about the change here as this is document is to give a quite high-level overview and this sounds quite technical already. Could this be phrased a bit simpler?
|
|
||
| Loculus uses [Keycloak](https://www.keycloak.org/) as its OpenID Connect (OIDC) provider. Keycloak authenticates the user, while the Loculus website starts the login transaction, validates the response and establishes the website session. | ||
|
|
||
| This page describes browser login. It does not change [authentication via the API](../../for-users/authenticate-via-api/), which uses a separate flow. |
There was a problem hiding this comment.
| This page describes browser login. It does not change [authentication via the API](../../for-users/authenticate-via-api/), which uses a separate flow. | |
| This page describes browser login. [Authentication via the API](../../for-users/authenticate-via-api/) follows a separate flow. |
|
|
||
| Loculus stores these values, together with `returnTo`, in an authenticated and encrypted HTTP-only cookie. The cookie: | ||
|
|
||
| - is valid for five minutes; |
There was a problem hiding this comment.
Indeed, I think the current UX is not great. If staying at the login screen for more than 5 minutes, one gets afterwards:
I think that the time should be extended. If one exceeds the time, one should get redirected back to the website with a useful message (like "Your session has expired, please click on login again.") - although what I find confusing here is that the authentication, at that point, will have been successfully performed by Keycloak already, so if one clicks on login, one gets directly logged in without needing to enter any credentials, that seems confusing and it's not clear to me how much security the time limt adds.
| ], | ||
| "attributes": { | ||
| "pkce.code.challenge.method": "S256", | ||
| "post.logout.redirect.uris": "https://{{$.Values.host}}/logout{{ if $.Values.insecureCookies }}##http://{{$.Values.host}}/logout##http://localhost:3000/logout{{ end }}" |
There was a problem hiding this comment.
To clarify what "Could we construct the logout URL from the origin instead" means: do you mean adapting UserPage.astro to not attach any ?foo=bar? Or do you mean adapting the keycloak config map here? (Do we have any use case for /logout?foo=bar?)
5 participants
Summary
Hardens the website’s OIDC login flow and restricts the configured post-logout redirect by moving authorization initiation behind Astro-managed endpoints and adding the standard protections required for an authorization-code flow.
Addresses:
Previous Flow
sequenceDiagram autonumber participant B as Browser participant K as Keycloak participant W as Loculus website B->>K: Start login K-->>B: Return authorization code B->>W: Send callback code W->>K: Exchange code for tokens K-->>W: Return tokens Note over B,W: The callback was not bound to the browser session that started loginThe callback did not validate
stateornonce, and the authorization-code exchange did not use PKCE.New Flow
sequenceDiagram autonumber participant B as Browser participant W as Loculus website (Astro) participant K as Keycloak B->>W: GET /auth/login W->>W: Generate state, nonce and PKCE verifier W-->>B: Set encrypted HTTP-only transaction cookie W-->>B: Redirect to Keycloak B->>K: Authorization request with state, nonce and PKCE challenge K-->>B: Redirect to /auth/callback with code and state B->>W: Send callback and transaction cookie W->>W: Validate and consume transaction W->>K: Exchange code with PKCE verifier K-->>W: Return tokens W->>W: Validate nonce and create session W-->>B: Redirect to the original Loculus pageWhat
state,nonceand PKCE dostateanswers: “Did this browser start this login?” Loculus generates a random value before redirecting to Keycloak, stores it in the protected transaction cookie, and requires Keycloak to return the same value. A callback without the matching value is rejected.nonceanswers: “Was this identity token issued for this login?” Loculus sends another random value to Keycloak and requires the resulting ID token to contain it. This prevents a token or authentication response from another login being reused.PKCE answers: “Is the application redeeming the code the same one that started the login?” Loculus creates a secret verifier and sends only its derived challenge to Keycloak. When exchanging the authorization code, Loculus must supply the original verifier. A stolen code is therefore insufficient on its own.
The browser carries the encrypted, HTTP-only transaction cookie, but its contents are not available to browser JavaScript. The transaction is short-lived and consumed when its callback is received. It cannot be replayed, and a failed callback requires starting a new login.
Security Properties
S256): binds the authorization code to the client that initiated the request./auth/callbackand configure/logoutseparately as an exact post-logout destination.Changes
/auth/loginand/auth/callbackendpoints./auth/login.Documentation
state,nonce, and PKCE, along with the encrypted, short-lived transaction cookie.Compatibility
This does not change backend API authentication or password-based CLI authentication. Existing website login and logout behaviour remains the same from the user’s perspective.
No database migration is required.
Verification
🚀 Preview: https://state-nonce.loculus.org