Opened 3 months ago

Closed 3 months ago

Last modified 3 weeks ago

#33691 closed Cleanup/optimization (fixed)

Deprecate CryptPasswordHasher.

Reported by: Mariusz Felisiak Owned by: Mariusz Felisiak
Component: contrib.auth Version: 4.0
Severity: Normal Keywords:
Cc: Florian Apolloner, Carlton Gibson Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

CryptPasswordHasher was added 15 years ago mainly to support legacy UNIX apps. It's almost undocumented, not recommended, and supported only on UNIX. Moreover crypt module was deprecated in Python 3.11 (see https://github.com/python/cpython/commit/f45aa8f304a12990c2ca687f2088f04b07906033).

We should deprecate it in Django 4.1 and remove in Django 5.0.

Change History (10)

comment:1 Changed 3 months ago by Carlton Gibson

Triage Stage: UnreviewedAccepted

comment:2 Changed 3 months ago by Florian Apolloner

ACK, while we are on it I wonder if we should deprecate the unsalted & sha/md5 hashers as well. It is time to face reality, if you haven't upgraded Django by now and are still on one of those old algorithms your installation is probably 10 years or older?

comment:3 in reply to:  2 ; Changed 3 months ago by Mariusz Felisiak

Replying to Florian Apolloner:

ACK, while we are on it I wonder if we should deprecate the unsalted & sha/md5 hashers as well. It is time to face reality, if you haven't upgraded Django by now and are still on one of those old algorithms your installation is probably 10 years or older?

MD5PasswordHasher is still a nice hook for faster testing :)

comment:4 Changed 3 months ago by Mariusz Felisiak

Has patch: set

comment:5 Changed 3 months ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 02dbf166:

Fixed #33691 -- Deprecated django.contrib.auth.hashers.CryptPasswordHasher.

comment:6 in reply to:  3 Changed 3 months ago by Nick Pope

Replying to Mariusz Felisiak:

Replying to Florian Apolloner:

ACK, while we are on it I wonder if we should deprecate the unsalted & sha/md5 hashers as well. It is time to face reality, if you haven't upgraded Django by now and are still on one of those old algorithms your installation is probably 10 years or older?

MD5PasswordHasher is still a nice hook for faster testing :)

Like Florian, I also think that now is the time to get rid of the following hashers:

  • MD5PasswordHasher
  • SHA1PasswordHasher
  • UnsaltedMD5PasswordHasher
  • UnsaltedSHA1PasswordHasher

It is long past time that these were gone, and they were removed from the default PASSWORD_HASHERS setting in Django 1.10 along with CryptPasswordHasher. In addition, they will not be fully removed until December 2023, so that gives Django 4.1 and 4.2 to move of these, and 4.2, being an LTS, gives plenty of time beyond that.

Yes, it'd be nice to keep something fast around for testing purposes and we've been recommending MD5PasswordHasher up to now, but I don't think that should prevent removal of the other three at the very least...

However, I propose that we add an explicit new TestPasswordHasher that has something in place to scream about not using it outside of local development environments and continuous integration pipelines for the purpose of speeding up testing. Maybe this could be achieved with the check framework (disabled when using testing tools). Or disabled when Django is executed via normal ASGI/WSGI stuff as would be done in production, but not when running tests. Or that TestPasswordHasher will only work when it is the only hasher.

Another question would be whether we make it still use MD5 as the algorithm, or whether, now that we're making it explicitly for testing, we make it even faster by making it totally insecure? (Although this would maybe be a step to far in case of people still ignoring all the warnings...)

I started messing around very roughly with this idea on the following branch, but I don't really have all that much time to follow it up right now. It needs thought about how we'd flag up and/or block use of TestPasswordHasher in the wrong places.

https://github.com/ngnpope/django/tree/test-password-hasher

comment:7 Changed 3 months ago by Mariusz Felisiak

Agreed, we can at least deprecate SHA1PasswordHasher, UnsaltedMD5PasswordHasher, UnsaltedSHA1PasswordHasher before the feature freeze.
Please Refs #33691 -- ... in your patch.

comment:8 Changed 3 weeks ago by Claude Paroz

PR for SHA1PasswordHasher, UnsaltedMD5PasswordHasher, UnsaltedSHA1PasswordHasher deprecation.

comment:9 Changed 3 weeks ago by Claude Paroz

What about something like this (TestPasswordHasher being imported from django.test.utils is also a rather strong indication not to use it in production):

diff --git a/django/contrib/auth/hashers.py b/django/contrib/auth/hashers.py
index 432c624483..f409882d70 100644
--- a/django/contrib/auth/hashers.py
+++ b/django/contrib/auth/hashers.py
@@ -675,7 +675,7 @@ class SHA1PasswordHasher(BasePasswordHasher):
         pass
 
 
-class MD5PasswordHasher(BasePasswordHasher):
+class _MD5UnsecurePasswordHasher(BasePasswordHasher):
     """
     The Salted MD5 password hashing algorithm (not recommended)
     """
@@ -717,6 +717,17 @@ class MD5PasswordHasher(BasePasswordHasher):
         pass
 
 
+# RemovedInDjango51Warning.
+class MD5PasswordHasher(_MD5UnsecurePasswordHasher):
+    def __init__(self, *args, **kwargs):
+        warnings.warn(
+            "django.contrib.auth.hashers.MD5PasswordHasher is deprecated.",
+            RemovedInDjango51Warning,
+            stacklevel=2,
+        )
+        super().__init__(*args, **kwargs)
+
+
 # RemovedInDjango51Warning.
 class UnsaltedSHA1PasswordHasher(BasePasswordHasher):
     """
diff --git a/django/test/utils.py b/django/test/utils.py
index 270e34b69d..1e87bcaa9f 100644
--- a/django/test/utils.py
+++ b/django/test/utils.py
@@ -17,6 +17,7 @@ from xml.dom.minidom import Node, parseString
 from django.apps import apps
 from django.apps.registry import Apps
 from django.conf import UserSettingsHolder, settings
+from django.contrib.auth.hashers import _MD5UnsecurePasswordHasher
 from django.core import mail
 from django.core.exceptions import ImproperlyConfigured
 from django.core.signals import request_started, setting_changed
@@ -999,3 +1000,9 @@ def register_lookup(field, *lookups, lookup_name=None):
     finally:
         for lookup in lookups:
             field._unregister_lookup(lookup, lookup_name)
+
+
+class TestPasswordHasher(_MD5UnsecurePasswordHasher):
+    """
+    An unsecure but fast Salted MD5 password hashing algorithm for speedier tests.
+    """

comment:10 Changed 3 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In 3b79dab1:

Refs #33691 -- Deprecated insecure password hashers.

SHA1PasswordHasher, UnsaltedSHA1PasswordHasher, and UnsaltedMD5PasswordHasher
are now deprecated.

Note: See TracTickets for help on using tickets.
Back to Top