Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed 30360 -- Support rotation of secret keys. #13618

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 25 additions & 2 deletions django/conf/__init__.py
Expand Up @@ -81,14 +81,32 @@ def __getattr__(self, name):
"""Return the value of a setting and cache it in self.__dict__."""
if self._wrapped is empty:
self._setup(name)

# Either of SECRET_KEY or SECRET_KEYS may be specified.
if name in {'SECRET_KEY', 'SECRET_KEYS'}:
secret_key = getattr(self._wrapped, 'SECRET_KEY', None)
secret_keys = getattr(self._wrapped, 'SECRET_KEYS', None)
# We can't check both, because override_settings.
# if secret_key and secret_keys:
# raise ImproperlyConfigured('Only one of SECRET_KEY and SECRET_KEYS can be specified, not both.')
if not secret_key and not secret_keys:
raise ImproperlyConfigured("The SECRET_KEY and SECRET_KEYS settings cannot both be empty.")
# Nor can we overide unless empty, because override_settings
if secret_key and not secret_keys:
secret_keys = [secret_key]
elif secret_keys and not secret_key:
secret_key = secret_keys[0]

self.__dict__['SECRET_KEY'] = secret_key
self.__dict__['SECRET_KEYS'] = secret_keys
return self.__dict__[name]

val = getattr(self._wrapped, name)

# Special case some settings which require further modification.
# This is done here for performance reasons so the modified value is cached.
if name in {'MEDIA_URL', 'STATIC_URL'} and val is not None:
val = self._add_script_prefix(val)
elif name == 'SECRET_KEY' and not val:
raise ImproperlyConfigured("The SECRET_KEY setting must not be empty.")

self.__dict__[name] = val
return val
Expand Down Expand Up @@ -192,6 +210,11 @@ def __init__(self, settings_module):
setattr(self, setting, setting_value)
self._explicit_settings.add(setting)

# Validity is checked on first access, but providing both values is a
# clear error.
# if self.SECRET_KEY and self.SECRET_KEYS is not None:
# raise ImproperlyConfigured('Only one of SECRET_KEY and SECRET_KEYS can be specified, not both.')

if self.is_overridden('PASSWORD_RESET_TIMEOUT_DAYS'):
if self.is_overridden('PASSWORD_RESET_TIMEOUT'):
raise ImproperlyConfigured(
Expand Down
6 changes: 6 additions & 0 deletions django/conf/global_settings.py
Expand Up @@ -266,6 +266,12 @@ def gettext_noop(s):
# loudly.
SECRET_KEY = ''

# A list of secret keys that can be used instead of SECRET_KEY. The first key
# in the list will be used for signatures and available as settings.SECRET_KEY.
# The other keys will be used to verify the validity of signatures to allow key
# rotation.
SECRET_KEYS = None

# Default file storage mechanism that holds media.
DEFAULT_FILE_STORAGE = 'django.core.files.storage.FileSystemStorage'

Expand Down
6 changes: 4 additions & 2 deletions django/conf/project_template/project_name/settings.py-tpl
Expand Up @@ -19,8 +19,10 @@ BASE_DIR = Path(__file__).resolve().parent.parent
# Quick-start development settings - unsuitable for production
# See https://docs.djangoproject.com/en/{{ docs_version }}/howto/deployment/checklist/

# SECURITY WARNING: keep the secret key used in production secret!
SECRET_KEY = '{{ secret_key }}'
# SECURITY WARNING: keep the secret keys used in production secret!
SECRET_KEYS = [
'{{ secret_key }}',
]

# SECURITY WARNING: don't run with debug turned on in production!
DEBUG = True
Expand Down
41 changes: 31 additions & 10 deletions django/contrib/auth/tokens.py
@@ -1,7 +1,10 @@
from datetime import datetime, time

from django.conf import settings
from django.utils.crypto import constant_time_compare, salted_hmac
from django.core.exceptions import ImproperlyConfigured
from django.utils.crypto import (
constant_time_any, constant_time_compare, salted_hmac,
)
from django.utils.http import base36_to_int, int_to_base36


Expand All @@ -13,19 +16,30 @@ class PasswordResetTokenGenerator:
key_salt = "django.contrib.auth.tokens.PasswordResetTokenGenerator"
algorithm = None
secret = None
secrets = None

def __init__(self):
self.secret = self.secret or settings.SECRET_KEY
# RemovedInDjango40Warning: when the deprecation ends, replace with:
# self.algorithm = self.algorithm or 'sha256'
self.algorithm = self.algorithm or settings.DEFAULT_HASHING_ALGORITHM

def _get_secrets(self):
if self.secret is not None:
if self.secrets is not None:
raise ImproperlyConfigured('Both secret and secrets can not be specified on {}'.format(self.__class__))

return [self.secret]
elif self.secrets is not None:
return self.secrets

return settings.SECRET_KEYS

def make_token(self, user):
"""
Return a token that can be used once to do a password reset
for the given user.
"""
return self._make_token_with_timestamp(user, self._num_seconds(self._now()))
return self._make_token_with_timestamp(user, self._num_seconds(self._now()), self._get_secrets()[0])

def check_token(self, user, token):
"""
Expand All @@ -47,14 +61,21 @@ def check_token(self, user, token):
return False

# Check that the timestamp/uid has not been tampered with
if not constant_time_compare(self._make_token_with_timestamp(user, ts), token):
attempts = [
constant_time_compare(self._make_token_with_timestamp(user, ts, secret), token)
for secret in self._get_secrets()
]
if not constant_time_any(attempts):
# RemovedInDjango40Warning: when the deprecation ends, replace
# with:
# return False
if not constant_time_compare(
self._make_token_with_timestamp(user, ts, legacy=True),
token,
):
legacy_attempts = [
constant_time_compare(
self._make_token_with_timestamp(user, ts, secret, legacy=True),
token
) for secret in self._get_secrets()
]
if not constant_time_any(legacy_attempts):
return False

# RemovedInDjango40Warning: convert days to seconds and round to
Expand All @@ -69,14 +90,14 @@ def check_token(self, user, token):

return True

def _make_token_with_timestamp(self, user, timestamp, legacy=False):
def _make_token_with_timestamp(self, user, timestamp, secret, legacy=False):
# timestamp is number of seconds since 2001-1-1. Converted to base 36,
# this gives us a 6 digit string until about 2069.
ts_b36 = int_to_base36(timestamp)
hash_string = salted_hmac(
self.key_salt,
self._make_hash_value(user, timestamp),
secret=self.secret,
secret=secret,
# RemovedInDjango40Warning: when the deprecation ends, remove the
# legacy argument and replace with:
# algorithm=self.algorithm,
Expand Down
20 changes: 10 additions & 10 deletions django/core/checks/security/base.py
@@ -1,5 +1,4 @@
from django.conf import settings
from django.core.exceptions import ImproperlyConfigured

from .. import Error, Tags, Warning, register

Expand Down Expand Up @@ -186,17 +185,18 @@ def check_ssl_redirect(app_configs, **kwargs):
return [] if passed_check else [W008]


# Not clear we SHOULDN'T still check SECRET_KEY.
@register(Tags.security, deploy=True)
def check_secret_key(app_configs, **kwargs):
try:
secret_key = settings.SECRET_KEY
except (ImproperlyConfigured, AttributeError):
passed_check = False
else:
passed_check = (
len(set(secret_key)) >= SECRET_KEY_MIN_UNIQUE_CHARACTERS and
len(secret_key) >= SECRET_KEY_MIN_LENGTH
def check_secret_keys(app_configs, **kwargs):
def check_key(key):
return (
key and
len(set(key)) >= SECRET_KEY_MIN_UNIQUE_CHARACTERS and
len(key) >= SECRET_KEY_MIN_LENGTH
)

passed_check = all(check_key(key) for key in settings.SECRET_KEYS)

return [] if passed_check else [W009]


Expand Down
56 changes: 38 additions & 18 deletions django/core/signing.py
Expand Up @@ -41,7 +41,9 @@

from django.conf import settings
from django.utils import baseconv
from django.utils.crypto import constant_time_compare, salted_hmac
from django.utils.crypto import (
constant_time_any, constant_time_compare, salted_hmac,
)
from django.utils.encoding import force_bytes
from django.utils.module_loading import import_string
from django.utils.regex_helper import _lazy_re_compile
Expand Down Expand Up @@ -72,10 +74,16 @@ def base64_hmac(salt, value, key, algorithm='sha1'):
return b64_encode(salted_hmac(salt, value, key, algorithm=algorithm).digest()).decode()


def _cookie_signer_key(key):
return b'django.http.cookies' + force_bytes(key) # SECRET_KEY may be str or bytes.


def get_cookie_signer(salt='django.core.signing.get_cookie_signer'):
Signer = import_string(settings.SIGNING_BACKEND)
key = force_bytes(settings.SECRET_KEY) # SECRET_KEY may be str or bytes.
return Signer(b'django.http.cookies' + key, salt=salt)
return Signer(
keys=[_cookie_signer_key(k) for k in settings.SECRET_KEYS],
salt=salt,
)


class JSONSerializer:
Expand All @@ -92,9 +100,9 @@ def loads(self, data):

def dumps(obj, key=None, salt='django.core.signing', serializer=JSONSerializer, compress=False):
"""
Return URL-safe, hmac signed base64 compressed JSON string. If key is
None, use settings.SECRET_KEY instead. The hmac algorithm is the default
Signer algorithm.
Return URL-safe, hmac signed base64 compressed JSON string. If key is None,
use the first key in settings.SECRET_KEYS instead. The hmac algorithm is
the default Signer algorithm.

If compress is True (not the default), check if compressing using zlib can
save some space. Prepend a '.' to signify compression. This is included
Expand Down Expand Up @@ -124,15 +132,15 @@ def dumps(obj, key=None, salt='django.core.signing', serializer=JSONSerializer,
return TimestampSigner(key, salt=salt).sign(base64d)


def loads(s, key=None, salt='django.core.signing', serializer=JSONSerializer, max_age=None):
def loads(s, key=None, salt='django.core.signing', serializer=JSONSerializer, max_age=None, keys=None):
"""
Reverse of dumps(), raise BadSignature if signature fails.

The serializer is expected to accept a bytestring.
"""
# TimestampSigner.unsign() returns str but base64 and zlib compression
# operate on bytes.
base64d = TimestampSigner(key, salt=salt).unsign(s, max_age=max_age).encode()
base64d = TimestampSigner(key=key, salt=salt, keys=keys).unsign(s, max_age=max_age).encode()
decompress = base64d[:1] == b'.'
if decompress:
# It's compressed; uncompress it first
Expand All @@ -147,8 +155,16 @@ class Signer:
# RemovedInDjango40Warning.
legacy_algorithm = 'sha1'

def __init__(self, key=None, sep=':', salt=None, algorithm=None):
self.key = key or settings.SECRET_KEY
def __init__(self, key=None, sep=':', salt=None, algorithm=None, keys=None):
if key is not None:
if keys is not None:
raise TypeError('key and keys can not be specified together.')

self.keys = [key]
elif keys is not None:
self.keys = keys
else:
self.keys = settings.SECRET_KEYS
self.sep = sep
if _SEP_UNSAFE.match(self.sep):
raise ValueError(
Expand All @@ -161,11 +177,14 @@ def __init__(self, key=None, sep=':', salt=None, algorithm=None):
self.algorithm = algorithm or settings.DEFAULT_HASHING_ALGORITHM

def signature(self, value):
return base64_hmac(self.salt + 'signer', value, self.key, algorithm=self.algorithm)
return self._signature(value, self.keys[0])

def _legacy_signature(self, value):
def _signature(self, value, key):
return base64_hmac(self.salt + 'signer', value, key, algorithm=self.algorithm)

def _legacy_signature(self, value, key):
# RemovedInDjango40Warning.
return base64_hmac(self.salt + 'signer', value, self.key, algorithm=self.legacy_algorithm)
return base64_hmac(self.salt + 'signer', value, key, algorithm=self.legacy_algorithm)

def sign(self, value):
return '%s%s%s' % (value, self.sep, self.signature(value))
Expand All @@ -174,12 +193,13 @@ def unsign(self, signed_value):
if self.sep not in signed_value:
raise BadSignature('No "%s" found in value' % self.sep)
value, sig = signed_value.rsplit(self.sep, 1)
if (
constant_time_compare(sig, self.signature(value)) or (
attempts = [
constant_time_compare(sig, self._signature(value, key)) or (
self.legacy_algorithm and
constant_time_compare(sig, self._legacy_signature(value))
)
):
constant_time_compare(sig, self._legacy_signature(value, key))
) for key in self.keys
]
if constant_time_any(attempts):
return value
raise BadSignature('Signature "%s" does not match' % sig)

Expand Down
10 changes: 10 additions & 0 deletions django/test/utils.py
Expand Up @@ -404,6 +404,9 @@ def __init__(self, **kwargs):
super().__init__()

def enable(self):
if 'SECRET_KEY' in self.options and 'SECRET_KEYS' in self.options:
raise AssertionError('Only SECRET_KEY or SECRET_KEYS can be specified, not both.')

# Keep this code at the beginning to leave the settings unchanged
# in case it raises an exception because INSTALLED_APPS is invalid.
if 'INSTALLED_APPS' in self.options:
Expand All @@ -415,6 +418,13 @@ def enable(self):
override = UserSettingsHolder(settings._wrapped)
for key, new_value in self.options.items():
setattr(override, key, new_value)

if 'SECRET_KEY' in self.options:
override.SECRET_KEYS = [self.options['SECRET_KEY']]

if 'SECRET_KEYS' in self.options:
override.SECRET_KEY = self.options['SECRET_KEYS'][0]

Comment on lines +421 to +427
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @apollo13 Can I get your thoughts if you have capacity please? I'm struggling to get this to work...

I'd like to move all of this if only one is set logic into LazySettings.__getattr__() — I've got that more or less working but override_settings is not playing ball.

I'm getting the underlying settings exposed:

(Pdb) settings.SECRET_KEY
'django_tests_secret_key'

FAIL: test_no_secret_key (settings_tests.tests.SettingsTests)

I'll push where I'm at so you can see.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this help you:

diff --git a/django/test/utils.py b/django/test/utils.py
index cca757bda1..79874133d8 100644
--- a/django/test/utils.py
+++ b/django/test/utils.py
@@ -420,7 +420,7 @@ class override_settings(TestContextDecorator):
             setattr(override, key, new_value)
 
         if 'SECRET_KEY' in self.options:
-            override.SECRET_KEYS = [self.options['SECRET_KEY']]
+            override.SECRET_KEYS = [self.options['SECRET_KEY']] if self.options['SECRET_KEY'] else []
 
         if 'SECRET_KEYS' in self.options:
             override.SECRET_KEY = self.options['SECRET_KEYS'][0]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your current code copies the empty (!) SECRET_KEY into SECRET_KEYS, making it a list ala [""]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw the line below can also fail if SECRET_KEYS is set to an empty list; in this case you should set SECRET_KEY to empty as well I think? We really have to check if we can make this code nicer, it makes my head hurt :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We do. 😄 — Still at the making it work phase. First real look this morning so, plenty of time 🤪

self.wrapped = settings._wrapped
settings._wrapped = override
for key, new_value in self.options.items():
Expand Down
10 changes: 10 additions & 0 deletions django/utils/crypto.py
Expand Up @@ -79,6 +79,16 @@ def constant_time_compare(val1, val2):
return secrets.compare_digest(force_bytes(val1), force_bytes(val2))


def constant_time_any(seq):
"""
Like any() but avoid returning early for successful outcomes.
"""
result = False
for value in seq:
result |= bool(value)
return result


def pbkdf2(password, salt, iterations, dklen=0, digest=None):
"""Return the hash of password using pbkdf2."""
if digest is None:
Expand Down
3 changes: 3 additions & 0 deletions docs/releases/3.2.txt
Expand Up @@ -358,6 +358,9 @@ Requests and Responses
Security
~~~~~~~~

* The new :setting:`SECRET_KEYS` allows... TODO: flesh-out, and adjust release
note below.

* The :setting:`SECRET_KEY` setting is now checked for a valid value upon first
access, rather than when settings are first loaded. This enables running
management commands that do not rely on the ``SECRET_KEY`` without needing to
Expand Down