From 2159b3852e79103c791eda159c8895c3b5cf2ff5 Mon Sep 17 00:00:00 2001
From: V02460 <git@kaialexhiller.de>
Date: Tue, 25 Feb 2025 17:26:01 +0100
Subject: [PATCH] Add --no-secrets-in-config command line option (#18092)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Adds the `--no-secrets-in-config` command line option that makes Synapse
reject all configurations containing keys with in-line secret values.
Currently this rejects
- `turn_shared_secret`
- `registration_shared_secret`
- `macaroon_secret_key`
- `recaptcha_private_key`
- `recaptcha_public_key`
- `experimental_features.msc3861.client_secret`
- `experimental_features.msc3861.jwk`
- `experimental_features.msc3861.admin_token`
- `form_secret`
- `redis.password`
- `worker_replication_secret`
> [!TIP]
> Hey, you! Yes, you! 😊 If you think this list is missing an item,
please leave a comment below. Thanks :)
This PR complements my other PRs[^1] that add the corresponding `_path`
variants for this class of config options. It enables admins to enforce
a policy of no secrets in configuration files and guards against
accident and malice.
Because I consider the flag `--no-secrets-in-config` to be
security-relevant, I did not add a corresponding `--secrets-in-config`
flag; this way, if Synapse command line options are appended at various
places, there is no way to weaken the once-set setting with a succeeding
flag.
[^1]: [#17690](https://github.com/element-hq/synapse/pull/17690),
[#17717](https://github.com/element-hq/synapse/pull/17717),
[#17983](https://github.com/element-hq/synapse/pull/17983),
[#17984](https://github.com/element-hq/synapse/pull/17984),
[#18004](https://github.com/element-hq/synapse/pull/18004),
[#18090](https://github.com/element-hq/synapse/pull/18090)
### Pull Request Checklist
<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->
* [x] Pull request is based on the develop branch
* [x] Pull request includes a [changelog
file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog).
The entry should:
- Be a short description of your change which makes sense to users.
"Fixed a bug that prevented receiving messages from other servers."
instead of "Moved X method from `EventStore` to `EventWorkerStore`.".
- Use markdown where necessary, mostly for `code blocks`.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by
@github_username." or "Contributed by [Your Name]." to the end of the
entry.
* [x] [Code
style](https://element-hq.github.io/synapse/latest/code_style.html) is
correct
(run the
[linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))
---
changelog.d/18092.feature | 1 +
synapse/config/_base.py | 32 +++++++++-
synapse/config/_base.pyi | 6 +-
synapse/config/captcha.py | 14 ++++-
synapse/config/experimental.py | 30 +++++++++-
synapse/config/key.py | 16 ++++-
synapse/config/redis.py | 9 ++-
synapse/config/registration.py | 9 ++-
synapse/config/voip.py | 9 ++-
synapse/config/workers.py | 9 ++-
tests/config/test_load.py | 104 +++++++++++++++++++++++++++++++++
tests/config/test_workers.py | 2 +-
12 files changed, 227 insertions(+), 14 deletions(-)
create mode 100644 changelog.d/18092.feature
diff --git a/changelog.d/18092.feature b/changelog.d/18092.feature
new file mode 100644
index 0000000000..26371cc810
--- /dev/null
+++ b/changelog.d/18092.feature
@@ -0,0 +1 @@
+Add the `--no-secrets-in-config` command line option.
\ No newline at end of file
diff --git a/synapse/config/_base.py b/synapse/config/_base.py
index 912346a423..132ba26af9 100644
--- a/synapse/config/_base.py
+++ b/synapse/config/_base.py
@@ -589,6 +589,14 @@ class RootConfig:
" Defaults to the directory containing the last config file",
)
+ config_parser.add_argument(
+ "--no-secrets-in-config",
+ dest="secrets_in_config",
+ action="store_false",
+ default=True,
+ help="Reject config options that expect an in-line secret as value.",
+ )
+
cls.invoke_all_static("add_arguments", config_parser)
@classmethod
@@ -626,7 +634,10 @@ class RootConfig:
config_dict = read_config_files(config_files)
obj.parse_config_dict(
- config_dict, config_dir_path=config_dir_path, data_dir_path=data_dir_path
+ config_dict,
+ config_dir_path=config_dir_path,
+ data_dir_path=data_dir_path,
+ allow_secrets_in_config=config_args.secrets_in_config,
)
obj.invoke_all("read_arguments", config_args)
@@ -653,6 +664,13 @@ class RootConfig:
help="Specify config file. Can be given multiple times and"
" may specify directories containing *.yaml files.",
)
+ parser.add_argument(
+ "--no-secrets-in-config",
+ dest="secrets_in_config",
+ action="store_false",
+ default=True,
+ help="Reject config options that expect an in-line secret as value.",
+ )
# we nest the mutually-exclusive group inside another group so that the help
# text shows them in their own group.
@@ -821,14 +839,21 @@ class RootConfig:
return None
obj.parse_config_dict(
- config_dict, config_dir_path=config_dir_path, data_dir_path=data_dir_path
+ config_dict,
+ config_dir_path=config_dir_path,
+ data_dir_path=data_dir_path,
+ allow_secrets_in_config=config_args.secrets_in_config,
)
obj.invoke_all("read_arguments", config_args)
return obj
def parse_config_dict(
- self, config_dict: Dict[str, Any], config_dir_path: str, data_dir_path: str
+ self,
+ config_dict: Dict[str, Any],
+ config_dir_path: str,
+ data_dir_path: str,
+ allow_secrets_in_config: bool = True,
) -> None:
"""Read the information from the config dict into this Config object.
@@ -846,6 +871,7 @@ class RootConfig:
config_dict,
config_dir_path=config_dir_path,
data_dir_path=data_dir_path,
+ allow_secrets_in_config=allow_secrets_in_config,
)
def generate_missing_files(
diff --git a/synapse/config/_base.pyi b/synapse/config/_base.pyi
index d9cb0da38b..55b0e2cbf4 100644
--- a/synapse/config/_base.pyi
+++ b/synapse/config/_base.pyi
@@ -132,7 +132,11 @@ class RootConfig:
@classmethod
def invoke_all_static(cls, func_name: str, *args: Any, **kwargs: Any) -> None: ...
def parse_config_dict(
- self, config_dict: Dict[str, Any], config_dir_path: str, data_dir_path: str
+ self,
+ config_dict: Dict[str, Any],
+ config_dir_path: str,
+ data_dir_path: str,
+ allow_secrets_in_config: bool = ...,
) -> None: ...
def generate_config(
self,
diff --git a/synapse/config/captcha.py b/synapse/config/captcha.py
index 84897c09c5..57d67abbc3 100644
--- a/synapse/config/captcha.py
+++ b/synapse/config/captcha.py
@@ -29,8 +29,15 @@ from ._base import Config, ConfigError
class CaptchaConfig(Config):
section = "captcha"
- def read_config(self, config: JsonDict, **kwargs: Any) -> None:
+ def read_config(
+ self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
+ ) -> None:
recaptcha_private_key = config.get("recaptcha_private_key")
+ if recaptcha_private_key and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("recaptcha_private_key",),
+ )
if recaptcha_private_key is not None and not isinstance(
recaptcha_private_key, str
):
@@ -38,6 +45,11 @@ class CaptchaConfig(Config):
self.recaptcha_private_key = recaptcha_private_key
recaptcha_public_key = config.get("recaptcha_public_key")
+ if recaptcha_public_key and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("recaptcha_public_key",),
+ )
if recaptcha_public_key is not None and not isinstance(
recaptcha_public_key, str
):
diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py
index 3beaeb8869..0a963b121a 100644
--- a/synapse/config/experimental.py
+++ b/synapse/config/experimental.py
@@ -250,7 +250,9 @@ class MSC3861:
)
return self._admin_token
- def check_config_conflicts(self, root: RootConfig) -> None:
+ def check_config_conflicts(
+ self, root: RootConfig, allow_secrets_in_config: bool
+ ) -> None:
"""Checks for any configuration conflicts with other parts of Synapse.
Raises:
@@ -260,6 +262,24 @@ class MSC3861:
if not self.enabled:
return
+ if self._client_secret and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("experimental", "msc3861", "client_secret"),
+ )
+
+ if self.jwk and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("experimental", "msc3861", "jwk"),
+ )
+
+ if self._admin_token and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("experimental", "msc3861", "admin_token"),
+ )
+
if (
root.auth.password_enabled_for_reauth
or root.auth.password_enabled_for_login
@@ -350,7 +370,9 @@ class ExperimentalConfig(Config):
section = "experimental"
- def read_config(self, config: JsonDict, **kwargs: Any) -> None:
+ def read_config(
+ self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
+ ) -> None:
experimental = config.get("experimental_features") or {}
# MSC3026 (busy presence state)
@@ -494,7 +516,9 @@ class ExperimentalConfig(Config):
) from exc
# Check that none of the other config options conflict with MSC3861 when enabled
- self.msc3861.check_config_conflicts(self.root)
+ self.msc3861.check_config_conflicts(
+ self.root, allow_secrets_in_config=allow_secrets_in_config
+ )
self.msc4028_push_encrypted_events = experimental.get(
"msc4028_push_encrypted_events", False
diff --git a/synapse/config/key.py b/synapse/config/key.py
index 01aae09c13..6f99f39e81 100644
--- a/synapse/config/key.py
+++ b/synapse/config/key.py
@@ -112,7 +112,11 @@ class KeyConfig(Config):
section = "key"
def read_config(
- self, config: JsonDict, config_dir_path: str, **kwargs: Any
+ self,
+ config: JsonDict,
+ config_dir_path: str,
+ allow_secrets_in_config: bool,
+ **kwargs: Any,
) -> None:
# the signing key can be specified inline or in a separate file
if "signing_key" in config:
@@ -172,6 +176,11 @@ class KeyConfig(Config):
)
macaroon_secret_key = config.get("macaroon_secret_key")
+ if macaroon_secret_key and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("macaroon_secret_key",),
+ )
macaroon_secret_key_path = config.get("macaroon_secret_key_path")
if macaroon_secret_key_path:
if macaroon_secret_key:
@@ -193,6 +202,11 @@ class KeyConfig(Config):
# a secret which is used to calculate HMACs for form values, to stop
# falsification of values
self.form_secret = config.get("form_secret", None)
+ if self.form_secret and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("form_secret",),
+ )
def generate_config_section(
self,
diff --git a/synapse/config/redis.py b/synapse/config/redis.py
index 3f38fa11b0..948c95eef7 100644
--- a/synapse/config/redis.py
+++ b/synapse/config/redis.py
@@ -34,7 +34,9 @@ These are mutually incompatible.
class RedisConfig(Config):
section = "redis"
- def read_config(self, config: JsonDict, **kwargs: Any) -> None:
+ def read_config(
+ self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
+ ) -> None:
redis_config = config.get("redis") or {}
self.redis_enabled = redis_config.get("enabled", False)
@@ -48,6 +50,11 @@ class RedisConfig(Config):
self.redis_path = redis_config.get("path", None)
self.redis_dbid = redis_config.get("dbid", None)
self.redis_password = redis_config.get("password")
+ if self.redis_password and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("redis", "password"),
+ )
redis_password_path = redis_config.get("password_path")
if redis_password_path:
if self.redis_password:
diff --git a/synapse/config/registration.py b/synapse/config/registration.py
index c7f3e6d35e..3cf7031656 100644
--- a/synapse/config/registration.py
+++ b/synapse/config/registration.py
@@ -43,7 +43,9 @@ You have configured both `registration_shared_secret` and
class RegistrationConfig(Config):
section = "registration"
- def read_config(self, config: JsonDict, **kwargs: Any) -> None:
+ def read_config(
+ self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
+ ) -> None:
self.enable_registration = strtobool(
str(config.get("enable_registration", False))
)
@@ -68,6 +70,11 @@ class RegistrationConfig(Config):
# read the shared secret, either inline or from an external file
self.registration_shared_secret = config.get("registration_shared_secret")
+ if self.registration_shared_secret and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("registration_shared_secret",),
+ )
registration_shared_secret_path = config.get("registration_shared_secret_path")
if registration_shared_secret_path:
if self.registration_shared_secret:
diff --git a/synapse/config/voip.py b/synapse/config/voip.py
index 8614a41dd4..f33602d975 100644
--- a/synapse/config/voip.py
+++ b/synapse/config/voip.py
@@ -34,9 +34,16 @@ These are mutually incompatible.
class VoipConfig(Config):
section = "voip"
- def read_config(self, config: JsonDict, **kwargs: Any) -> None:
+ def read_config(
+ self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
+ ) -> None:
self.turn_uris = config.get("turn_uris", [])
self.turn_shared_secret = config.get("turn_shared_secret")
+ if self.turn_shared_secret and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("turn_shared_secret",),
+ )
turn_shared_secret_path = config.get("turn_shared_secret_path")
if turn_shared_secret_path:
if self.turn_shared_secret:
diff --git a/synapse/config/workers.py b/synapse/config/workers.py
index ab896be307..632cef46ce 100644
--- a/synapse/config/workers.py
+++ b/synapse/config/workers.py
@@ -218,7 +218,9 @@ class WorkerConfig(Config):
section = "worker"
- def read_config(self, config: JsonDict, **kwargs: Any) -> None:
+ def read_config(
+ self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
+ ) -> None:
self.worker_app = config.get("worker_app")
# Canonicalise worker_app so that master always has None
@@ -243,6 +245,11 @@ class WorkerConfig(Config):
# The shared secret used for authentication when connecting to the main synapse.
self.worker_replication_secret = config.get("worker_replication_secret", None)
+ if self.worker_replication_secret and not allow_secrets_in_config:
+ raise ConfigError(
+ "Config options that expect an in-line secret as value are disabled",
+ ("worker_replication_secret",),
+ )
self.worker_name = config.get("worker_name", self.worker_app)
self.instance_name = self.worker_name or MAIN_PROCESS_INSTANCE_NAME
diff --git a/tests/config/test_load.py b/tests/config/test_load.py
index 220ca23aa7..18fb2e0c2c 100644
--- a/tests/config/test_load.py
+++ b/tests/config/test_load.py
@@ -21,6 +21,7 @@
#
import tempfile
from typing import Callable
+from unittest import mock
import yaml
from parameterized import parameterized
@@ -31,6 +32,11 @@ from synapse.config.homeserver import HomeServerConfig
from tests.config.utils import ConfigFileTestCase
+try:
+ import authlib
+except ImportError:
+ authlib = None
+
try:
import hiredis
except ImportError:
@@ -189,3 +195,101 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase):
config = HomeServerConfig.load_config("", ["-c", self.config_file])
self.assertEqual(get_secret(config), b"53C237")
+
+ @parameterized.expand(
+ [
+ "turn_shared_secret: 53C237",
+ "registration_shared_secret: 53C237",
+ "macaroon_secret_key: 53C237",
+ "recaptcha_private_key: 53C237",
+ "recaptcha_public_key: ¬53C237",
+ "form_secret: 53C237",
+ "worker_replication_secret: 53C237",
+ *[
+ "experimental_features:\n"
+ " msc3861:\n"
+ " enabled: true\n"
+ " client_secret: 53C237"
+ ]
+ * (authlib is not None),
+ *[
+ "experimental_features:\n"
+ " msc3861:\n"
+ " enabled: true\n"
+ " client_auth_method: private_key_jwt\n"
+ ' jwk: {{"mock": "mock"}}'
+ ]
+ * (authlib is not None),
+ *[
+ "experimental_features:\n"
+ " msc3861:\n"
+ " enabled: true\n"
+ " admin_token: 53C237\n"
+ " client_secret_path: {secret_file}"
+ ]
+ * (authlib is not None),
+ *["redis:\n enabled: true\n password: 53C237"] * (hiredis is not None),
+ ]
+ )
+ def test_no_secrets_in_config(self, config_line: str) -> None:
+ if authlib is not None:
+ patcher = mock.patch("authlib.jose.rfc7517.JsonWebKey.import_key")
+ self.addCleanup(patcher.stop)
+ patcher.start()
+
+ with tempfile.NamedTemporaryFile(buffering=0) as secret_file:
+ # Only used for less mocking with admin_token
+ secret_file.write(b"53C237")
+
+ self.generate_config_and_remove_lines_containing(
+ ["form_secret", "macaroon_secret_key", "registration_shared_secret"]
+ )
+ # Check strict mode with no offenders.
+ HomeServerConfig.load_config(
+ "", ["-c", self.config_file, "--no-secrets-in-config"]
+ )
+ self.add_lines_to_config(
+ ["", config_line.format(secret_file=secret_file.name)]
+ )
+ # Check strict mode with a single offender.
+ with self.assertRaises(ConfigError):
+ HomeServerConfig.load_config(
+ "", ["-c", self.config_file, "--no-secrets-in-config"]
+ )
+
+ # Check lenient mode with a single offender.
+ HomeServerConfig.load_config("", ["-c", self.config_file])
+
+ def test_no_secrets_in_config_but_in_files(self) -> None:
+ with tempfile.NamedTemporaryFile(buffering=0) as secret_file:
+ secret_file.write(b"53C237")
+
+ self.generate_config_and_remove_lines_containing(
+ ["form_secret", "macaroon_secret_key", "registration_shared_secret"]
+ )
+ self.add_lines_to_config(
+ [
+ "",
+ f"turn_shared_secret_path: {secret_file.name}",
+ f"registration_shared_secret_path: {secret_file.name}",
+ f"macaroon_secret_key_path: {secret_file.name}",
+ f"recaptcha_private_key_path: {secret_file.name}",
+ f"recaptcha_public_key_path: {secret_file.name}",
+ f"form_secret_path: {secret_file.name}",
+ f"worker_replication_secret_path: {secret_file.name}",
+ *[
+ "experimental_features:\n"
+ " msc3861:\n"
+ " enabled: true\n"
+ f" admin_token_path: {secret_file.name}\n"
+ f" client_secret_path: {secret_file.name}\n"
+ # f" jwk_path: {secret_file.name}"
+ ]
+ * (authlib is not None),
+ *[f"redis:\n enabled: true\n password_path: {secret_file.name}"]
+ * (hiredis is not None),
+ ]
+ )
+ HomeServerConfig.load_config(
+ "", ["-c", self.config_file, "--no-secrets-in-config"]
+ )
diff --git a/tests/config/test_workers.py b/tests/config/test_workers.py
index 64c0285d01..3a21975b89 100644
--- a/tests/config/test_workers.py
+++ b/tests/config/test_workers.py
@@ -47,7 +47,7 @@ class WorkerDutyConfigTestCase(TestCase):
"worker_app": worker_app,
**extras,
}
- worker_config.read_config(worker_config_dict)
+ worker_config.read_config(worker_config_dict, allow_secrets_in_config=True)
return worker_config
def test_old_configs_master(self) -> None:
--
GitLab