Add config option redis.password_path (#17717)

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

@@ -0,0 +1 @@
+Add config option `redis.password_path`.

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: <secret_password>
+  password_path: <path_to_the_password_file>
+  # OR password: <secret_password>
   dbid: <dbid>
   #use_tls: True
   #certificate_file: <path_to_the_certificate_file>

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 @@ 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")
+        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)

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 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase):
         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")