From e8e0f0fad71ef253e08c9f7dab1b2837d5f2334d Mon Sep 17 00:00:00 2001 From: V02460 Date: Mon, 7 Oct 2024 10:46:51 +0200 Subject: [PATCH] Add config option redis.password_path (#17717) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the option to load the Redis password from a file, instead of giving it in the config directly. The code is similar to how it’s done for `registration_shared_secret_path`. I changed the example in the documentation to represent the best practice regarding the handling of secrets. Reading secrets from files has the security advantage of separating the secrets from the config. It also simplifies secrets management in Kubernetes. --- changelog.d/17717.feature | 1 + .../configuration/config_documentation.md | 8 ++- synapse/config/redis.py | 18 +++++- tests/config/test_load.py | 56 +++++++++++++++++++ 4 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 changelog.d/17717.feature diff --git a/changelog.d/17717.feature b/changelog.d/17717.feature new file mode 100644 index 00000000000..292c99ccc55 --- /dev/null +++ b/changelog.d/17717.feature @@ -0,0 +1 @@ +Add config option `redis.password_path`. \ No newline at end of file diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 08eedc03b71..29f3528c7e1 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -4524,6 +4524,9 @@ This setting has the following sub-options: * `path`: The full path to a local Unix socket file. **If this is used, `host` and `port` are ignored.** Defaults to `/tmp/redis.sock' * `password`: Optional password if configured on the Redis instance. +* `password_path`: Alternative to `password`, reading the password from an + external file. The file should be a plain text file, containing only the + password. Synapse reads the password from the given file once at startup. * `dbid`: Optional redis dbid if needs to connect to specific redis logical db. * `use_tls`: Whether to use tls connection. Defaults to false. * `certificate_file`: Optional path to the certificate file @@ -4537,13 +4540,16 @@ This setting has the following sub-options: _Changed in Synapse 1.85.0: Added path option to use a local Unix socket_ + _Changed in Synapse 1.116.0: Added password\_path_ + Example configuration: ```yaml redis: enabled: true host: localhost port: 6379 - password: + password_path: + # OR password: dbid: #use_tls: True #certificate_file: diff --git a/synapse/config/redis.py b/synapse/config/redis.py index f1405380882..3f38fa11b0a 100644 --- a/synapse/config/redis.py +++ b/synapse/config/redis.py @@ -21,10 +21,15 @@ from typing import Any -from synapse.config._base import Config +from synapse.config._base import Config, ConfigError, read_file from synapse.types import JsonDict from synapse.util.check_dependencies import check_requirements +CONFLICTING_PASSWORD_OPTS_ERROR = """\ +You have configured both `redis.password` and `redis.password_path`. +These are mutually incompatible. +""" + class RedisConfig(Config): section = "redis" @@ -43,6 +48,17 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.redis_path = redis_config.get("path", None) self.redis_dbid = redis_config.get("dbid", None) self.redis_password = redis_config.get("password") + redis_password_path = redis_config.get("password_path") + if redis_password_path: + if self.redis_password: + raise ConfigError(CONFLICTING_PASSWORD_OPTS_ERROR) + self.redis_password = read_file( + redis_password_path, + ( + "redis", + "password_path", + ), + ).strip() self.redis_use_tls = redis_config.get("use_tls", False) self.redis_certificate = redis_config.get("certificate_file", None) diff --git a/tests/config/test_load.py b/tests/config/test_load.py index 479d2aab914..c5dee06af55 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -19,13 +19,23 @@ # [This file includes modifications made by New Vector Limited] # # +import tempfile +from typing import Callable + import yaml +from parameterized import parameterized from synapse.config import ConfigError +from synapse.config._base import RootConfig from synapse.config.homeserver import HomeServerConfig from tests.config.utils import ConfigFileTestCase +try: + import hiredis +except ImportError: + hiredis = None # type: ignore + class ConfigLoadingFileTestCase(ConfigFileTestCase): def test_load_fails_if_server_name_missing(self) -> None: @@ -116,3 +126,49 @@ def test_depreciated_identity_server_flag_throws_error(self) -> None: self.add_lines_to_config(["trust_identity_server_for_password_resets: true"]) with self.assertRaises(ConfigError): HomeServerConfig.load_config("", ["-c", self.config_file]) + + @parameterized.expand( + [ + "turn_shared_secret_path: /does/not/exist", + "registration_shared_secret_path: /does/not/exist", + *["redis:\n enabled: true\n password_path: /does/not/exist"] + * (hiredis is not None), + ] + ) + def test_secret_files_missing(self, config_str: str) -> None: + self.generate_config() + self.add_lines_to_config(["", config_str]) + + with self.assertRaises(ConfigError): + HomeServerConfig.load_config("", ["-c", self.config_file]) + + @parameterized.expand( + [ + ( + "turn_shared_secret_path: {}", + lambda c: c.voip.turn_shared_secret, + ), + ( + "registration_shared_secret_path: {}", + lambda c: c.registration.registration_shared_secret, + ), + *[ + ( + "redis:\n enabled: true\n password_path: {}", + lambda c: c.redis.redis_password, + ) + ] + * (hiredis is not 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") + 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")