Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Unexpected error if an OIDC IdP is configured without jwks_uri #12980

Open
babolivier opened this issue Jun 7, 2022 · 3 comments
Open

Unexpected error if an OIDC IdP is configured without jwks_uri #12980

babolivier opened this issue Jun 7, 2022 · 3 comments
Labels
A-Social Login Login via external identity providers S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@babolivier
Copy link
Contributor

babolivier commented Jun 7, 2022

If an IdP is configured with user_profile_method: "userinfo_endpoint", we allow it to not specify a jwks_uri property. jwks_uri is used if the IdP gives us an id_token, which is a JWT including profile information about the user being authenticated.

Initially, we would ignore id_token if the configured method to retrieve the user profile is userinfo_endpoint, which means in this case we don't care whether jwks_uri is defined. However, #11482 changed this logic so that we always validate and parse the id_token, regardless of what user_profile_method is set to.

This means that if an IdP is configured to use the userinfo endpoint and doesn't have jwks_uri set, authenticating via this IdP will fail with this error:

image

This error is raised in this function:

async def _load_jwks(self) -> JWKS:
metadata = await self.load_metadata()
# Load the JWKS using the `jwks_uri` metadata.
uri = metadata.get("jwks_uri")
if not uri:
# this should be unreachable: load_metadata validates that
# there is a jwks_uri in the metadata if _uses_userinfo is unset
raise RuntimeError('Missing "jwks_uri" in metadata')
jwk_set = await self._http_client.get_json(uri)
return jwk_set

Which still seems to think that it's impossible to reach it if the userinfo endpoint is used.

I chatted with @sandhose about this and the reasoning behind this logic is that if the IdP sends an id_token, it expects it to be validated.

I see a few ways to fix this issue:

  1. raise early with a less cryptic error if we realise the IdP sent us an id_token but we don't have a jwks_uri in the config
  2. make it mandatory to have jwks_uri set if the openid scope is listed in scopes
  3. go back to ignoring the id_token if we're using the userinfo endpoint (we do nothing with it apart from validating it in this case anyway)

I would lean towards 2, as it looks like the openid scope basically tells the IdP it should send an id_token. However, I don't have much context or knowledge around the OIDC spec and its implementation in Synapse, so I'd be happy to read others' opinions.

@babolivier babolivier added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. A-Social Login Login via external identity providers labels Jun 7, 2022
@babolivier
Copy link
Contributor Author

To improve searchability for this issue, the error message's string is Missing "jwks_uri" in metadata

@babolivier
Copy link
Contributor Author

It's worth noting workaround exists, for example setting the jwks_uri property for the IdP if there's one, or removing the openid scope.

@babolivier babolivier changed the title Confusing error if an OIDC IdP is configured without jwks_uri Unexpected error if an OIDC IdP is configured without jwks_uri Jun 8, 2022
@jaywink
Copy link
Member

jaywink commented Mar 24, 2023

I'm also hitting this trying to set up OIDC against a Mattermost server. However workaround "removing the openid scope" doesn't seem to work, at least anymore, and looking at the code I can't see how it would as the id_token always gets verified.

go back to ignoring the id_token if we're using the userinfo endpoint (we do nothing with it apart from validating it in this case anyway)

I believe in this case it would be ok to skip validation since based on what I can see in Mattermost code, it added id_token to a common access model due to implementing OpenID as a consumer (if I'm reading things correctly). Thus

if token.get("id_token") is not None:
afaict ends up thinking there is an id_token as the response is an empty string, not None.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Social Login Login via external identity providers S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

No branches or pull requests

2 participants