From 09e7a1e98932a03678d450d46e06f7624a620e88 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Fri, 6 Dec 2024 12:52:57 +0100 Subject: [PATCH 1/5] experimental_features.msc3861.client_secret_path --- synapse/api/auth/msc3861_delegated.py | 5 +-- synapse/config/experimental.py | 52 ++++++++++++++++++++++++--- tests/config/test_load.py | 5 +++ 3 files changed, 56 insertions(+), 6 deletions(-) diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index 53907c01d43..f1eb7754d7f 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -133,9 +133,10 @@ def __init__(self, hs: "HomeServer"): ) else: # Else use the client secret - assert self._config.client_secret, "No client_secret provided" + client_secret = self._config.client_secret() + assert client_secret, "No client_secret provided" self._client_auth = ClientAuth( - self._config.client_id, self._config.client_secret, auth_method + self._config.client_id, client_secret, auth_method ) async def _load_metadata(self) -> OpenIDProviderMetadata: diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index eb8d967e709..6ef25781b06 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -27,7 +27,7 @@ from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersions from synapse.config import ConfigError -from synapse.config._base import Config, RootConfig +from synapse.config._base import Config, RootConfig, read_file from synapse.types import JsonDict # Determine whether authlib is installed. @@ -63,6 +63,23 @@ def _parse_jwks(jwks: Optional[JsonDict]) -> Optional["JsonWebKey"]: return JsonWebKey.import_key(jwks) +def _check_client_secret( + instance: "MSC3861", _attribute: attr.Attribute, _value: Optional[str] +) -> None: + if instance._client_secret and instance._client_secret_path: + raise ConfigError( + ( + "You have configured both " + "`experimental_features.msc3861.client_secret` and " + "`experimental_features.msc3861.client_secret_path`. " + "These are mutually incompatible." + ), + ("experimental", "msc3861", "client_secret"), + ) + # Check client secret can be retrieved + instance.client_secret() + + @attr.s(slots=True, frozen=True) class MSC3861: """Configuration for MSC3861: Matrix architecture change to delegate authentication via OIDC""" @@ -97,15 +114,30 @@ def _check_enabled(self, attribute: attr.Attribute, value: bool) -> None: ) """The auth method used when calling the introspection endpoint.""" - client_secret: Optional[str] = attr.ib( + _client_secret: Optional[str] = attr.ib( default=None, - validator=attr.validators.optional(attr.validators.instance_of(str)), + validator=[ + attr.validators.optional(attr.validators.instance_of(str)), + _check_client_secret, + ], ) """ The client secret to use when calling the introspection endpoint, when using any of the client_secret_* client auth methods. """ + _client_secret_path: Optional[str] = attr.ib( + default=None, + validator=[ + attr.validators.optional(attr.validators.instance_of(str)), + _check_client_secret, + ], + ) + """ + Alternative to `client_secret`: allows the secret to be specified in an + external file. + """ + jwk: Optional["JsonWebKey"] = attr.ib(default=None, converter=_parse_jwks) """ The JWKS to use when calling the introspection endpoint, @@ -133,7 +165,7 @@ def _check_client_auth_method( ClientAuthMethod.CLIENT_SECRET_BASIC, ClientAuthMethod.CLIENT_SECRET_JWT, ) - and self.client_secret is None + and self.client_secret() is None ): raise ConfigError( f"A client secret must be provided when using the {value} client auth method", @@ -161,6 +193,18 @@ def _check_client_auth_method( This is used by the OIDC provider, to make admin calls to Synapse. """ + def client_secret(self) -> Optional[str]: + """Returns the client secret. + + If client_secret_path is given, the secret is always read from file. + """ + if self._client_secret_path: + return read_file( + self._client_secret_path, + ("experimental_features", "msc3861", "client_secret_path"), + ).strip() + return self._client_secret + def check_config_conflicts(self, root: RootConfig) -> None: """Checks for any configuration conflicts with other parts of Synapse. diff --git a/tests/config/test_load.py b/tests/config/test_load.py index c5dee06af55..4ef95e38269 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -131,6 +131,7 @@ def test_depreciated_identity_server_flag_throws_error(self) -> None: [ "turn_shared_secret_path: /does/not/exist", "registration_shared_secret_path: /does/not/exist", + "experimental_features:\n msc3861:\n client_secret_path: /does/not/exist", *["redis:\n enabled: true\n password_path: /does/not/exist"] * (hiredis is not None), ] @@ -152,6 +153,10 @@ def test_secret_files_missing(self, config_str: str) -> None: "registration_shared_secret_path: {}", lambda c: c.registration.registration_shared_secret, ), + ( + "experimental_features:\n msc3861:\n client_secret_path: {}", + lambda c: c.experimental.msc3861.client_secret(), + ), *[ ( "redis:\n enabled: true\n password_path: {}", From e46187d6bbce344a693f1ba641774990de3f3221 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Fri, 6 Dec 2024 13:11:05 +0100 Subject: [PATCH 2/5] experimental_features.msc3861.admin_token_path --- synapse/api/auth/msc3861_delegated.py | 9 ++--- synapse/config/experimental.py | 48 +++++++++++++++++++++++++-- tests/config/test_load.py | 5 +++ 3 files changed, 56 insertions(+), 6 deletions(-) diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index f1eb7754d7f..87710d64deb 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -19,7 +19,7 @@ # # import logging -from typing import TYPE_CHECKING, Any, Dict, List, Optional +from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional from urllib.parse import urlencode from authlib.oauth2 import ClientAuth @@ -119,7 +119,7 @@ def __init__(self, hs: "HomeServer"): self._clock = hs.get_clock() self._http_client = hs.get_proxied_http_client() self._hostname = hs.hostname - self._admin_token = self._config.admin_token + self._admin_token: Callable[[], Optional[str]] = self._config.admin_token self._issuer_metadata = RetryOnExceptionCachedCall[OpenIDProviderMetadata]( self._load_metadata @@ -278,7 +278,7 @@ async def get_user_by_req( requester = await self.get_user_by_access_token(access_token, allow_expired) # Do not record requests from MAS using the virtual `__oidc_admin` user. - if access_token != self._admin_token: + if access_token != self._admin_token(): await self._record_request(request, requester) if not allow_guest and requester.is_guest: @@ -319,7 +319,8 @@ async def get_user_by_access_token( token: str, allow_expired: bool = False, ) -> Requester: - if self._admin_token is not None and token == self._admin_token: + admin_token = self._admin_token() + if admin_token is not None and token == admin_token: # XXX: This is a temporary solution so that the admin API can be called by # the OIDC provider. This will be removed once we have OIDC client # credentials grant support in matrix-authentication-service. diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 6ef25781b06..d7ae453011d 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -80,6 +80,23 @@ def _check_client_secret( instance.client_secret() +def _check_admin_token( + instance: "MSC3861", _attribute: attr.Attribute, _value: Optional[str] +) -> None: + if instance._admin_token and instance._admin_token_path: + raise ConfigError( + ( + "You have configured both " + "`experimental_features.msc3861.admin_token` and " + "`experimental_features.msc3861.admin_token_path`. " + "These are mutually incompatible." + ), + ("experimental", "msc3861", "admin_token"), + ) + # Check client secret can be retrieved + instance.admin_token() + + @attr.s(slots=True, frozen=True) class MSC3861: """Configuration for MSC3861: Matrix architecture change to delegate authentication via OIDC""" @@ -184,15 +201,30 @@ def _check_client_auth_method( ) """The URL of the My Account page on the OIDC Provider as per MSC2965.""" - admin_token: Optional[str] = attr.ib( + _admin_token: Optional[str] = attr.ib( default=None, - validator=attr.validators.optional(attr.validators.instance_of(str)), + validator=[ + attr.validators.optional(attr.validators.instance_of(str)), + _check_admin_token, + ], ) """ A token that should be considered as an admin token. This is used by the OIDC provider, to make admin calls to Synapse. """ + _admin_token_path: Optional[str] = attr.ib( + default=None, + validator=[ + attr.validators.optional(attr.validators.instance_of(str)), + _check_admin_token, + ], + ) + """ + Alternative to `admin_token`: allows the secret to be specified in an + external file. + """ + def client_secret(self) -> Optional[str]: """Returns the client secret. @@ -205,6 +237,18 @@ def client_secret(self) -> Optional[str]: ).strip() return self._client_secret + def admin_token(self) -> Optional[str]: + """Returns the admin token. + + If admin_token_path is given, the token is always read from file. + """ + if self._admin_token_path: + return read_file( + self._admin_token_path, + ("experimental_features", "msc3861", "admin_token_path"), + ).strip() + return self._admin_token + def check_config_conflicts(self, root: RootConfig) -> None: """Checks for any configuration conflicts with other parts of Synapse. diff --git a/tests/config/test_load.py b/tests/config/test_load.py index 4ef95e38269..738f9f8ec77 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -132,6 +132,7 @@ def test_depreciated_identity_server_flag_throws_error(self) -> None: "turn_shared_secret_path: /does/not/exist", "registration_shared_secret_path: /does/not/exist", "experimental_features:\n msc3861:\n client_secret_path: /does/not/exist", + "experimental_features:\n msc3861:\n admin_token_path: /does/not/exist", *["redis:\n enabled: true\n password_path: /does/not/exist"] * (hiredis is not None), ] @@ -157,6 +158,10 @@ def test_secret_files_missing(self, config_str: str) -> None: "experimental_features:\n msc3861:\n client_secret_path: {}", lambda c: c.experimental.msc3861.client_secret(), ), + ( + "experimental_features:\n msc3861:\n admin_token_path: {}", + lambda c: c.experimental.msc3861.admin_token(), + ), *[ ( "redis:\n enabled: true\n password_path: {}", From fbebb1d85a66d20c7d18be8b9f87d631d72feaf3 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Fri, 6 Dec 2024 13:15:20 +0100 Subject: [PATCH 3/5] Add newsfragment --- changelog.d/18004.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/18004.feature diff --git a/changelog.d/18004.feature b/changelog.d/18004.feature new file mode 100644 index 00000000000..8cacd1a0efd --- /dev/null +++ b/changelog.d/18004.feature @@ -0,0 +1 @@ +Add experimental config options `admin_token_path` and `client_secret_path` for MSC 3861. \ No newline at end of file From 5da34d5f4145f0b8958c17767255b139f172a9c6 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Mon, 3 Feb 2025 16:23:37 +0100 Subject: [PATCH 4/5] Fix missing function call --- tests/handlers/test_oauth_delegation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_oauth_delegation.py b/tests/handlers/test_oauth_delegation.py index 5f73469daa4..ba2f8ff5103 100644 --- a/tests/handlers/test_oauth_delegation.py +++ b/tests/handlers/test_oauth_delegation.py @@ -795,7 +795,7 @@ async def mock_http_client_request( req = SynapseRequest(channel, self.site) # type: ignore[arg-type] req.client.host = MAS_IPV4_ADDR req.requestHeaders.addRawHeader( - "Authorization", f"Bearer {self.auth._admin_token}" + "Authorization", f"Bearer {self.auth._admin_token()}" ) req.requestHeaders.addRawHeader("User-Agent", MAS_USER_AGENT) req.content = BytesIO(b"") From ebbfcca714c54ea3ff97ed67723ee8a02f992597 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Mon, 3 Feb 2025 14:21:41 +0100 Subject: [PATCH 5/5] Read secrets from file only once at startup --- synapse/config/experimental.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index d7ae453011d..5735bfe706d 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -20,7 +20,8 @@ # import enum -from typing import TYPE_CHECKING, Any, Optional +from functools import cache +from typing import TYPE_CHECKING, Any, Iterable, Optional import attr import attr.validators @@ -43,6 +44,12 @@ from authlib.jose.rfc7517 import JsonWebKey +@cache +def read_secret_from_file_once(file_path: Any, config_path: Iterable[str]) -> str: + """Returns the memoized secret read from file.""" + return read_file(file_path, config_path).strip() + + class ClientAuthMethod(enum.Enum): """List of supported client auth methods.""" @@ -226,27 +233,21 @@ def _check_client_auth_method( """ def client_secret(self) -> Optional[str]: - """Returns the client secret. - - If client_secret_path is given, the secret is always read from file. - """ + """Returns the secret given via `client_secret` or `client_secret_path`.""" if self._client_secret_path: - return read_file( + return read_secret_from_file_once( self._client_secret_path, ("experimental_features", "msc3861", "client_secret_path"), - ).strip() + ) return self._client_secret def admin_token(self) -> Optional[str]: - """Returns the admin token. - - If admin_token_path is given, the token is always read from file. - """ + """Returns the admin token given via `admin_token` or `admin_token_path`.""" if self._admin_token_path: - return read_file( + return read_secret_from_file_once( self._admin_token_path, ("experimental_features", "msc3861", "admin_token_path"), - ).strip() + ) return self._admin_token def check_config_conflicts(self, root: RootConfig) -> None: