mirror of
https://github.com/element-hq/synapse.git
synced 2025-03-14 09:45:51 +00:00
Add --no-secrets-in-config command line option (#18092)
Adds the `--no-secrets-in-config` command line option that makes Synapse reject all configurations containing keys with in-line secret values. Currently this rejects - `turn_shared_secret` - `registration_shared_secret` - `macaroon_secret_key` - `recaptcha_private_key` - `recaptcha_public_key` - `experimental_features.msc3861.client_secret` - `experimental_features.msc3861.jwk` - `experimental_features.msc3861.admin_token` - `form_secret` - `redis.password` - `worker_replication_secret` > [!TIP] > Hey, you! Yes, you! 😊 If you think this list is missing an item, please leave a comment below. Thanks :) This PR complements my other PRs[^1] that add the corresponding `_path` variants for this class of config options. It enables admins to enforce a policy of no secrets in configuration files and guards against accident and malice. Because I consider the flag `--no-secrets-in-config` to be security-relevant, I did not add a corresponding `--secrets-in-config` flag; this way, if Synapse command line options are appended at various places, there is no way to weaken the once-set setting with a succeeding flag. [^1]: [#17690](https://github.com/element-hq/synapse/pull/17690), [#17717](https://github.com/element-hq/synapse/pull/17717), [#17983](https://github.com/element-hq/synapse/pull/17983), [#17984](https://github.com/element-hq/synapse/pull/17984), [#18004](https://github.com/element-hq/synapse/pull/18004), [#18090](https://github.com/element-hq/synapse/pull/18090) ### Pull Request Checklist <!-- Please read https://element-hq.github.io/synapse/latest/development/contributing_guide.html before submitting your pull request --> * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))
This commit is contained in:
parent
5121f9210c
commit
2159b3852e
12 changed files with 227 additions and 14 deletions
1
changelog.d/18092.feature
Normal file
1
changelog.d/18092.feature
Normal file
|
@ -0,0 +1 @@
|
|||
Add the `--no-secrets-in-config` command line option.
|
|
@ -589,6 +589,14 @@ class RootConfig:
|
|||
" Defaults to the directory containing the last config file",
|
||||
)
|
||||
|
||||
config_parser.add_argument(
|
||||
"--no-secrets-in-config",
|
||||
dest="secrets_in_config",
|
||||
action="store_false",
|
||||
default=True,
|
||||
help="Reject config options that expect an in-line secret as value.",
|
||||
)
|
||||
|
||||
cls.invoke_all_static("add_arguments", config_parser)
|
||||
|
||||
@classmethod
|
||||
|
@ -626,7 +634,10 @@ class RootConfig:
|
|||
|
||||
config_dict = read_config_files(config_files)
|
||||
obj.parse_config_dict(
|
||||
config_dict, config_dir_path=config_dir_path, data_dir_path=data_dir_path
|
||||
config_dict,
|
||||
config_dir_path=config_dir_path,
|
||||
data_dir_path=data_dir_path,
|
||||
allow_secrets_in_config=config_args.secrets_in_config,
|
||||
)
|
||||
|
||||
obj.invoke_all("read_arguments", config_args)
|
||||
|
@ -653,6 +664,13 @@ class RootConfig:
|
|||
help="Specify config file. Can be given multiple times and"
|
||||
" may specify directories containing *.yaml files.",
|
||||
)
|
||||
parser.add_argument(
|
||||
"--no-secrets-in-config",
|
||||
dest="secrets_in_config",
|
||||
action="store_false",
|
||||
default=True,
|
||||
help="Reject config options that expect an in-line secret as value.",
|
||||
)
|
||||
|
||||
# we nest the mutually-exclusive group inside another group so that the help
|
||||
# text shows them in their own group.
|
||||
|
@ -821,14 +839,21 @@ class RootConfig:
|
|||
return None
|
||||
|
||||
obj.parse_config_dict(
|
||||
config_dict, config_dir_path=config_dir_path, data_dir_path=data_dir_path
|
||||
config_dict,
|
||||
config_dir_path=config_dir_path,
|
||||
data_dir_path=data_dir_path,
|
||||
allow_secrets_in_config=config_args.secrets_in_config,
|
||||
)
|
||||
obj.invoke_all("read_arguments", config_args)
|
||||
|
||||
return obj
|
||||
|
||||
def parse_config_dict(
|
||||
self, config_dict: Dict[str, Any], config_dir_path: str, data_dir_path: str
|
||||
self,
|
||||
config_dict: Dict[str, Any],
|
||||
config_dir_path: str,
|
||||
data_dir_path: str,
|
||||
allow_secrets_in_config: bool = True,
|
||||
) -> None:
|
||||
"""Read the information from the config dict into this Config object.
|
||||
|
||||
|
@ -846,6 +871,7 @@ class RootConfig:
|
|||
config_dict,
|
||||
config_dir_path=config_dir_path,
|
||||
data_dir_path=data_dir_path,
|
||||
allow_secrets_in_config=allow_secrets_in_config,
|
||||
)
|
||||
|
||||
def generate_missing_files(
|
||||
|
|
|
@ -132,7 +132,11 @@ class RootConfig:
|
|||
@classmethod
|
||||
def invoke_all_static(cls, func_name: str, *args: Any, **kwargs: Any) -> None: ...
|
||||
def parse_config_dict(
|
||||
self, config_dict: Dict[str, Any], config_dir_path: str, data_dir_path: str
|
||||
self,
|
||||
config_dict: Dict[str, Any],
|
||||
config_dir_path: str,
|
||||
data_dir_path: str,
|
||||
allow_secrets_in_config: bool = ...,
|
||||
) -> None: ...
|
||||
def generate_config(
|
||||
self,
|
||||
|
|
|
@ -29,8 +29,15 @@ from ._base import Config, ConfigError
|
|||
class CaptchaConfig(Config):
|
||||
section = "captcha"
|
||||
|
||||
def read_config(self, config: JsonDict, **kwargs: Any) -> None:
|
||||
def read_config(
|
||||
self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
|
||||
) -> None:
|
||||
recaptcha_private_key = config.get("recaptcha_private_key")
|
||||
if recaptcha_private_key and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("recaptcha_private_key",),
|
||||
)
|
||||
if recaptcha_private_key is not None and not isinstance(
|
||||
recaptcha_private_key, str
|
||||
):
|
||||
|
@ -38,6 +45,11 @@ class CaptchaConfig(Config):
|
|||
self.recaptcha_private_key = recaptcha_private_key
|
||||
|
||||
recaptcha_public_key = config.get("recaptcha_public_key")
|
||||
if recaptcha_public_key and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("recaptcha_public_key",),
|
||||
)
|
||||
if recaptcha_public_key is not None and not isinstance(
|
||||
recaptcha_public_key, str
|
||||
):
|
||||
|
|
|
@ -250,7 +250,9 @@ class MSC3861:
|
|||
)
|
||||
return self._admin_token
|
||||
|
||||
def check_config_conflicts(self, root: RootConfig) -> None:
|
||||
def check_config_conflicts(
|
||||
self, root: RootConfig, allow_secrets_in_config: bool
|
||||
) -> None:
|
||||
"""Checks for any configuration conflicts with other parts of Synapse.
|
||||
|
||||
Raises:
|
||||
|
@ -260,6 +262,24 @@ class MSC3861:
|
|||
if not self.enabled:
|
||||
return
|
||||
|
||||
if self._client_secret and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("experimental", "msc3861", "client_secret"),
|
||||
)
|
||||
|
||||
if self.jwk and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("experimental", "msc3861", "jwk"),
|
||||
)
|
||||
|
||||
if self._admin_token and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("experimental", "msc3861", "admin_token"),
|
||||
)
|
||||
|
||||
if (
|
||||
root.auth.password_enabled_for_reauth
|
||||
or root.auth.password_enabled_for_login
|
||||
|
@ -350,7 +370,9 @@ class ExperimentalConfig(Config):
|
|||
|
||||
section = "experimental"
|
||||
|
||||
def read_config(self, config: JsonDict, **kwargs: Any) -> None:
|
||||
def read_config(
|
||||
self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
|
||||
) -> None:
|
||||
experimental = config.get("experimental_features") or {}
|
||||
|
||||
# MSC3026 (busy presence state)
|
||||
|
@ -494,7 +516,9 @@ class ExperimentalConfig(Config):
|
|||
) from exc
|
||||
|
||||
# Check that none of the other config options conflict with MSC3861 when enabled
|
||||
self.msc3861.check_config_conflicts(self.root)
|
||||
self.msc3861.check_config_conflicts(
|
||||
self.root, allow_secrets_in_config=allow_secrets_in_config
|
||||
)
|
||||
|
||||
self.msc4028_push_encrypted_events = experimental.get(
|
||||
"msc4028_push_encrypted_events", False
|
||||
|
|
|
@ -112,7 +112,11 @@ class KeyConfig(Config):
|
|||
section = "key"
|
||||
|
||||
def read_config(
|
||||
self, config: JsonDict, config_dir_path: str, **kwargs: Any
|
||||
self,
|
||||
config: JsonDict,
|
||||
config_dir_path: str,
|
||||
allow_secrets_in_config: bool,
|
||||
**kwargs: Any,
|
||||
) -> None:
|
||||
# the signing key can be specified inline or in a separate file
|
||||
if "signing_key" in config:
|
||||
|
@ -172,6 +176,11 @@ class KeyConfig(Config):
|
|||
)
|
||||
|
||||
macaroon_secret_key = config.get("macaroon_secret_key")
|
||||
if macaroon_secret_key and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("macaroon_secret_key",),
|
||||
)
|
||||
macaroon_secret_key_path = config.get("macaroon_secret_key_path")
|
||||
if macaroon_secret_key_path:
|
||||
if macaroon_secret_key:
|
||||
|
@ -193,6 +202,11 @@ class KeyConfig(Config):
|
|||
# a secret which is used to calculate HMACs for form values, to stop
|
||||
# falsification of values
|
||||
self.form_secret = config.get("form_secret", None)
|
||||
if self.form_secret and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("form_secret",),
|
||||
)
|
||||
|
||||
def generate_config_section(
|
||||
self,
|
||||
|
|
|
@ -34,7 +34,9 @@ These are mutually incompatible.
|
|||
class RedisConfig(Config):
|
||||
section = "redis"
|
||||
|
||||
def read_config(self, config: JsonDict, **kwargs: Any) -> None:
|
||||
def read_config(
|
||||
self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
|
||||
) -> None:
|
||||
redis_config = config.get("redis") or {}
|
||||
self.redis_enabled = redis_config.get("enabled", False)
|
||||
|
||||
|
@ -48,6 +50,11 @@ 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")
|
||||
if self.redis_password and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("redis", "password"),
|
||||
)
|
||||
redis_password_path = redis_config.get("password_path")
|
||||
if redis_password_path:
|
||||
if self.redis_password:
|
||||
|
|
|
@ -43,7 +43,9 @@ You have configured both `registration_shared_secret` and
|
|||
class RegistrationConfig(Config):
|
||||
section = "registration"
|
||||
|
||||
def read_config(self, config: JsonDict, **kwargs: Any) -> None:
|
||||
def read_config(
|
||||
self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
|
||||
) -> None:
|
||||
self.enable_registration = strtobool(
|
||||
str(config.get("enable_registration", False))
|
||||
)
|
||||
|
@ -68,6 +70,11 @@ class RegistrationConfig(Config):
|
|||
|
||||
# read the shared secret, either inline or from an external file
|
||||
self.registration_shared_secret = config.get("registration_shared_secret")
|
||||
if self.registration_shared_secret and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("registration_shared_secret",),
|
||||
)
|
||||
registration_shared_secret_path = config.get("registration_shared_secret_path")
|
||||
if registration_shared_secret_path:
|
||||
if self.registration_shared_secret:
|
||||
|
|
|
@ -34,9 +34,16 @@ These are mutually incompatible.
|
|||
class VoipConfig(Config):
|
||||
section = "voip"
|
||||
|
||||
def read_config(self, config: JsonDict, **kwargs: Any) -> None:
|
||||
def read_config(
|
||||
self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
|
||||
) -> None:
|
||||
self.turn_uris = config.get("turn_uris", [])
|
||||
self.turn_shared_secret = config.get("turn_shared_secret")
|
||||
if self.turn_shared_secret and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("turn_shared_secret",),
|
||||
)
|
||||
turn_shared_secret_path = config.get("turn_shared_secret_path")
|
||||
if turn_shared_secret_path:
|
||||
if self.turn_shared_secret:
|
||||
|
|
|
@ -218,7 +218,9 @@ class WorkerConfig(Config):
|
|||
|
||||
section = "worker"
|
||||
|
||||
def read_config(self, config: JsonDict, **kwargs: Any) -> None:
|
||||
def read_config(
|
||||
self, config: JsonDict, allow_secrets_in_config: bool, **kwargs: Any
|
||||
) -> None:
|
||||
self.worker_app = config.get("worker_app")
|
||||
|
||||
# Canonicalise worker_app so that master always has None
|
||||
|
@ -243,6 +245,11 @@ class WorkerConfig(Config):
|
|||
|
||||
# The shared secret used for authentication when connecting to the main synapse.
|
||||
self.worker_replication_secret = config.get("worker_replication_secret", None)
|
||||
if self.worker_replication_secret and not allow_secrets_in_config:
|
||||
raise ConfigError(
|
||||
"Config options that expect an in-line secret as value are disabled",
|
||||
("worker_replication_secret",),
|
||||
)
|
||||
|
||||
self.worker_name = config.get("worker_name", self.worker_app)
|
||||
self.instance_name = self.worker_name or MAIN_PROCESS_INSTANCE_NAME
|
||||
|
|
|
@ -21,6 +21,7 @@
|
|||
#
|
||||
import tempfile
|
||||
from typing import Callable
|
||||
from unittest import mock
|
||||
|
||||
import yaml
|
||||
from parameterized import parameterized
|
||||
|
@ -31,6 +32,11 @@ from synapse.config.homeserver import HomeServerConfig
|
|||
|
||||
from tests.config.utils import ConfigFileTestCase
|
||||
|
||||
try:
|
||||
import authlib
|
||||
except ImportError:
|
||||
authlib = None
|
||||
|
||||
try:
|
||||
import hiredis
|
||||
except ImportError:
|
||||
|
@ -189,3 +195,101 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase):
|
|||
config = HomeServerConfig.load_config("", ["-c", self.config_file])
|
||||
|
||||
self.assertEqual(get_secret(config), b"53C237")
|
||||
|
||||
@parameterized.expand(
|
||||
[
|
||||
"turn_shared_secret: 53C237",
|
||||
"registration_shared_secret: 53C237",
|
||||
"macaroon_secret_key: 53C237",
|
||||
"recaptcha_private_key: 53C237",
|
||||
"recaptcha_public_key: ¬53C237",
|
||||
"form_secret: 53C237",
|
||||
"worker_replication_secret: 53C237",
|
||||
*[
|
||||
"experimental_features:\n"
|
||||
" msc3861:\n"
|
||||
" enabled: true\n"
|
||||
" client_secret: 53C237"
|
||||
]
|
||||
* (authlib is not None),
|
||||
*[
|
||||
"experimental_features:\n"
|
||||
" msc3861:\n"
|
||||
" enabled: true\n"
|
||||
" client_auth_method: private_key_jwt\n"
|
||||
' jwk: {{"mock": "mock"}}'
|
||||
]
|
||||
* (authlib is not None),
|
||||
*[
|
||||
"experimental_features:\n"
|
||||
" msc3861:\n"
|
||||
" enabled: true\n"
|
||||
" admin_token: 53C237\n"
|
||||
" client_secret_path: {secret_file}"
|
||||
]
|
||||
* (authlib is not None),
|
||||
*["redis:\n enabled: true\n password: 53C237"] * (hiredis is not None),
|
||||
]
|
||||
)
|
||||
def test_no_secrets_in_config(self, config_line: str) -> None:
|
||||
if authlib is not None:
|
||||
patcher = mock.patch("authlib.jose.rfc7517.JsonWebKey.import_key")
|
||||
self.addCleanup(patcher.stop)
|
||||
patcher.start()
|
||||
|
||||
with tempfile.NamedTemporaryFile(buffering=0) as secret_file:
|
||||
# Only used for less mocking with admin_token
|
||||
secret_file.write(b"53C237")
|
||||
|
||||
self.generate_config_and_remove_lines_containing(
|
||||
["form_secret", "macaroon_secret_key", "registration_shared_secret"]
|
||||
)
|
||||
# Check strict mode with no offenders.
|
||||
HomeServerConfig.load_config(
|
||||
"", ["-c", self.config_file, "--no-secrets-in-config"]
|
||||
)
|
||||
self.add_lines_to_config(
|
||||
["", config_line.format(secret_file=secret_file.name)]
|
||||
)
|
||||
# Check strict mode with a single offender.
|
||||
with self.assertRaises(ConfigError):
|
||||
HomeServerConfig.load_config(
|
||||
"", ["-c", self.config_file, "--no-secrets-in-config"]
|
||||
)
|
||||
|
||||
# Check lenient mode with a single offender.
|
||||
HomeServerConfig.load_config("", ["-c", self.config_file])
|
||||
|
||||
def test_no_secrets_in_config_but_in_files(self) -> None:
|
||||
with tempfile.NamedTemporaryFile(buffering=0) as secret_file:
|
||||
secret_file.write(b"53C237")
|
||||
|
||||
self.generate_config_and_remove_lines_containing(
|
||||
["form_secret", "macaroon_secret_key", "registration_shared_secret"]
|
||||
)
|
||||
self.add_lines_to_config(
|
||||
[
|
||||
"",
|
||||
f"turn_shared_secret_path: {secret_file.name}",
|
||||
f"registration_shared_secret_path: {secret_file.name}",
|
||||
f"macaroon_secret_key_path: {secret_file.name}",
|
||||
f"recaptcha_private_key_path: {secret_file.name}",
|
||||
f"recaptcha_public_key_path: {secret_file.name}",
|
||||
f"form_secret_path: {secret_file.name}",
|
||||
f"worker_replication_secret_path: {secret_file.name}",
|
||||
*[
|
||||
"experimental_features:\n"
|
||||
" msc3861:\n"
|
||||
" enabled: true\n"
|
||||
f" admin_token_path: {secret_file.name}\n"
|
||||
f" client_secret_path: {secret_file.name}\n"
|
||||
# f" jwk_path: {secret_file.name}"
|
||||
]
|
||||
* (authlib is not None),
|
||||
*[f"redis:\n enabled: true\n password_path: {secret_file.name}"]
|
||||
* (hiredis is not None),
|
||||
]
|
||||
)
|
||||
HomeServerConfig.load_config(
|
||||
"", ["-c", self.config_file, "--no-secrets-in-config"]
|
||||
)
|
||||
|
|
|
@ -47,7 +47,7 @@ class WorkerDutyConfigTestCase(TestCase):
|
|||
"worker_app": worker_app,
|
||||
**extras,
|
||||
}
|
||||
worker_config.read_config(worker_config_dict)
|
||||
worker_config.read_config(worker_config_dict, allow_secrets_in_config=True)
|
||||
return worker_config
|
||||
|
||||
def test_old_configs_master(self) -> None:
|
||||
|
|
Loading…
Add table
Reference in a new issue