Query OIDC's UserInfo endpoint for authorotative claims - #2266
Conversation
SFTPGo reads all OIDC claims from the ID token doesn't query the UserInfo endpoint, forcing every provider to be configured to place profile claims (preferred_username, email, role, ...) into the ID token (e.g. an Authelia claims_policy). This is not a faithful implementation of OIDC: the UserInfo endpoint is the authoritative source for profile claims, so a stock install could not authenticate against a compliant provider. After verifying the ID token, SFTPGo now queries the UserInfo endpoint and overlays its claims over the ID token claims (UserInfo wins on conflict; except for the Back-Channel Logout "sid"). An unreachable endpoint is an availability failure and falls back to the ID token claims; a UserInfo response that is reached but cannot be trusted (its subject differs from the ID token subject, or its body cannot be parsed) rejects the login. The new id_token_claims_only option skips the request for providers that fail to support UserInfo.
Query the UserInfo endpoint only when the provider's discovery document advertises one, using the userinfo_endpoint exposed by go-oidc's Provider.UserInfoEndpoint(). An unadvertised endpoint is expected (RECOMMENDED, not REQUIRED per OIDC Discovery): and now only uses the id token claims rather than attempting a request that fails. Once the endpoint is advertised the provider is asserting UserInfo as part of the identity contract, so a failure to query and trust it now rejects the login instead of falling back: a fetch error, a missing access token, a subject mismatch, or an unparseable body are all treated as integrity failures. The id_token_claims_only option, which returns before any request, is the escape hatch for a provider whose advertised endpoint is broken or undesirable.
|
Thanks for the update, the overall structure looks good. I just did a quick review, so I may be wrong on some points, I'm also open to discuss the proposed changes, not strong opinions.
|
|
Thanks for giving this a look at your thoughts. I'll be able to act on your feedback in a couple of days.
👍
👍 thanks for the pointer
From a "default to unchanged" behaviour here, I absolutely see why this is what comes to mind as the most conservative approach. It just does very much seem to me like the wrong default, from my understanding of the flow OIDC Core seems to encourage (which may well be incorrect/incomplete). Considering the providers that I know of/have taken a peek at (as mentioned in the issue)/seen docs on, it looks like reading userinfo by default when advertised:
I think the only possible scenarios where reading userinfo could cause an issue is when:
These sound like instances of a provider that would fail OpenID certification and/or are misconfigured, i.e. instances when it's probably actually good to fail. @james-d-elliott it would be useful to hear if this sounds like the right interpretation/assesment of the situation to you? |
|
@tecosaur For SFTPGo Enterprise, I'll be conservative because I don't want to break existing deployments, and the codebase is already different anyway. For the community edition here, I'm fine with enabling the userinfo endpoint by default. These changes will be included in the next major release, so a potential breaking change is acceptable. I just need to remember to mention it in the release notes. The overall implementation looks fine but I need more time to properly review. Thank you! |
2 participants
This is something that's niggled me ever since I set up SFTPGo and discovered it was one of the handful of OIDC clients that hasn't quite faithfully implemented the OIDC spec and needs a workaround.
As per OIDC Core §5.3, the authoritative way to receive OIDC claims is through the userinfo endpoint. Currently, SFTPGo doesn't query this, and relies on all claims being shoved into the token.
So, I've gone ahead and added userinfo reading to the OIDC client implementation. Specifically, unless
id_token_claims_onlyis set (only needed in case of broken OIDC providers), we try to access the userinfo endpoint, and upon a response:It goes without saying that auth needs to be handled with care; this isn't the first time I've contributed OIDC improvements. I've upstreamed OIDC implementations/improvements to a handful of projects at this point, and consulted the Authelia maintainer to make sure I had the right understanding of how token/userinfo merging should be handled and what's a decent approach to non-conformant OIDC providers.
I got an LLM to both help with reviewing this work, and also write tests for it.
Closes #2265
Checklist for Pull Requests