From dc999b30e4b9b518748e8dd9ea32ad5e5143d475 Mon Sep 17 00:00:00 2001 From: Roman Donchenko Date: Thu, 31 Aug 2023 17:24:07 +0300 Subject: [PATCH] Fix a race condition with initial secret_key.py creation Currently, it's possible (even if unlikely) for multiple backend processes to create and use different Django `SECRET_KEY` values, because the following scenario is possible: * process 1: fail to import secret_key.py * process 2: fail to import secret_key.py * process 1: generate a new secret_key.py * process 1: import secret_key.py * process 2: generate a new secret_key.py * process 2: import secret_key.py Fix this by making it so that `secret_key.py` is created atomically, and never overwritten if it already exists. In addition, only generate the secret key if the import fails due to the module not being found, since other failure reasons suggest incorrect configuration or data corruption, and so require administrator attention. --- CHANGELOG.md | 2 ++ cvat/settings/base.py | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6e2c86867f7..c8435644fe7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Running deep learning models on non-jpeg compressed tif images () - Paddings on tasks/projects/models pages () - Memory leak in the logging system () +- A race condition during initial `secret_key.py` creation + () ### Security - TDB diff --git a/cvat/settings/base.py b/cvat/settings/base.py index eebcfeacbeea..c1cceeaace0b 100644 --- a/cvat/settings/base.py +++ b/cvat/settings/base.py @@ -21,6 +21,7 @@ import shutil import subprocess import sys +import tempfile from datetime import timedelta from distutils.util import strtobool from enum import Enum @@ -40,18 +41,42 @@ ALLOWED_HOSTS = os.environ.get('ALLOWED_HOSTS', 'localhost,127.0.0.1').split(',') INTERNAL_IPS = ['127.0.0.1'] -try: - sys.path.append(BASE_DIR) - from keys.secret_key import SECRET_KEY # pylint: disable=unused-import -except ImportError: +def generate_secret_key(): + """ + Creates secret_key.py in such a way that multiple processes calling + this will all end up with the same key (assuming that they share the + same "keys" directory). + """ from django.utils.crypto import get_random_string keys_dir = os.path.join(BASE_DIR, 'keys') if not os.path.isdir(keys_dir): os.mkdir(keys_dir) - with open(os.path.join(keys_dir, 'secret_key.py'), 'w') as f: + + secret_key_fname = 'secret_key.py' # nosec + + with tempfile.NamedTemporaryFile( + mode='wt', dir=keys_dir, prefix=secret_key_fname + ".", + ) as f: chars = 'abcdefghijklmnopqrstuvwxyz0123456789!@#$%^&*(-_=+)' f.write("SECRET_KEY = '{}'\n".format(get_random_string(50, chars))) + + # Make sure the file contents are written before we link to it + # from the final location. + f.flush() + + try: + os.link(f.name, os.path.join(keys_dir, secret_key_fname)) + except FileExistsError: + # Somebody else created the secret key first. + # Discard ours and use theirs. + pass + +try: + sys.path.append(BASE_DIR) + from keys.secret_key import SECRET_KEY # pylint: disable=unused-import +except ModuleNotFoundError: + generate_secret_key() from keys.secret_key import SECRET_KEY