Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MSC3861 config options admin_token_path and client_secret_path #18004

Merged
merged 6 commits into from
Feb 4, 2025
Merged

Add MSC3861 config options admin_token_path and client_secret_path #18004

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
09e7a1e
experimental_features.msc3861.client_secret_path
V02460 Dec 6, 2024
e46187d
experimental_features.msc3861.admin_token_path
V02460 Dec 6, 2024
fbebb1d
Add newsfragment
V02460 Dec 6, 2024
5da34d5
Fix missing function call
V02460 Feb 3, 2025
ebbfcca
Read secrets from file only once at startup
V02460 Feb 3, 2025
87be544
Merge branch 'develop' into msc3861-secrets
V02460 Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/18004.feature
View file
Open in desktop
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add experimental config options `admin_token_path` and `client_secret_path` for MSC 3861.
14 changes: 8 additions & 6 deletions synapse/api/auth/msc3861_delegated.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved

self._issuer_metadata = RetryOnExceptionCachedCall[OpenIDProviderMetadata](
self._load_metadata
Expand All @@ -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:
Expand Down Expand Up @@ -277,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:
Expand Down Expand Up @@ -318,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.
Expand Down
100 changes: 94 additions & 6 deletions synapse/config/experimental.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -63,6 +63,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"""
Expand Down Expand Up @@ -97,15 +131,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,
Expand Down Expand Up @@ -133,7 +182,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",
Expand All @@ -152,15 +201,54 @@ 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.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is noteworthy that in this patch each access to the client secret or the admin token will re-read it from file. In the current state a ConfigError will be raised at runtime when the file can not be read anymore.

Is there a different idiomatic pattern with attrs where we can just read from the file once?

It looks like we just access config.client_secret() once in __init__() in the usage but I don't think we should make downstream consumers think about having to cache this value because of the expensive/slow file read each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context in which I made this design decision is the following: I’ve started thinking about secret rotation in Synapse and would like to have it supported eventually. For that to work, the consumers of secrets have to know about updates to the secret in use (and sometimes previous secrets as well).

I find the small scope of this change (together with the unwieldiness of attrs1) a good place to get a first taste of how updating secrets might work.

Regarding your concern about performance: I’d expect such a small file to be cached by the OS and very quick to be accessed. If that would not be the case, memory mapping or the use of inotify would be a more full-fledged solution. But does this MR actually cause any performance regression?

Footnotes

  1. I don’t blame you, attrs, it’s simply not the task you were made for. ↩

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve started thinking about secret rotation in Synapse and would like to have it supported eventually

This feels like a pre-optimization and something that needs discussion on whether we even want to support it and strategies for how best to tackle.

Regarding your concern about performance:

I don't think it's a performance problem in its current form. I just don't think that downstream consumers using config.client_secret() should have to think about it. It should just always be an in-memory thing after startup.

For example, if we wanted to support secret rotation, we could have a watcher on the file that would only update when the file changes (not read the file each time). But this is just another example of a strategy that should be discussed in a separate, dedicated secret rotation discussion. And why I don't think we should attempt to figure it out with this PR.

In any case, I think we should figure out how best to read the file once on startup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated patch set now only reads in the files once at startup.

To allow consumers access to the raw values close to what is written in the config, the attributes _{client_secret,admin_token}{,_path} are part of the dataclass – with leading underscore to underline their lower-level nature. The methods client_secret and admin_token on the other hand provide convenient access for a consumer to the secret, independently of where it initially came from.

You are right, I think those maybe-future plans should not impede the best solution to the problem at hand. It was not my intention to overload the PR with a discussion about secret rotation. In the meantime I had some helpful and structuring conversations about it at FOSDEM, and I will write up an issue to discuss it further.


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.

Expand Down
10 changes: 10 additions & 0 deletions tests/config/test_load.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ 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),
]
Expand All @@ -152,6 +154,14 @@ 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(),
),
(
"experimental_features:\n msc3861:\n admin_token_path: {}",
lambda c: c.experimental.msc3861.admin_token(),
),
*[
(
"redis:\n enabled: true\n password_path: {}",
Expand Down
Loading