MSC3861: load the issuer and account management URLs from OIDC discovery (#17407)

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.

changelog.d/17407.misc

@@ -0,0 +1 @@
+MSC3861: load the issuer and account management URLs from OIDC discovery.

synapse/api/auth/msc3861_delegated.py

@@ -121,7 +121,9 @@ class MSC3861DelegatedAuth(BaseAuth):
         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
@@ -145,6 +147,33 @@ class MSC3861DelegatedAuth(BaseAuth):
         # 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 if not set in the metadata
+        """
+        metadata = await self._issuer_metadata.get()
+        return metadata.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._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:
+            logger.warning("Failed to load metadata:", exc_info=True)
+            return None
+
     async def _introspection_endpoint(self) -> str:
         """
         Returns the introspection endpoint of the issuer
@@ -154,7 +183,7 @@ class MSC3861DelegatedAuth(BaseAuth):
         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:

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 @@ class AuthRestServlet(RestServlet):
             self.hs.config.experimental.msc3861.enabled
             and stagetype == "org.matrix.cross_signing_reset"
         ):
-            config = self.hs.config.experimental.msc3861
+            # If MSC3861 is enabled, we can assume self._auth is an instance of MSC3861DelegatedAuth
-            if config.account_management_url is not None:
+            # We import lazily here because of the authlib requirement
-                url = f"{config.account_management_url}?action=org.matrix.cross_signing_reset"
+            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:

synapse/rest/client/auth_issuer.py

@@ -13,7 +13,7 @@
 # limitations under the License.
 import logging
 import typing
-from typing import Tuple
+from typing import Tuple, cast
 
 from synapse.api.errors import Codes, SynapseError
 from synapse.http.server import HttpServer
@@ -43,10 +43,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.

synapse/rest/client/keys.py

@@ -23,7 +23,7 @@
 import logging
 import re
 from collections import Counter
-from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple
+from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple, cast
 
 from synapse.api.errors import (
     InteractiveAuthIncompleteError,
@@ -406,11 +406,17 @@ class SigningKeyUploadServlet(RestServlet):
             # 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 MSC3861 is enabled, we can assume self.auth is an instance of MSC3861DelegatedAuth
-                    if config.account_management_url is not None:
+                    # We import lazily here because of the authlib requirement
-                        url = f"{config.account_management_url}?action=org.matrix.cross_signing_reset"
+                    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()
 
                     # We use a dummy session ID as this isn't really a UIA flow, but we
                     # reuse the same API shape for better client compatibility.

synapse/rest/client/login.py

@@ -268,7 +268,7 @@ class LoginRestServlet(RestServlet):
                     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

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 @@ logger = logging.getLogger(__name__)
 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 @@ class WellKnownBuilder:
 
         # 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 @@ class WellKnownBuilder:
         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:
+    async def _async_render_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
-        set_cors_headers(request)
+        r = await self._well_known_builder.get_well_known()
-        r = self._well_known_builder.get_well_known()
         if not r:
-            request.setResponseCode(404)
+            raise NotFoundError(".well-known not available")
-            request.setHeader(b"Content-Type", b"text/plain")
-            return b".well-known not available"
 
         logger.debug("returning: %s", r)
-        request.setHeader(b"Content-Type", b"application/json")
+        return 200, r
-        return json_encoder.encode(r).encode("utf-8")
 
 
 class ServerWellKnownResource(Resource):

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 @@ class AuthIssuerTestCase(HomeserverTestCase):
         }
     )
     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()

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 @@ class WellKnownTests(unittest.HomeserverTestCase):
                 "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 @@ class WellKnownTests(unittest.HomeserverTestCase):
         }
     )
     def test_client_well_known_msc3861_oauth_delegation(self) -> None:
-        channel = self.make_request(
+        # Patch the HTTP client to return the issuer metadata
-            "GET", "/.well-known/matrix/client", shorthand=False
+        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)
+        for _ in range(2):
-        self.assertEqual(
+            channel = self.make_request(
-            channel.json_body,
+                "GET", "/.well-known/matrix/client", shorthand=False
-            {
+            )
-                "m.homeserver": {"base_url": "https://homeserver/"},
+
-                "org.matrix.msc2965.authentication": {
+            self.assertEqual(channel.code, 200)
-                    "issuer": "https://issuer",
+            self.assertEqual(
-                    "account": "https://my-account.issuer",
+                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")