Add macaroon_secret_key_path config option (#17983)

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

@@ -0,0 +1 @@
+Add `macaroon_secret_key_path` config option.

docs/usage/configuration/config_documentation.md

@@ -3091,6 +3091,22 @@ Example configuration:
 ```yaml
 macaroon_secret_key: <PRIVATE STRING>
 ```
+---
+### `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`
 

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 = config.get("macaroon_secret_key")
-            "macaroon_secret_key", self.root.registration.registration_shared_secret
+        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.

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

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))