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

+Add experimental config options `admin_token_path` and `client_secret_path` for MSC 3861.
\ No newline at end of file

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.

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.
 

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: {}",

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"")