From e41174cae3a74a2e6f9cb1386eb6e0b5510d6fce Mon Sep 17 00:00:00 2001
From: V02460 <git@kaialexhiller.de>
Date: Tue, 4 Feb 2025 19:45:33 +0100
Subject: [PATCH] Add MSC3861 config options admin_token_path and
client_secret_path (#18004)
Another PR on my quest to a `*_path` variant for every secret. Adds two
config options `admin_token_path` and `client_secret_path` to the
experimental config under `experimental_features.msc3861`. Also includes
tests.
I tried to be a good citizen here by following `attrs` conventions and
not rewriting the corresponding non-path variants in the class, but
instead adding methods to retrieve the value.
Reading secrets from files has the security advantage of separating the
secrets from the config. It also simplifies secrets management in
Kubernetes. Also useful to NixOS users.
---
changelog.d/18004.feature | 1 +
synapse/api/auth/msc3861_delegated.py | 14 ++--
synapse/config/experimental.py | 103 ++++++++++++++++++++++--
tests/config/test_load.py | 10 +++
tests/handlers/test_oauth_delegation.py | 2 +-
5 files changed, 116 insertions(+), 14 deletions(-)
create mode 100644 changelog.d/18004.feature
diff --git a/changelog.d/18004.feature b/changelog.d/18004.feature
new file mode 100644
index 0000000000..8cacd1a0ef
--- /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
diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py
index 802ea51d18..f825b5c95e 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 @@ class MSC3861DelegatedAuth(BaseAuth):
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
@@ -133,9 +133,10 @@ class MSC3861DelegatedAuth(BaseAuth):
)
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:
@@ -283,7 +284,7 @@ class MSC3861DelegatedAuth(BaseAuth):
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:
@@ -324,7 +325,8 @@ class MSC3861DelegatedAuth(BaseAuth):
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 94a25c7ee8..3beaeb8869 100644
--- a/synapse/config/experimental.py
+++ b/synapse/config/experimental.py
@@ -20,14 +20,15 @@
#
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
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.
@@ -43,6 +44,12 @@ if TYPE_CHECKING:
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."""
@@ -63,6 +70,40 @@ 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()
+
+
+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"""
@@ -97,15 +138,30 @@ class MSC3861:
)
"""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 +189,7 @@ class MSC3861:
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",
@@ -152,15 +208,48 @@ class MSC3861:
)
"""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 secret given via `client_secret` or `client_secret_path`."""
+ if self._client_secret_path:
+ return read_secret_from_file_once(
+ self._client_secret_path,
+ ("experimental_features", "msc3861", "client_secret_path"),
+ )
+ return self._client_secret
+
+ def admin_token(self) -> Optional[str]:
+ """Returns the admin token given via `admin_token` or `admin_token_path`."""
+ if self._admin_token_path:
+ return read_secret_from_file_once(
+ self._admin_token_path,
+ ("experimental_features", "msc3861", "admin_token_path"),
+ )
+ 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 f8f7b72e40..220ca23aa7 100644
--- a/tests/config/test_load.py
+++ b/tests/config/test_load.py
@@ -132,6 +132,8 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase):
"turn_shared_secret_path: /does/not/exist",
"registration_shared_secret_path: /does/not/exist",
"macaroon_secret_key_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 +159,14 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase):
"macaroon_secret_key_path: {}",
lambda c: c.key.macaroon_secret_key,
),
+ (
+ "experimental_features:\n msc3861:\n client_secret_path: {}",
+ lambda c: c.experimental.msc3861.client_secret().encode("utf-8"),
+ ),
+ (
+ "experimental_features:\n msc3861:\n admin_token_path: {}",
+ lambda c: c.experimental.msc3861.admin_token().encode("utf-8"),
+ ),
*[
(
"redis:\n enabled: true\n password_path: {}",
diff --git a/tests/handlers/test_oauth_delegation.py b/tests/handlers/test_oauth_delegation.py
index 5f73469daa..ba2f8ff510 100644
--- a/tests/handlers/test_oauth_delegation.py
+++ b/tests/handlers/test_oauth_delegation.py
@@ -795,7 +795,7 @@ class MSC3861OAuthDelegation(HomeserverTestCase):
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"")
--
GitLab