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

fix: replaced md4 with blake2b #34442

Merged
merged 1 commit into from
Apr 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,9 @@
# .. toggle_creation_date: 2024-03-22
# .. toggle_tickets: https://github.com/openedx/edx-platform/pull/33911
'ENABLE_GRADING_METHOD_IN_PROBLEMS': False,

# See annotations in lms/envs/common.py for details.

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we don't need to redefine the annotations again. There are some examples as well in which the flag's annonation has not been redefined in cms settings. Examples: https://github.com/openedx/edx-platform/blob/34b1b90c499ad2b78233a0dcc70fbb789a78afc9/cms/envs/common.py#L187-L188

https://github.com/openedx/edx-platform/blob/34b1b90c499ad2b78233a0dcc70fbb789a78afc9/cms/envs/common.py#L189-L190

'ENABLE_BLAKE2B_HASHiNG': False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: Dictionary matching is going to be case-sensitive. This might cause errors.

Suggested change
'ENABLE_BLAKE2B_HASHiNG': False,
'ENABLE_BLAKE2B_HASHING': False,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created the PR to fix this

}

# .. toggle_name: ENABLE_COPPA_COMPLIANCE
Expand Down
10 changes: 7 additions & 3 deletions common/djangoapps/util/memcache.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,20 @@
import hashlib
from urllib.parse import quote_plus

from django.conf import settings
from django.utils.encoding import smart_str


def fasthash(string):
"""
Hashes `string` into a string representation of a 128-bit digest.
"""
md4 = hashlib.new("md4")
md4.update(string.encode('utf-8'))
return md4.hexdigest()
if settings.FEATURES.get("ENABLE_BLAKE2B_HASHING", False):
hash_obj = hashlib.new("blake2b", digest_size=16)
else:
hash_obj = hashlib.new("md4")
hash_obj.update(string.encode('utf-8'))
return hash_obj.hexdigest()


def cleaned_string(val):
Expand Down
35 changes: 34 additions & 1 deletion common/djangoapps/util/tests/test_memcache.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@
"""


from django.conf import settings
from django.core.cache import caches
from django.test import TestCase
from django.test import TestCase, override_settings

from common.djangoapps.util.memcache import safe_key

BLAKE2B_ENABLED_FEATURES = settings.FEATURES.copy()
BLAKE2B_ENABLED_FEATURES["ENABLE_BLAKE2B_HASHING"] = True


class MemcacheTest(TestCase):
"""
Expand Down Expand Up @@ -51,6 +55,20 @@ def test_safe_key_long(self):
# The key should now be valid
assert self._is_valid_key(key), f'Failed for key length {length}'

@override_settings(FEATURES=BLAKE2B_ENABLED_FEATURES)
def test_safe_key_long_with_blake2b_enabled(self):
# Choose lengths close to memcached's cutoff (250)
for length in [248, 249, 250, 251, 252]:

# Generate a key of that length
key = 'a' * length

# Make the key safe
key = safe_key(key, '', '')

# The key should now be valid
assert self._is_valid_key(key), f'Failed for key length {length}'

def test_long_key_prefix_version(self):

# Long key
Expand All @@ -65,6 +83,21 @@ def test_long_key_prefix_version(self):
key = safe_key('key', 'prefix', 'a' * 300)
assert self._is_valid_key(key)

@override_settings(FEATURES=BLAKE2B_ENABLED_FEATURES)
def test_long_key_prefix_version_with_blake2b_enabled(self):

# Long key
key = safe_key('a' * 300, 'prefix', 'version')
assert self._is_valid_key(key)

# Long prefix
key = safe_key('key', 'a' * 300, 'version')
assert self._is_valid_key(key)

# Long version
key = safe_key('key', 'prefix', 'a' * 300)
assert self._is_valid_key(key)

def test_safe_key_unicode(self):

for unicode_char in self.UNICODE_CHAR_CODES:
Expand Down
12 changes: 12 additions & 0 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -1059,6 +1059,18 @@
# .. toggle_creation_date: 2024-03-22
# .. toggle_tickets: https://github.com/openedx/edx-platform/pull/33911
'ENABLE_GRADING_METHOD_IN_PROBLEMS': False,

# .. toggle_name: FEATURES['ENABLE_BLAKE2B_HASHING']
# .. toggle_implementation: DjangoSetting
# .. toggle_default: False
# .. toggle_description: Enables the memcache to use the blake2b hash algorithm instead of depreciated md4 for keys
# exceeding 250 characters
# .. toggle_use_cases: open_edx
# .. toggle_creation_date: 2024-04-02
# .. toggle_target_removal_date: 2024-12-09
# .. toggle_warning: For consistency, keep the value in sync with the setting of the same name in the LMS and CMS.
# .. toggle_tickets: https://github.com/openedx/edx-platform/pull/34442
'ENABLE_BLAKE2B_HASHING': False,
}

# Specifies extra XBlock fields that should available when requested via the Course Blocks API
Expand Down
Loading