From 059d0e7be8f39cc5f63a56fa71641eb8182867ba Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 11 Aug 2022 22:41:13 -0400 Subject: [PATCH] settings: Make SHARED_SECRET mandatory. This implements get_mandatory_secret that ensures SHARED_SECRET is set when we hit zerver.decorator.authenticate_notify. To avoid getting ZulipSettingsError when setting up the secrets, we set an environment variable DISABLE_MANDATORY_SECRET_CHECK to skip the check and default its value to an empty string. Signed-off-by: Zixuan James Li --- scripts/setup/generate_secrets.py | 1 + zerver/tests/test_email_mirror.py | 2 -- zerver/tests/test_event_system.py | 1 - zerver/tests/test_server_settings.py | 20 ++++++++++++++++++++ zproject/computed_settings.py | 3 ++- zproject/config.py | 17 ++++++++++++++++- 6 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 zerver/tests/test_server_settings.py diff --git a/scripts/setup/generate_secrets.py b/scripts/setup/generate_secrets.py index 061e714973..cba412a47a 100755 --- a/scripts/setup/generate_secrets.py +++ b/scripts/setup/generate_secrets.py @@ -11,6 +11,7 @@ from scripts.lib.zulip_tools import get_config, get_config_file setup_path() +os.environ["DISABLE_MANDATORY_SECRET_CHECK"] = "True" os.environ["DJANGO_SETTINGS_MODULE"] = "zproject.settings" import argparse diff --git a/zerver/tests/test_email_mirror.py b/zerver/tests/test_email_mirror.py index 9c6b738db9..b252ffed79 100644 --- a/zerver/tests/test_email_mirror.py +++ b/zerver/tests/test_email_mirror.py @@ -1392,7 +1392,6 @@ class TestScriptMTA(ZulipTestCase): mail_template = self.fixture_data("simple.txt", type="email") mail = mail_template.format(stream_to_address=stream_to_address, sender=sender) - assert settings.SHARED_SECRET is not None subprocess.run( [script, "-r", stream_to_address, "-s", settings.SHARED_SECRET, "-t"], input=mail, @@ -1408,7 +1407,6 @@ class TestScriptMTA(ZulipTestCase): stream_to_address = encode_email_address(stream) mail_template = self.fixture_data("simple.txt", type="email") mail = mail_template.format(stream_to_address=stream_to_address, sender=sender) - assert settings.SHARED_SECRET is not None p = subprocess.run( [script, "-s", settings.SHARED_SECRET, "-t"], input=mail, diff --git a/zerver/tests/test_event_system.py b/zerver/tests/test_event_system.py index 982314056a..93c660ed7a 100644 --- a/zerver/tests/test_event_system.py +++ b/zerver/tests/test_event_system.py @@ -220,7 +220,6 @@ class EventsEndpointTest(ZulipTestCase): self.assertEqual(str(access_denied_error.exception), "Access denied") self.assertEqual(access_denied_error.exception.http_status_code, 403) - assert settings.SHARED_SECRET is not None post_data["secret"] = settings.SHARED_SECRET req = HostRequestMock(post_data, tornado_handler=dummy_handler) req.META["REMOTE_ADDR"] = "127.0.0.1" diff --git a/zerver/tests/test_server_settings.py b/zerver/tests/test_server_settings.py new file mode 100644 index 0000000000..0b4e0c0770 --- /dev/null +++ b/zerver/tests/test_server_settings.py @@ -0,0 +1,20 @@ +import os +from unittest import mock + +from zerver.lib.test_classes import ZulipTestCase +from zproject import config + + +class ConfigTest(ZulipTestCase): + def test_get_mandatory_secret_succeed(self) -> None: + secret = config.get_mandatory_secret("shared_secret") + self.assertGreater(len(secret), 0) + + def test_get_mandatory_secret_failed(self) -> None: + with self.assertRaisesRegex(config.ZulipSettingsError, "nonexistent"): + config.get_mandatory_secret("nonexistent") + + def test_disable_mandatory_secret_check(self) -> None: + with mock.patch.dict(os.environ, {"DISABLE_MANDATORY_SECRET_CHECK": "True"}): + secret = config.get_mandatory_secret("nonexistent") + self.assertEqual(secret, "") diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index 4aaf2b958d..ec84882713 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -19,6 +19,7 @@ from .config import ( config_file, get_config, get_from_file_if_exists, + get_mandatory_secret, get_secret, ) from .configured_settings import ( @@ -75,7 +76,7 @@ from .configured_settings import ( SECRET_KEY = get_secret("secret_key") # A shared secret, used to authenticate different parts of the app to each other. -SHARED_SECRET = get_secret("shared_secret") +SHARED_SECRET = get_mandatory_secret("shared_secret") # We use this salt to hash a user's email into a filename for their user-uploaded # avatar. If this salt is discovered, attackers will only be able to determine diff --git a/zproject/config.py b/zproject/config.py index 42ec637b74..70130c262f 100644 --- a/zproject/config.py +++ b/zproject/config.py @@ -2,6 +2,13 @@ import configparser import os from typing import Optional, overload +from django.core.exceptions import ImproperlyConfigured + + +class ZulipSettingsError(ImproperlyConfigured): + pass + + DEPLOY_ROOT = os.path.realpath(os.path.dirname(os.path.dirname(__file__))) config_file = configparser.RawConfigParser() @@ -10,7 +17,6 @@ config_file.read("/etc/zulip/zulip.conf") # Whether this instance of Zulip is running in a production environment. PRODUCTION = config_file.has_option("machine", "deploy_type") DEVELOPMENT = not PRODUCTION - secrets_file = configparser.RawConfigParser() if PRODUCTION: secrets_file.read("/etc/zulip/zulip-secrets.conf") @@ -38,6 +44,15 @@ def get_secret( return secrets_file.get("secrets", key, fallback=default_value) +def get_mandatory_secret(key: str) -> str: + secret = get_secret(key) + if secret is None: + if os.environ.get("DISABLE_MANDATORY_SECRET_CHECK") == "True": + return "" + raise ZulipSettingsError(f'Mandatory secret "{key}" is not set') + return secret + + @overload def get_config(section: str, key: str, default_value: str) -> str: ...