Add MSC3861 config options admin_token_path and client_secret_path #18004
Conversation
| 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 |
There was a problem hiding this comment.
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
ConfigErrorwill 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.
There was a problem hiding this comment.
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
-
I don’t blame you,
attrs, it’s simply not the task you were made for. ↩
There was a problem hiding this comment.
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.
Another PR on my quest to a
*_pathvariant for every secret. Adds two config optionsadmin_token_pathandclient_secret_pathto the experimental config underexperimental_features.msc3861. Also includes tests.I tried to be a good citizen here by following
attrsconventions and not rewriting the corresponding non-path variants in the class, but instead adding methods to retrieve the value.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. You might prefer another form of error handling than the current behavior.
A big pro of directly using the secret contents from file is that it enables secret rotation without having to restart Synapse.
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.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.(run the linters)