From 65120fe60485da5b3a1b9ae82baea3ef25c83cda Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 8 Jul 2024 11:54:04 +0200 Subject: [PATCH 1/9] MSC3861: load the issuer and account management URLs from OIDC discovery This will help mitigating any discrepancies between the issuer configured and the one returned by the OIDC provider. This also removes the need for configuring the `account_management_url` explicitely, as it will now be loaded from the OIDC discovery, as per MSC2965. Because we may now fetch stuff for the .well-known/matrix/client endpoint, this also transforms the client well-known resource to be asynchronous. --- synapse/api/auth/msc3861_delegated.py | 36 ++++++++++++++++++++++++++ synapse/rest/client/auth_issuer.py | 10 +++++++- synapse/rest/client/keys.py | 16 ++++++++---- synapse/rest/client/login.py | 2 +- synapse/rest/well_known.py | 37 +++++++++++++++------------ 5 files changed, 78 insertions(+), 23 deletions(-) diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index f61b39ded7c..92ad41dbabf 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -145,6 +145,42 @@ async def _load_metadata(self) -> OpenIDProviderMetadata: # metadata.validate_introspection_endpoint() return metadata + async def issuer(self) -> str: + """ + Get the configured issuer + + This will use the issuer value set in the metadata, + falling back to the one set in the config in case the metadata can't be loaded + """ + issuer: Optional[str] = None + try: + metadata = await self._load_metadata() + issuer = metadata.issuer + # We don't want to raise here if we can't load the metadata + except Exception: + logger.warning("Failed to load metadata:", exc_info=True) + + # Fallback to the config value if the metadata can't be loaded + # or if the metadata doesn't have an issuer set + return issuer or self._config.issuer + + async def account_management_url(self) -> Optional[str]: + """ + Get the configured account management URL + + This will discover the account management URL from the issuer if it's not set in the config + """ + if self._config.account_management_url is not None: + return self._config.account_management_url + + try: + metadata = await self._load_metadata() + return metadata.account_management_uri + # We don't want to raise here if we can't load the metadata + except Exception: + logger.warning("Failed to load metadata:", exc_info=True) + return None + async def _introspect_token(self, token: str) -> IntrospectionToken: """ Send a token to the introspection endpoint and returns the introspection response diff --git a/synapse/rest/client/auth_issuer.py b/synapse/rest/client/auth_issuer.py index 77b97209569..d630724e2b5 100644 --- a/synapse/rest/client/auth_issuer.py +++ b/synapse/rest/client/auth_issuer.py @@ -15,6 +15,8 @@ import typing from typing import Tuple +from typing_extensions import cast + from synapse.api.errors import Codes, SynapseError from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet @@ -43,10 +45,16 @@ class AuthIssuerServlet(RestServlet): def __init__(self, hs: "HomeServer"): super().__init__() self._config = hs.config + self._auth = hs.get_auth() async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if self._config.experimental.msc3861.enabled: - return 200, {"issuer": self._config.experimental.msc3861.issuer} + # If MSC3861 is enabled, we can assume self._auth is an instance of MSC3861DelegatedAuth + # We import lazily here because of the authlib requirement + from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth + + auth = cast(MSC3861DelegatedAuth, self._auth) + return 200, {"issuer": await auth.issuer()} else: # Wouldn't expect this to be reached: the servelet shouldn't have been # registered. Still, fail gracefully if we are registered for some reason. diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 67de634eab5..11760d04cb6 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -24,7 +24,7 @@ import re from collections import Counter from http import HTTPStatus -from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple +from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple, cast from synapse.api.errors import Codes, InvalidAPICallError, SynapseError from synapse.http.server import HttpServer @@ -397,11 +397,17 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # explicitly mark the master key as replaceable. if self.hs.config.experimental.msc3861.enabled: if not master_key_updatable_without_uia: - config = self.hs.config.experimental.msc3861 - if config.account_management_url is not None: - url = f"{config.account_management_url}?action=org.matrix.cross_signing_reset" + # If MSC3861 is enabled, we can assume self.auth is an instance of MSC3861DelegatedAuth + # We import lazily here because of the authlib requirement + from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth + + auth = cast(MSC3861DelegatedAuth, self.auth) + + uri = await auth.account_management_url() + if uri is not None: + url = f"{uri}?action=org.matrix.cross_signing_reset" else: - url = config.issuer + url = await auth.issuer() raise SynapseError( HTTPStatus.NOT_IMPLEMENTED, diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index ae691bcdba1..03b1e7edc49 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -268,7 +268,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]: approval_notice_medium=ApprovalNoticeMedium.NONE, ) - well_known_data = self._well_known_builder.get_well_known() + well_known_data = await self._well_known_builder.get_well_known() if well_known_data: result["well_known"] = well_known_data return 200, result diff --git a/synapse/rest/well_known.py b/synapse/rest/well_known.py index d0ca8ca46b4..989e570671b 100644 --- a/synapse/rest/well_known.py +++ b/synapse/rest/well_known.py @@ -18,12 +18,13 @@ # # import logging -from typing import TYPE_CHECKING, Optional +from typing import TYPE_CHECKING, Optional, Tuple, cast from twisted.web.resource import Resource from twisted.web.server import Request -from synapse.http.server import set_cors_headers +from synapse.api.errors import NotFoundError +from synapse.http.server import DirectServeJsonResource from synapse.http.site import SynapseRequest from synapse.types import JsonDict from synapse.util import json_encoder @@ -38,8 +39,9 @@ class WellKnownBuilder: def __init__(self, hs: "HomeServer"): self._config = hs.config + self._auth = hs.get_auth() - def get_well_known(self) -> Optional[JsonDict]: + async def get_well_known(self) -> Optional[JsonDict]: if not self._config.server.serve_client_wellknown: return None @@ -52,13 +54,20 @@ def get_well_known(self) -> Optional[JsonDict]: # We use the MSC3861 values as they are used by multiple MSCs if self._config.experimental.msc3861.enabled: + # If MSC3861 is enabled, we can assume self._auth is an instance of MSC3861DelegatedAuth + # We import lazily here because of the authlib requirement + from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth + + auth = cast(MSC3861DelegatedAuth, self._auth) + result["org.matrix.msc2965.authentication"] = { - "issuer": self._config.experimental.msc3861.issuer + "issuer": await auth.issuer(), } - if self._config.experimental.msc3861.account_management_url is not None: + account_management_url = await auth.account_management_url() + if account_management_url is not None: result["org.matrix.msc2965.authentication"][ "account" - ] = self._config.experimental.msc3861.account_management_url + ] = account_management_url if self._config.server.extra_well_known_client_content: for ( @@ -71,26 +80,22 @@ def get_well_known(self) -> Optional[JsonDict]: return result -class ClientWellKnownResource(Resource): +class ClientWellKnownResource(DirectServeJsonResource): """A Twisted web resource which renders the .well-known/matrix/client file""" isLeaf = 1 def __init__(self, hs: "HomeServer"): - Resource.__init__(self) + super().__init__() self._well_known_builder = WellKnownBuilder(hs) - def render_GET(self, request: SynapseRequest) -> bytes: - set_cors_headers(request) - r = self._well_known_builder.get_well_known() + async def _async_render_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: + r = await self._well_known_builder.get_well_known() if not r: - request.setResponseCode(404) - request.setHeader(b"Content-Type", b"text/plain") - return b".well-known not available" + raise NotFoundError(".well-known not available") logger.debug("returning: %s", r) - request.setHeader(b"Content-Type", b"application/json") - return json_encoder.encode(r).encode("utf-8") + return 200, r class ServerWellKnownResource(Resource): From 86d7fee8cfdd7888980a019a2196f0ba41839bbb Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 8 Jul 2024 12:03:47 +0200 Subject: [PATCH 2/9] Newsfile. --- changelog.d/17407.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17407.misc diff --git a/changelog.d/17407.misc b/changelog.d/17407.misc new file mode 100644 index 00000000000..9ed6e61a5b5 --- /dev/null +++ b/changelog.d/17407.misc @@ -0,0 +1 @@ +MSC3861: load the issuer and account management URLs from OIDC discovery. From ee0cd1a1e699413aacc4bd4fdb1e69acd041a5cd Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 8 Jul 2024 12:17:49 +0200 Subject: [PATCH 3/9] Import `cast` from `typing` instead of `typing_extensions`. --- synapse/rest/client/auth_issuer.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/rest/client/auth_issuer.py b/synapse/rest/client/auth_issuer.py index d630724e2b5..acd0399d856 100644 --- a/synapse/rest/client/auth_issuer.py +++ b/synapse/rest/client/auth_issuer.py @@ -13,9 +13,7 @@ # limitations under the License. import logging import typing -from typing import Tuple - -from typing_extensions import cast +from typing import Tuple, cast from synapse.api.errors import Codes, SynapseError from synapse.http.server import HttpServer From 2ff5321ef172f7f5b17f6ec001c0073c13eeee6b Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 8 Jul 2024 13:27:44 +0200 Subject: [PATCH 4/9] Fix loading the account management URL from the issuer --- synapse/api/auth/msc3861_delegated.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index 92ad41dbabf..cc55be40d80 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -175,7 +175,7 @@ async def account_management_url(self) -> Optional[str]: try: metadata = await self._load_metadata() - return metadata.account_management_uri + return metadata.get("account_management_uri", None) # We don't want to raise here if we can't load the metadata except Exception: logger.warning("Failed to load metadata:", exc_info=True) From 0119d40b7da825096269fc3834cd62e04bc6ff1c Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 11 Jul 2024 16:28:27 +0200 Subject: [PATCH 5/9] Use the cached metadata call --- synapse/api/auth/msc3861_delegated.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index 285a0395ddb..ecda2a14f2a 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -121,7 +121,9 @@ def __init__(self, hs: "HomeServer"): self._hostname = hs.hostname self._admin_token = self._config.admin_token - self._issuer_metadata = RetryOnExceptionCachedCall(self._load_metadata) + self._issuer_metadata = RetryOnExceptionCachedCall[OpenIDProviderMetadata]( + self._load_metadata + ) if isinstance(auth_method, PrivateKeyJWTWithKid): # Use the JWK as the client secret when using the private_key_jwt method @@ -154,7 +156,7 @@ async def issuer(self) -> str: """ issuer: Optional[str] = None try: - metadata = await self._load_metadata() + metadata = await self._issuer_metadata.get() issuer = metadata.issuer # We don't want to raise here if we can't load the metadata except Exception: @@ -174,7 +176,7 @@ async def account_management_url(self) -> Optional[str]: return self._config.account_management_url try: - metadata = await self._load_metadata() + metadata = await self._issuer_metadata.get() return metadata.get("account_management_uri", None) # We don't want to raise here if we can't load the metadata except Exception: @@ -190,7 +192,7 @@ async def _introspection_endpoint(self) -> str: if self._config.introspection_endpoint is not None: return self._config.introspection_endpoint - metadata = await self._load_metadata() + metadata = await self._issuer_metadata.get() return metadata.get("introspection_endpoint") async def _introspect_token(self, token: str) -> IntrospectionToken: From 5d9d26919abb5a0fcbe2b54a64cec13e4b5db171 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 30 Aug 2024 10:27:37 +0200 Subject: [PATCH 6/9] Don't fallback to the config issuer if loading the metadata fails --- synapse/api/auth/msc3861_delegated.py | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index ecda2a14f2a..6bd845c7e30 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -152,19 +152,10 @@ async def issuer(self) -> str: Get the configured issuer This will use the issuer value set in the metadata, - falling back to the one set in the config in case the metadata can't be loaded + falling back to the one set in the config if not set in the metadata """ - issuer: Optional[str] = None - try: - metadata = await self._issuer_metadata.get() - issuer = metadata.issuer - # We don't want to raise here if we can't load the metadata - except Exception: - logger.warning("Failed to load metadata:", exc_info=True) - - # Fallback to the config value if the metadata can't be loaded - # or if the metadata doesn't have an issuer set - return issuer or self._config.issuer + metadata = await self._issuer_metadata.get() + return metadata.issuer or self._config.issuer async def account_management_url(self) -> Optional[str]: """ From 089a542fe7bd768af57ffb03fb6bc3279ccaec6c Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 30 Aug 2024 14:59:25 +0200 Subject: [PATCH 7/9] Use the right account management URL in the UIA fallback --- synapse/rest/client/auth.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/synapse/rest/client/auth.py b/synapse/rest/client/auth.py index 32eeecd6620..b8dca7c7977 100644 --- a/synapse/rest/client/auth.py +++ b/synapse/rest/client/auth.py @@ -20,7 +20,7 @@ # import logging -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast from twisted.web.server import Request @@ -70,11 +70,17 @@ async def on_GET(self, request: SynapseRequest, stagetype: str) -> None: self.hs.config.experimental.msc3861.enabled and stagetype == "org.matrix.cross_signing_reset" ): - config = self.hs.config.experimental.msc3861 - if config.account_management_url is not None: - url = f"{config.account_management_url}?action=org.matrix.cross_signing_reset" + # If MSC3861 is enabled, we can assume self._auth is an instance of MSC3861DelegatedAuth + # We import lazily here because of the authlib requirement + from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth + + auth = cast(MSC3861DelegatedAuth, self.auth) + + url = await auth.account_management_url() + if url is not None: + url = f"{url}?action=org.matrix.cross_signing_reset" else: - url = config.issuer + url = await auth.issuer() respond_with_redirect(request, str.encode(url)) if stagetype == LoginType.RECAPTCHA: From 17a90f752c61b4231f277e2428278b788aa1b349 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 30 Aug 2024 15:22:39 +0200 Subject: [PATCH 8/9] Fix tests now that we're always fetching the issuer metadata --- tests/rest/client/test_auth_issuer.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_auth_issuer.py b/tests/rest/client/test_auth_issuer.py index 964baeec324..299475a35cc 100644 --- a/tests/rest/client/test_auth_issuer.py +++ b/tests/rest/client/test_auth_issuer.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. from http import HTTPStatus +from unittest.mock import AsyncMock from synapse.rest.client import auth_issuer @@ -50,10 +51,27 @@ def test_returns_404_when_msc3861_disabled(self) -> None: } ) def test_returns_issuer_when_oidc_enabled(self) -> None: - # Make an unauthenticated request for the discovery info. + # Patch the HTTP client to return the issuer metadata + req_mock = AsyncMock(return_value={"issuer": ISSUER}) + self.hs.get_proxied_http_client().get_json = req_mock # type: ignore[method-assign] + + channel = self.make_request( + "GET", + "/_matrix/client/unstable/org.matrix.msc2965/auth_issuer", + ) + + self.assertEqual(channel.code, HTTPStatus.OK) + self.assertEqual(channel.json_body, {"issuer": ISSUER}) + + req_mock.assert_called_with("https://account.example.com/.well-known/openid-configuration") + req_mock.reset_mock() + + # Second call it should use the cached value channel = self.make_request( "GET", "/_matrix/client/unstable/org.matrix.msc2965/auth_issuer", ) + self.assertEqual(channel.code, HTTPStatus.OK) self.assertEqual(channel.json_body, {"issuer": ISSUER}) + req_mock.assert_not_called() From 40c854c93c072311260aca2e0914be6af363eb74 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 30 Aug 2024 15:39:57 +0200 Subject: [PATCH 9/9] Fix well-known tests now that we're fetching through the metadata --- tests/rest/test_well_known.py | 37 ++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/tests/rest/test_well_known.py b/tests/rest/test_well_known.py index e166c13bc16..ac992766e87 100644 --- a/tests/rest/test_well_known.py +++ b/tests/rest/test_well_known.py @@ -17,6 +17,8 @@ # [This file includes modifications made by New Vector Limited] # # +from unittest.mock import AsyncMock + from twisted.web.resource import Resource from synapse.rest.well_known import well_known_resource @@ -112,7 +114,6 @@ def test_server_well_known_disabled(self) -> None: "msc3861": { "enabled": True, "issuer": "https://issuer", - "account_management_url": "https://my-account.issuer", "client_id": "id", "client_auth_method": "client_secret_post", "client_secret": "secret", @@ -122,18 +123,26 @@ def test_server_well_known_disabled(self) -> None: } ) def test_client_well_known_msc3861_oauth_delegation(self) -> None: - channel = self.make_request( - "GET", "/.well-known/matrix/client", shorthand=False - ) + # Patch the HTTP client to return the issuer metadata + req_mock = AsyncMock(return_value={"issuer": "https://issuer", "account_management_uri": "https://my-account.issuer"}) + self.hs.get_proxied_http_client().get_json = req_mock # type: ignore[method-assign] - self.assertEqual(channel.code, 200) - self.assertEqual( - channel.json_body, - { - "m.homeserver": {"base_url": "https://homeserver/"}, - "org.matrix.msc2965.authentication": { - "issuer": "https://issuer", - "account": "https://my-account.issuer", + for _ in range(2): + channel = self.make_request( + "GET", "/.well-known/matrix/client", shorthand=False + ) + + self.assertEqual(channel.code, 200) + self.assertEqual( + channel.json_body, + { + "m.homeserver": {"base_url": "https://homeserver/"}, + "org.matrix.msc2965.authentication": { + "issuer": "https://issuer", + "account": "https://my-account.issuer", + }, }, - }, - ) + ) + + # It should have been called exactly once, because it gets cached + req_mock.assert_called_once_with("https://issuer/.well-known/openid-configuration")