fix: replaced md4 with blake2b #34442

Anas12091101 · 2024-03-28T18:02:38Z

Description

This PR replaces md4 with blake2b in memcache. At MIT, we recently encountered an issue after upgrading our base image from a debian-buster image to a debian-bookworm image. This uncovered an incompatibility between the newer version of openssl and one of the hash digests that edx-platform is using (1.1.1d-0+deb10u7). Basically, openssl + hashlib are saying that md4 is supported and available but if you actually try to invoke openssl md4 you’ll get an error saying it is not available. This PR resolves this issue by updating the hash function to something a bit newer and more widely supported.

Useful information to include:

Which edX user roles will this change impact? Learner, Course Author

Supporting information

https://discuss.openedx.org/t/openssl-upgrade-md4-hashing/12654

Testing instructions

In the lms container run python manage.py lms shell
In the python shell run:

>>> import hashlib
>>> blake2b = hashlib.new("blake2b", digest_size=16)
>>> string = "abc"
>>> blake2b.update(string.encode('utf-8'))
>>> blake2b.hexdigest()

You should get the hash without any errors in the above statements.

Deadline

"None"

Other information

Include anything else that will help reviewers and consumers understand the change.

Relevant openssl issue: OpenSSL 3.0 and 3.1: md4 support is broken openssl/openssl#21247

openedx-webhooks · 2024-03-28T18:02:43Z

Thanks for the pull request, @Anas12091101! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

pdpinch · 2024-03-28T18:12:58Z

@Anas12091101 I know this caused problems with launching the gradebook MFE. Do you know what else in edx-platform might use this caching scheme?

timmc-edx · 2024-03-28T20:06:53Z

My only concern is that this will have the effective of clearing the entire cache, potentially causing a disruption. (On edx.org we only clear memcache as a last resort, as it logs out all sessions and causes a performance hit.)

So, a few thoughts:

Is there some option for a rotation mechanism? Fetching two keys (one blake2b, one md4) would be a much higher traffic burden, but maybe there's something clever we can do.
Would it be possible to just use a different implementation of MD4? MD4 is a weak hash function for cryptographic purposes, but should be fine for sharding.

Anas12091101 · 2024-03-29T07:26:39Z

@Anas12091101 I know this caused problems with launching the gradebook MFE. Do you know what else in edx-platform might use this caching scheme?

@pdpinch I think by default in LMS and CMS memcache is being used as the caching scheme in edx-platform. So anywhere in edx-platform where we are using cache.get, we are calling the safe_key fn in the memcache.py

blarghmatey · 2024-03-29T16:11:36Z

One option would be to put this in a try/except block catching the ValueError that returns when md4 is not a supported algorithm and falling back to Blake2B. That doesn't solve for the case when deploying a new set of instances with the newer OpenSSL that no longer supports that method.

On the note of using a different implementation of md4, that will give a stable target (for now) but introduces the potential for stagnation, as that algorithm is (rightfully) not supported any more. That leaves us in the situation of either accepting a one-time hit of dumping cached items or developing a migration path.

All of that being said, the hashed key is only used if the combined values of key, key_prefix, and version exceeds 250 characters. Given that the cache is designed to be a performance enhancement, not a functional requirement and that only a subset of cached values will be impacted I think that this should be a safe change to merge without developing a complex stack of migration logic.

timmc-edx · 2024-04-01T00:31:51Z

Good to know this only affects larger keys. I might put in a quick PR to add telemetry for checking what key sizes we're seeing on edX.

I think it might make sense to put this behind a feature switch and make a DEPR out of removing the md4 option entirely. Switching to blake2b as a default seems like a very reasonable thing to do, but having a well-defined timeframe would be helpful here.

From personal experience, while caches are nominally a performance enhancement, a full cache can very quickly become a critical requirement for system stability. I'd like to understand which situation we're in here before saying that I'm comfortable with a hard cutover.

pdpinch · 2024-04-02T12:52:52Z

cms/envs/common.py

+    # .. 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


I think we should mention somewhere that this should be a short-lived toggle. Maybe only until the sycamore release.

And do we need to define this twice?

Are you talking about annonations? Yes, we don't need to add the annonations again in the CMS. The flag is however required in both LMS and CMS

pdpinch · 2024-04-03T00:29:17Z

@timmc-edx what do you think?

timmc-edx · 2024-04-03T13:38:18Z

Looks good! The feature flag would allow us to schedule a time to do a cutover in a controlled way (and would give us time to research which keys would be affected).

asadali145

LGTM! Just a few code improvements.

Anas12091101 · 2024-04-09T10:51:55Z

cms/envs/common.py

@@ -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.


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

asadali145 · 2024-04-09T09:01:46Z

cms/envs/common.py

@@ -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.
+    'ENABLE_BLAKE2B_HASH_FN': False,


Can we avoid the abbreviation FN? ENABLE_BLAKE2B_HASH_FUNCTION should be fine.

asadali145 · 2024-04-09T09:23:56Z

common/djangoapps/util/memcache.py

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


We need to sort the imports.

common/djangoapps/util/memcache.py

+    if settings.FEATURES.get("ENABLE_BLAKE2B_HASH_FN", 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()


asadali145 · 2024-04-09T10:10:50Z

lms/envs/common.py

+    # .. toggle_name: FEATURES['ENABLE_BLAKE2B_HASH_FN']
+    # .. 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_tickets: https://github.com/openedx/edx-platform/pull/34442


Can we add toggle_warning for this flag like other LMS & CMS common flags?
# .. toggle_warning: For consistency, keep the value in sync with the setting of the same name in the LMS and CMS.

asadali145 · 2024-04-09T10:12:42Z

common/djangoapps/util/tests/test_memcache.py

+from django.test import TestCase, override_settings
+from django.conf import settings


Sorting? You can use isort.

pdpinch · 2024-04-18T15:40:22Z

@Anas12091101 would you mind squashing your commits?

pdpinch · 2024-04-18T15:44:48Z

@timmc-edx we took the route of adding a feature flag to ease the changeover. Is this OK to merge from your perspective?

After this merges, we'll open a DEPR for removing the md4 code and the feature flag, but it wouldn't be removed until after the Redwood or Sumac releases, depending on timing.

timmc-edx · 2024-04-22T02:21:12Z

Yes, I like this approach, and have no reservations about it merging.

openedx-webhooks · 2024-04-22T18:09:43Z

@Anas12091101 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2024-04-22T23:13:02Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-04-23T00:13:58Z

2U Release Notice: This PR has been deployed to the edX production environment.

arslanashraf7 · 2024-04-23T08:37:09Z

cms/envs/common.py

@@ -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.
+    'ENABLE_BLAKE2B_HASHiNG': False,


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

Suggested change

'ENABLE_BLAKE2B_HASHiNG': False,

'ENABLE_BLAKE2B_HASHING': False,

Created the PR to fix this

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 28, 2024

pdpinch reviewed Apr 2, 2024

View reviewed changes

Anas12091101 force-pushed the anas/replace-md4-with-blake2b branch 2 times, most recently from 63c10e3 to 527f1d8 Compare April 8, 2024 13:04

asadali145 approved these changes Apr 9, 2024

View reviewed changes

Anas12091101 force-pushed the anas/replace-md4-with-blake2b branch from bfeabba to 4658109 Compare April 15, 2024 12:37

feat: added feature flag to enable blake2b hashing

76385ed

Anas12091101 force-pushed the anas/replace-md4-with-blake2b branch from 4658109 to 76385ed Compare April 19, 2024 07:23

pdpinch merged commit aea7fce into openedx:master Apr 22, 2024
66 checks passed

arslanashraf7 reviewed Apr 23, 2024

View reviewed changes

Anas12091101 mentioned this pull request Apr 23, 2024

fix: blake2b feature flag name #34574

Merged

pdpinch mentioned this pull request Dec 12, 2024

[DEPR]: Remove md4 caching and related ENABLE_BLAKE2B_HASHING feature flag #36024

Open

robrap mentioned this pull request Dec 12, 2024

[DEPR-2U]: Remove md4 caching and related ENABLE_BLAKE2B_HASHING feature flag edx/edx-arch-experiments#872

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: replaced md4 with blake2b #34442

fix: replaced md4 with blake2b #34442

Anas12091101 commented Mar 28, 2024 •

edited

Loading

openedx-webhooks commented Mar 28, 2024

pdpinch commented Mar 28, 2024

timmc-edx commented Mar 28, 2024

Anas12091101 commented Mar 29, 2024 •

edited

Loading

blarghmatey commented Mar 29, 2024

timmc-edx commented Apr 1, 2024

pdpinch Apr 2, 2024

pdpinch Apr 2, 2024

Anas12091101 Apr 2, 2024 •

edited

Loading

pdpinch commented Apr 3, 2024

timmc-edx commented Apr 3, 2024

asadali145 left a comment

This comment was marked as resolved.

Anas12091101 Apr 9, 2024

asadali145 Apr 9, 2024

asadali145 Apr 9, 2024

This comment was marked as resolved.

asadali145 Apr 9, 2024

asadali145 Apr 9, 2024

pdpinch commented Apr 18, 2024

pdpinch commented Apr 18, 2024

timmc-edx commented Apr 22, 2024

openedx-webhooks commented Apr 22, 2024

edx-pipeline-bot commented Apr 22, 2024

edx-pipeline-bot commented Apr 23, 2024

arslanashraf7 Apr 23, 2024

Anas12091101 Apr 23, 2024

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

		from django.test import TestCase, override_settings
		from django.conf import settings

	'ENABLE_BLAKE2B_HASHiNG': False,
	'ENABLE_BLAKE2B_HASHING': False,

fix: replaced md4 with blake2b #34442

fix: replaced md4 with blake2b #34442

Conversation

Anas12091101 commented Mar 28, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented Mar 28, 2024

pdpinch commented Mar 28, 2024

timmc-edx commented Mar 28, 2024

Anas12091101 commented Mar 29, 2024 • edited Loading

blarghmatey commented Mar 29, 2024

timmc-edx commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anas12091101 Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

pdpinch commented Apr 3, 2024

timmc-edx commented Apr 3, 2024

asadali145 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdpinch commented Apr 18, 2024

pdpinch commented Apr 18, 2024

timmc-edx commented Apr 22, 2024

openedx-webhooks commented Apr 22, 2024

edx-pipeline-bot commented Apr 22, 2024

edx-pipeline-bot commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anas12091101 commented Mar 28, 2024 •

edited

Loading

Anas12091101 commented Mar 29, 2024 •

edited

Loading

Anas12091101 Apr 2, 2024 •

edited

Loading