Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #941 +/- ##
==========================================
- Coverage 78.70% 75.69% -3.01%
==========================================
Files 57 61 +4
Lines 2517 2786 +269
==========================================
+ Hits 1981 2109 +128
- Misses 397 523 +126
- Partials 139 154 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Stuff I changed:
For inactive client deleting I've created #943, it's probably a bigger change, when all models get a createdAt field. So I didn't want to pollute this PR. |
|
@eternal-flame-AD Can you recheck the review fixes? I merge this then, and do the step up auth in a separate MR to not have a big MR with all changes in it. |
|
Sure, will take a look tomorrow , sorry have been busy |
|
Wait why do we still need this source field? Seems a little arbitrary .. If there is a planned behavior that depends on this field we should have a fixed list of permitted values, if not I would say just remove it or use a more generic "comment" field. lgtm otherwise, I'm not going to do a functional test as I'm unfamiliar with the technology so likely can't give advice better than it works or it doesn't. Your plan of putting it on docker for the proponents to test it seems like a good next step. |
I intended it to be used for the step up auth. When doing the session elevation, we need to know if we should present oidc or password authentication to the user. This could be based on the client source. We could let the user decide for session elevation which authentication method to use, as it doesn't really matter for the gotify side if the same authentication was used for creating the client. I think I'd prefer letting the user chose on session elevation, as this makes the logic simpler. What do you think? |
I would say let the user decide and don't worry about it. You can even reuse the login UI component. Memorizing the original auth method feels to be achieving very little to me at least in term of security, might as well give the user the option to use either at the time of use. If you meant it as a convenience (so user don't have to select the auth method at the step up prompt) I don't think it's worth the complexity, and we still must have a list of permitted value instead of a freestyle field. I can't think of a scenario where you would want to have a user to have client A (that must be stepped up with the user password) but client B which must be stepped up with OIDC instead, especially considering all clients are equal. If the goal is to prevent the user from switching authentication methods: just mark the user with a login_method field or bitmask seems to be most straightforward and standard to me. (I.e. each user can be userpass only, OIDC only, oe both) |
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.
|
Okay, removed the client source commit(s). |
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.