From 57bf44941e52f09dc7ea21acdbe20633b7449f5a Mon Sep 17 00:00:00 2001 From: V02460 Date: Tue, 17 Dec 2024 01:01:33 +0100 Subject: [PATCH] Add `macaroon_secret_key_path` config option (#17983) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Another config option on my quest to a `*_path` variant for every secret. This time it’s `macaroon_secret_key_path`. 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/17983.feature | 1 + .../configuration/config_documentation.md | 16 +++++++++++++ synapse/config/key.py | 21 +++++++++++++---- tests/config/test_load.py | 23 ++++++++++++------- tests/config/utils.py | 5 ++-- 5 files changed, 51 insertions(+), 15 deletions(-) create mode 100644 changelog.d/17983.feature diff --git a/changelog.d/17983.feature b/changelog.d/17983.feature new file mode 100644 index 0000000000..2c54c80c40 --- /dev/null +++ b/changelog.d/17983.feature @@ -0,0 +1 @@ +Add `macaroon_secret_key_path` config option. \ No newline at end of file diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 7a48d76bbb..98ceb878e2 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3091,6 +3091,22 @@ Example configuration: ```yaml macaroon_secret_key: ``` +--- +### `macaroon_secret_key_path` + +An alternative to [`macaroon_secret_key`](#macaroon_secret_key): +allows the secret key to be specified in an external file. + +The file should be a plain text file, containing only the secret key. +Synapse reads the secret key from the given file once at startup. + +Example configuration: +```yaml +macaroon_secret_key_path: /path/to/secrets/file +``` + +_Added in Synapse 1.121.0._ + --- ### `form_secret` diff --git a/synapse/config/key.py b/synapse/config/key.py index bc96888967..01aae09c13 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -43,7 +43,7 @@ from unpaddedbase64 import decode_base64 from synapse.types import JsonDict from synapse.util.stringutils import random_string, random_string_with_symbols -from ._base import Config, ConfigError +from ._base import Config, ConfigError, read_file if TYPE_CHECKING: from signedjson.key import VerifyKeyWithExpiry @@ -91,6 +91,11 @@ To suppress this warning and continue using 'matrix.org', admins should set 'suppress_key_server_warning' to 'true' in homeserver.yaml. --------------------------------------------------------------------------------""" +CONFLICTING_MACAROON_SECRET_KEY_OPTS_ERROR = """\ +Conflicting options 'macaroon_secret_key' and 'macaroon_secret_key_path' are +both defined in config file. +""" + logger = logging.getLogger(__name__) @@ -166,10 +171,16 @@ class KeyConfig(Config): ) ) - macaroon_secret_key: Optional[str] = config.get( - "macaroon_secret_key", self.root.registration.registration_shared_secret - ) - + macaroon_secret_key = config.get("macaroon_secret_key") + macaroon_secret_key_path = config.get("macaroon_secret_key_path") + if macaroon_secret_key_path: + if macaroon_secret_key: + raise ConfigError(CONFLICTING_MACAROON_SECRET_KEY_OPTS_ERROR) + macaroon_secret_key = read_file( + macaroon_secret_key_path, "macaroon_secret_key_path" + ).strip() + if not macaroon_secret_key: + macaroon_secret_key = self.root.registration.registration_shared_secret if not macaroon_secret_key: # Unfortunately, there are people out there that don't have this # set. Lets just be "nice" and derive one from their secret key. diff --git a/tests/config/test_load.py b/tests/config/test_load.py index c5dee06af5..f8f7b72e40 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -39,7 +39,7 @@ except ImportError: class ConfigLoadingFileTestCase(ConfigFileTestCase): def test_load_fails_if_server_name_missing(self) -> None: - self.generate_config_and_remove_lines_containing("server_name") + self.generate_config_and_remove_lines_containing(["server_name"]) with self.assertRaises(ConfigError): HomeServerConfig.load_config("", ["-c", self.config_file]) with self.assertRaises(ConfigError): @@ -76,7 +76,7 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase): ) def test_load_succeeds_if_macaroon_secret_key_missing(self) -> None: - self.generate_config_and_remove_lines_containing("macaroon") + self.generate_config_and_remove_lines_containing(["macaroon"]) config1 = HomeServerConfig.load_config("", ["-c", self.config_file]) config2 = HomeServerConfig.load_config("", ["-c", self.config_file]) config3 = HomeServerConfig.load_or_generate_config("", ["-c", self.config_file]) @@ -111,7 +111,7 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase): self.assertTrue(config3.registration.enable_registration) def test_stats_enabled(self) -> None: - self.generate_config_and_remove_lines_containing("enable_metrics") + self.generate_config_and_remove_lines_containing(["enable_metrics"]) self.add_lines_to_config(["enable_metrics: true"]) # The default Metrics Flags are off by default. @@ -131,6 +131,7 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase): [ "turn_shared_secret_path: /does/not/exist", "registration_shared_secret_path: /does/not/exist", + "macaroon_secret_key_path: /does/not/exist", *["redis:\n enabled: true\n password_path: /does/not/exist"] * (hiredis is not None), ] @@ -146,16 +147,20 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase): [ ( "turn_shared_secret_path: {}", - lambda c: c.voip.turn_shared_secret, + lambda c: c.voip.turn_shared_secret.encode("utf-8"), ), ( "registration_shared_secret_path: {}", - lambda c: c.registration.registration_shared_secret, + lambda c: c.registration.registration_shared_secret.encode("utf-8"), + ), + ( + "macaroon_secret_key_path: {}", + lambda c: c.key.macaroon_secret_key, ), *[ ( "redis:\n enabled: true\n password_path: {}", - lambda c: c.redis.redis_password, + lambda c: c.redis.redis_password.encode("utf-8"), ) ] * (hiredis is not None), @@ -164,11 +169,13 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase): def test_secret_files_existing( self, config_line: str, get_secret: Callable[[RootConfig], str] ) -> None: - self.generate_config_and_remove_lines_containing("registration_shared_secret") + self.generate_config_and_remove_lines_containing( + ["registration_shared_secret", "macaroon_secret_key"] + ) with tempfile.NamedTemporaryFile(buffering=0) as secret_file: secret_file.write(b"53C237") self.add_lines_to_config(["", config_line.format(secret_file.name)]) config = HomeServerConfig.load_config("", ["-c", self.config_file]) - self.assertEqual(get_secret(config), "53C237") + self.assertEqual(get_secret(config), b"53C237") diff --git a/tests/config/utils.py b/tests/config/utils.py index 11140ff979..3cba4ac588 100644 --- a/tests/config/utils.py +++ b/tests/config/utils.py @@ -51,12 +51,13 @@ class ConfigFileTestCase(unittest.TestCase): ], ) - def generate_config_and_remove_lines_containing(self, needle: str) -> None: + def generate_config_and_remove_lines_containing(self, needles: list[str]) -> None: self.generate_config() with open(self.config_file) as f: contents = f.readlines() - contents = [line for line in contents if needle not in line] + for needle in needles: + contents = [line for line in contents if needle not in line] with open(self.config_file, "w") as f: f.write("".join(contents))