From caa1f9d806945e056261ec6879da3b1a1cc23b17 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Thu, 20 Feb 2025 17:56:53 -0600
Subject: [PATCH] Add support for overriding
`id_token_signing_alg_values_supported` for an OpenID identity provider
(#18177)
Normally, when `discovery` is enabled,
`id_token_signing_alg_values_supported` comes from the OpenID Discovery
Document (`/.well-known/openid-configuration`). If nothing was
specified, we default to supporting `RS256` in the downstream usage.
This PR just adds support for adding a default/overriding the the
discovered value [just like we do for other things like the
`token_endpoint`](https://github.com/element-hq/synapse/blob/1525a3b4d48a0f5657d61423e1f205bff9a77948/docs/usage/configuration/config_documentation.md#oidc_providers),
etc.
---
changelog.d/18177.feature | 1 +
.../configuration/config_documentation.md | 18 ++++++
synapse/config/oidc.py | 35 ++++++++++
synapse/handlers/oidc.py | 5 ++
tests/handlers/test_oidc.py | 64 +++++++++++++++++--
5 files changed, 119 insertions(+), 4 deletions(-)
create mode 100644 changelog.d/18177.feature
diff --git a/changelog.d/18177.feature b/changelog.d/18177.feature
new file mode 100644
index 0000000000..71d568474b
--- /dev/null
+++ b/changelog.d/18177.feature
@@ -0,0 +1 @@
+Add support for specifying/overriding `id_token_signing_alg_values_supported` for an OpenID identity provider.
diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md
index e3c06d5371..facf60a043 100644
--- a/docs/usage/configuration/config_documentation.md
+++ b/docs/usage/configuration/config_documentation.md
@@ -3579,6 +3579,24 @@ Options for each entry include:
to `auto`, which uses PKCE if supported during metadata discovery. Set to `always`
to force enable PKCE or `never` to force disable PKCE.
+* `id_token_signing_alg_values_supported`: List of the JWS signing algorithms (`alg`
+ values) that are supported for signing the `id_token`.
+
+ This is *not* required if `discovery` is disabled. We default to supporting `RS256` in
+ the downstream usage if no algorithms are configured here or in the discovery
+ document.
+
+ According to the spec, the algorithm `"RS256"` MUST be included. The absolute rigid
+ approach would be to reject this provider as non-compliant if it's not included but we
+ simply allow whatever and see what happens (you're the one that configured the value
+ and cooperating with the identity provider).
+
+ The `alg` value `"none"` MAY be supported but can only be used if the Authorization
+ Endpoint does not include `id_token` in the `response_type` (ex.
+ `/authorize?response_type=code` where `none` can apply,
+ `/authorize?response_type=code%20id_token` where `none` can't apply) (such as when
+ using the Authorization Code Flow).
+
* `scopes`: list of scopes to request. This should normally include the "openid"
scope. Defaults to `["openid"]`.
diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py
index d0a03baf55..fc4bc35b30 100644
--- a/synapse/config/oidc.py
+++ b/synapse/config/oidc.py
@@ -125,6 +125,10 @@ OIDC_PROVIDER_CONFIG_SCHEMA = {
"enum": ["client_secret_basic", "client_secret_post", "none"],
},
"pkce_method": {"type": "string", "enum": ["auto", "always", "never"]},
+ "id_token_signing_alg_values_supported": {
+ "type": "array",
+ "items": {"type": "string"},
+ },
"scopes": {"type": "array", "items": {"type": "string"}},
"authorization_endpoint": {"type": "string"},
"token_endpoint": {"type": "string"},
@@ -326,6 +330,9 @@ def _parse_oidc_config_dict(
client_secret_jwt_key=client_secret_jwt_key,
client_auth_method=client_auth_method,
pkce_method=oidc_config.get("pkce_method", "auto"),
+ id_token_signing_alg_values_supported=oidc_config.get(
+ "id_token_signing_alg_values_supported"
+ ),
scopes=oidc_config.get("scopes", ["openid"]),
authorization_endpoint=oidc_config.get("authorization_endpoint"),
token_endpoint=oidc_config.get("token_endpoint"),
@@ -402,6 +409,34 @@ class OidcProviderConfig:
# Valid values are 'auto', 'always', and 'never'.
pkce_method: str
+ id_token_signing_alg_values_supported: Optional[List[str]]
+ """
+ List of the JWS signing algorithms (`alg` values) that are supported for signing the
+ `id_token`.
+
+ This is *not* required if `discovery` is disabled. We default to supporting `RS256`
+ in the downstream usage if no algorithms are configured here or in the discovery
+ document.
+
+ According to the spec, the algorithm `"RS256"` MUST be included. The absolute rigid
+ approach would be to reject this provider as non-compliant if it's not included but
+ we can just allow whatever and see what happens (they're the ones that configured
+ the value and cooperating with the identity provider). It wouldn't be wise to add it
+ ourselves because absence of `RS256` might indicate that the provider actually
+ doesn't support it, despite the spec requirement. Adding it silently could lead to
+ failed authentication attempts or strange mismatch attacks.
+
+ The `alg` value `"none"` MAY be supported but can only be used if the Authorization
+ Endpoint does not include `id_token` in the `response_type` (ex.
+ `/authorize?response_type=code` where `none` can apply,
+ `/authorize?response_type=code%20id_token` where `none` can't apply) (such as when
+ using the Authorization Code Flow).
+
+ Spec:
+ - https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata
+ - https://openid.net/specs/openid-connect-core-1_0.html#AuthorizationExamples
+ """
+
# list of scopes to request
scopes: Collection[str]
diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py
index c9109c9e79..76b692928d 100644
--- a/synapse/handlers/oidc.py
+++ b/synapse/handlers/oidc.py
@@ -640,6 +640,11 @@ class OidcProvider:
elif self._config.pkce_method == "never":
metadata.pop("code_challenge_methods_supported", None)
+ if self._config.id_token_signing_alg_values_supported:
+ metadata["id_token_signing_alg_values_supported"] = (
+ self._config.id_token_signing_alg_values_supported
+ )
+
self._validate_metadata(metadata)
return metadata
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 1b43ee43c6..5ffc5a90a8 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -70,12 +70,16 @@ DEFAULT_CONFIG = {
}
# extends the default config with explicit OAuth2 endpoints instead of using discovery
+#
+# We add "explicit" to things to make them different from the discovered values to make
+# sure that the explicit values override the discovered ones.
EXPLICIT_ENDPOINT_CONFIG = {
**DEFAULT_CONFIG,
"discover": False,
- "authorization_endpoint": ISSUER + "authorize",
- "token_endpoint": ISSUER + "token",
- "jwks_uri": ISSUER + "jwks",
+ "authorization_endpoint": ISSUER + "authorize-explicit",
+ "token_endpoint": ISSUER + "token-explicit",
+ "jwks_uri": ISSUER + "jwks-explicit",
+ "id_token_signing_alg_values_supported": ["RS256", "<explicit>"],
}
@@ -259,12 +263,64 @@ class OidcHandlerTestCase(HomeserverTestCase):
self.get_success(self.provider.load_metadata())
self.fake_server.get_metadata_handler.assert_not_called()
+ @override_config({"oidc_config": {**EXPLICIT_ENDPOINT_CONFIG, "discover": True}})
+ def test_discovery_with_explicit_config(self) -> None:
+ """
+ The handler should discover the endpoints from OIDC discovery document but
+ values are overriden by the explicit config.
+ """
+ # This would throw if some metadata were invalid
+ metadata = self.get_success(self.provider.load_metadata())
+ self.fake_server.get_metadata_handler.assert_called_once()
+
+ self.assertEqual(metadata.issuer, self.fake_server.issuer)
+ # It seems like authlib does not have that defined in its metadata models
+ self.assertEqual(
+ metadata.get("userinfo_endpoint"),
+ self.fake_server.userinfo_endpoint,
+ )
+
+ # Ensure the values are overridden correctly since these were configured
+ # explicitly
+ self.assertEqual(
+ metadata.authorization_endpoint,
+ EXPLICIT_ENDPOINT_CONFIG["authorization_endpoint"],
+ )
+ self.assertEqual(
+ metadata.token_endpoint, EXPLICIT_ENDPOINT_CONFIG["token_endpoint"]
+ )
+ self.assertEqual(metadata.jwks_uri, EXPLICIT_ENDPOINT_CONFIG["jwks_uri"])
+ self.assertEqual(
+ metadata.id_token_signing_alg_values_supported,
+ EXPLICIT_ENDPOINT_CONFIG["id_token_signing_alg_values_supported"],
+ )
+
+ # subsequent calls should be cached
+ self.reset_mocks()
+ self.get_success(self.provider.load_metadata())
+ self.fake_server.get_metadata_handler.assert_not_called()
+
@override_config({"oidc_config": EXPLICIT_ENDPOINT_CONFIG})
def test_no_discovery(self) -> None:
"""When discovery is disabled, it should not try to load from discovery document."""
- self.get_success(self.provider.load_metadata())
+ metadata = self.get_success(self.provider.load_metadata())
self.fake_server.get_metadata_handler.assert_not_called()
+ # Ensure the values are overridden correctly since these were configured
+ # explicitly
+ self.assertEqual(
+ metadata.authorization_endpoint,
+ EXPLICIT_ENDPOINT_CONFIG["authorization_endpoint"],
+ )
+ self.assertEqual(
+ metadata.token_endpoint, EXPLICIT_ENDPOINT_CONFIG["token_endpoint"]
+ )
+ self.assertEqual(metadata.jwks_uri, EXPLICIT_ENDPOINT_CONFIG["jwks_uri"])
+ self.assertEqual(
+ metadata.id_token_signing_alg_values_supported,
+ EXPLICIT_ENDPOINT_CONFIG["id_token_signing_alg_values_supported"],
+ )
+
@override_config({"oidc_config": DEFAULT_CONFIG})
def test_load_jwks(self) -> None:
"""JWKS loading is done once (then cached) if used."""
--
GitLab