Conversation
Add two new endpoints for native app OIDC authentication using the PKCE relay pattern (similar to Vaultwarden's SSO implementation): - POST /auth/oidc/external/authorize - accepts a PKCE code_challenge from the client, forwards it to the IdP, and returns the authorize URL - POST /auth/oidc/external/token - accepts the auth code and code_verifier, relays them to the IdP for token exchange, and returns a gotify client token The server never generates its own PKCE pair for this flow. It then relays the client's code_challenge to the IdP during authorization and the code_verifier during token exchange. The IdP validates the binding. Pending auth sessions are stored in memory with a 10-minute TTL. CSRF protection is provided by the state parameter, which contains a cryptographically random nonce and is validated on the token exchange. The state is single-use (deleted from the pending session map on lookup), preventing replay attacks. Even without single-use enforcement, replay would be harmless since the IdP's authorization code can only be exchanged once.
It's not easy to test this automatically without a real oidc server.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #941 +/- ##
==========================================
- Coverage 78.70% 75.92% -2.79%
==========================================
Files 57 60 +3
Lines 2517 2758 +241
==========================================
+ Hits 1981 2094 +113
- Misses 397 514 +117
- Partials 139 150 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -16,7 +16,7 @@ type HealthAPI struct { | |||
| } | |||
There was a problem hiding this comment.
@eternal-flame-AD sorry this is kinda big, but would you mind having a look at this? Don't worry about declining if you don't want to review this, or don't have the time. I don't want to burden you with this :D.
Anyway, my plan is to create a snapshot docker build in the coming days and then let the people in #433 try out the functionality to improve the user-guide and find possible bugs with special IdP servers. So there is no rush reviewing this.
There was a problem hiding this comment.
Sure, I can look at this tonight first to see if there are large architectural/security changes needed and we can do a final round for final check and nitpicks after the code settles. Probably won't review Android though because I'm not very familar with android dev.
| func (a *OIDCAPI) storePendingSession(state string, session *pendingOIDCSession) { | ||
| a.pendingSessionsMu.Lock() | ||
| defer a.pendingSessionsMu.Unlock() | ||
| for s, sess := range a.pendingSessions { |
There was a problem hiding this comment.
I feel this could be done better, this has O(n^2) complexity over number of users who are authenticating at the same time.
If we are unconditionally iterating through the entire map as the first action of the flow anyways, might as well just use an array (with swap-delete).
A more complex solution is bulk cleanup like this: https://gist.github.com/eternal-flame-AD/e4a47a8a7e6b26bba365a7d24ab84221
There was a problem hiding this comment.
My plan was to have a simple implementation as we probably don't have many parallel logins anyway.
Your decay map doesn't seem to complex, I'll use it, as it's not that much code overhead.
| http.Error(w, err.Error(), status) | ||
| return | ||
| } | ||
| clientName, _, _ := strings.Cut(state, ":") |
There was a problem hiding this comment.
Can the name itself already have a colon in it?
There was a problem hiding this comment.
True, I'll reverse the order of the random nonce and the client name and then split by the first colon as the nonce is hex encoded and never includes a colon.
There was a problem hiding this comment.
See below, but for a simple fix I would say just base64 URL safe encode it.
| ctx.AbortWithError(http.StatusBadRequest, err) | ||
| return | ||
| } | ||
| state, err := a.generateState(req.Name) |
There was a problem hiding this comment.
Is this how it works for VaultWarden (server generates state) ? I'm not super sure what this is for. From my cursory exploration it seems like in VaultWarden state is simply for the client's convenience (might be wrong..)
There was a problem hiding this comment.
Okay this seems to be documented in RFC6749, I got it messed up. The flow is right although the bookkeeping ClientName could be clearer (don't keep them both in the server memory and in the token text at the same time).
There was a problem hiding this comment.
No, this is different from vaultwarden, but as I understood is this for CSRF protection, so basically the android app has to check the initial state against the state that will be received on the redirect uri. So I don't think it matters.
Do you mean removing the client name from the pending session struct and then directly reading it from the state similar to how it's done in the web ui flow?
There was a problem hiding this comment.
Do you mean removing the client name from the pending session struct and then directly reading it from the state similar to how it's done in the web ui flow?
Yes, actually I would prefer the inverse, I think either of these are better:
- remove the "plaintext" name from the state as it's only a CSRF token but store it in the pending session struct as we are already doing in L72), OR
- make the content of state actually semantically strong (JSON, optionally signed, etc)
There was a problem hiding this comment.
Currently, the pending sessions struct is only used with the mobile flow. The webui flow, doesn't need it, as it internally already stores the state in cookies.
To make it consistent, we coulfd fill the pending session also for the web flow and then only store the client name in the pending session struct. This would duplicate some validations that are already done in the standard webui oidc flow. But I don't think it's much of a problem. I'll change it to be consistent between the flows.
2 participants
This MR:
requireToken/ callback structure with a simpler approach./auth/local/{login,logout}endpoints for web UI authentication.https://gotify-ui.net/#/oidc/callback?token=...to store the token in the local storage. Using cookies seems to be more easy than securing this against CSRF and stuff. Also with HttpOnly cookies we guard against pontential token exposures.RefererorX-Forwarded-Protoheaders in the future.Web UI: Standard OAuth2 Authorization Code + PKCE
This is the standard OIDC implementation with a library.
GET /auth/oidc/login, which then redirects to the IdP./auth/oidc/callback, which:Android App flow
Android's recommended App Links (verified domains) aren't usable here because the server is self-hosted at a user-chosen domain.
I guess most of the android app, are doing their own OIDC flow against the IdP server to then obtain a token from the IdP server to then access the resources with the token. In our case, we need a client token from gotify.
We could let the android app do the whole OIDC flow, but this would require us to have a public oauth2 client, as we don't want to store the client_secret in the android app. Requesting the client secret from the server seems like a bad idea.
Another simple implementation would be to do the same flow as the Web-UI, but in the end redirecting to a custom URI scheme like gotify://oidc/callback?token=C123123123 and then using this token for authentication. I've decided against this because the custom URI scheme can be set by any app, and therefore intercept the token.
PKCE was exactly made to prevent this, so I've searched for a way to still get the benefits from PKCE, but not having to reimplement a OIDC like handshake.
This flow is non standard but it's basically stolen from Vaultwarden. I'm not that deep in security, but I'd expect they spend some time designing this, and ensuring it's secure.
It works like this.
code_verifier/code_challengepair.POST /auth/oidc/external/authorizewithcode_challenge, a custom-schemeredirect_uri, and a client name. The server stores a pending session keyed by a randomstatetoken, and then reponds with the the authorization URL +state. Vaultwarden 1, Vaultwarden 2,codeandstate.POST /auth/oidc/external/tokenwithcode,state, andcode_verifier. The server gets the pending session, relays thecode_verifierto the IdP for PKCE validation, exchanges the code for tokens, resolves the user, and returns a Gotify client token + user info. Vaultwarden 1, Vaultwarden 2PKCE is required, and only verified by the IdP. We could support non PKCE IdP by manually validating the challange on the /auth/oidc/external/token endpoint, simliar to Vaultwarden Impl, but I think most of the Idp will support PKCE, so it's required until someone requests it.
For testing I've added Dex and Authelia config in ./test/oidc.
Related MRs:
I've manually tested this with:
Disclaimer: this PR was created with AI assistance.