From 5257e3b215e6bbd28f16334188e32684fe75a204 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Mon, 2 Dec 2024 15:37:58 +0100 Subject: [PATCH 1/4] Add macaroon_secret_key_path config option --- changelog.d/17983.feature | 1 + .../configuration/config_documentation.md | 16 ++++++++++++++ synapse/config/key.py | 21 ++++++++++++++----- 3 files changed, 33 insertions(+), 5 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 00000000000..2c54c80c407 --- /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 7a48d76bbb1..98ceb878e25 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 bc968889676..01aae09c135 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -43,7 +43,7 @@ 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 @@ '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 @@ def read_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. From 9367d18eba0a3bbebe2c988ff644b52897a1fa21 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Mon, 2 Dec 2024 15:39:46 +0100 Subject: [PATCH 2/4] tests: config generaton allow multiple needles --- tests/config/test_load.py | 8 ++++---- tests/config/utils.py | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/config/test_load.py b/tests/config/test_load.py index c5dee06af55..f5d5ba441a1 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -39,7 +39,7 @@ 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 @@ def test_generates_and_loads_macaroon_secret_key(self) -> None: ) 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 @@ def test_disable_registration(self) -> None: 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. @@ -164,7 +164,7 @@ def test_secret_files_missing(self, config_str: str) -> None: 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"]) with tempfile.NamedTemporaryFile(buffering=0) as secret_file: secret_file.write(b"53C237") diff --git a/tests/config/utils.py b/tests/config/utils.py index 11140ff9790..3cba4ac5889 100644 --- a/tests/config/utils.py +++ b/tests/config/utils.py @@ -51,12 +51,13 @@ def generate_config(self) -> None: ], ) - 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)) From 087ec0f74328f04897f0710e26458e0b0c43762a Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Mon, 2 Dec 2024 15:40:48 +0100 Subject: [PATCH 3/4] test_secret_files_existing: Check bytes not str --- tests/config/test_load.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/config/test_load.py b/tests/config/test_load.py index f5d5ba441a1..1fc8723fae7 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -146,16 +146,16 @@ def test_secret_files_missing(self, config_str: str) -> None: [ ( "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"), ), *[ ( "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), @@ -171,4 +171,4 @@ def test_secret_files_existing( 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") From 6f10405d3003fb0c67c08bb83533edd82458fb69 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Mon, 2 Dec 2024 15:43:45 +0100 Subject: [PATCH 4/4] Test macaroon_secret_key{,_path} --- tests/config/test_load.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/config/test_load.py b/tests/config/test_load.py index 1fc8723fae7..f8f7b72e401 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -131,6 +131,7 @@ def test_depreciated_identity_server_flag_throws_error(self) -> None: [ "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), ] @@ -152,6 +153,10 @@ def test_secret_files_missing(self, config_str: str) -> None: "registration_shared_secret_path: {}", 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: {}", @@ -164,7 +169,9 @@ def test_secret_files_missing(self, config_str: str) -> None: 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")