[Django] #36546: Deprecate django.utils.crypto.constant_time_compare()

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Type:
| Cleanup/optimization
Status: new | Component: Utilities
Version: dev | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Historically it was a bespoke implementation (added in
45c7f427ce830dd1b2f636fb9c244fda9201cadb) before the stdlib implementation
was preferred (58176dee88ac7c1038c7f685af023e634b143d02). Now it's just
alias of `secrets.compare_digest` (which itself is
[https://github.com/python/cpython/blob/dd079db4b96fa474b8e6d71ae9db662c4ce28caf/Lib/secrets.py#L17
an alias] of `hmac.compare_digest`, which was used before
1d0bab0bfd77edcf1228d45bf654457a8ff1890d).

`constant_time_compare()` does call `force_bytes()` on its arguments but
this was a workaround for Python 2.7
(7e3cf3cfd27e53ced0a1fc65a02849f78a292d3d) and no tests in Django's test
suite fail with those calls removed.
--
Ticket URL: <https://code.djangoproject.com/ticket/36546>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by suhail vs):

what about adding a warning in function constant_time_compare in file
`django.utils.crypto.py`

{{{
import warnings
from django.utils.deprecation import RemovedInNextVersionWarning

def constant_time_compare(val1, val2):
"""Return True if the two strings are equal, False otherwise."""
warnings.warn(
"constant_time_compare(val1, val2) is deprecated and will be
removed in Django 6.0. "
"`import secrets` from python standard library, then run
secrets.compare_digest(val1,val2) instead.",
RemovedInNextVersionWarning,
stacklevel=2
)
return secrets.compare_digest(force_bytes(val1), force_bytes(val2))
}}}
`
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:1>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
--------------------------------------+------------------------------------

Reporter: Tim Graham | Owner: (none)

Type: Cleanup/optimization | Status: new

Component: Utilities | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Sarah Boyce):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:2>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:

Type: | JaeHyuckSa
Cleanup/optimization | Status: assigned

Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by JaeHyuckSa):

* owner: (none) => JaeHyuckSa
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:3>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by JaeHyuckSa):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:4>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by JaeHyuckSa):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:5>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by JaeHyuckSa):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:6>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:7>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by JaeHyuckSa):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:8>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:9>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa

Cleanup/optimization | Status: closed
Component: Utilities | Version: dev
Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"0246f478882c26bc1fe293224653074cd46a90d0" 0246f47]:
{{{#!CommitTicketReference repository=""
revision="0246f478882c26bc1fe293224653074cd46a90d0"
Fixed #36546 -- Deprecated django.utils.crypto.constant_time_compare() in
favor of hmac.compare_digest().

Signed-off-by: SaJH <wogur...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:10>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa
Cleanup/optimization | Status: closed
Component: Utilities | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"d0e4dd5cdd743a5c43c4ccc2c8fa29d3982eaa71" d0e4dd5c]:
{{{#!CommitTicketReference repository=""
revision="d0e4dd5cdd743a5c43c4ccc2c8fa29d3982eaa71"
Fixed #36572 -- Revert "Fixed #36546 -- Deprecated

django.utils.crypto.constant_time_compare() in favor of
hmac.compare_digest()."

This reverts commit 0246f478882c26bc1fe293224653074cd46a90d0.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:11>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa

Cleanup/optimization | Status: new

Component: Utilities | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* has_patch: 1 => 0
* resolution: fixed =>
* stage: Ready for checkin => Someday/Maybe
* status: closed => new

Comment:

Reopening following revert. See discussion in #36572

> A later first step could be to replace internal usage of
constant_time_compare() with compare_digest()

The deprecation itself needs more consideration
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:12>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner:
Type: | JaeHyuckSa
Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by JaeHyuckSa):

There are quite a few points that need further discussion, so it’s
difficult to move this forward right now. I’ll unassign myself for the
time being
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:13>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: | Status: assigned
Cleanup/optimization |

Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by JaeHyuckSa):

* owner: JaeHyuckSa => (none)

* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:14>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by JaeHyuckSa):

* cc: JaeHyuckSa (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:15>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Pravin):

What is current state, Can i work on this ?
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:16>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Pravin, refer to the discussion on the revert commit in comment:12. Looks
like we first want to stop using it internally.
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:17>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Pravin):

* cc: Pravin (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:18>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Pravin):

@Simon Thanks for the more context.

I'm going to start working on this , First I will remove internal
dependency on constant_time_compare.
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:19>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Pravin

Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Pravin):

* owner: (none) => Pravin

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:20>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Pravin
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Pravin):

just confirming the technical approach for the internal dependency
removal:

1. I will be using **`secrets.compare_digest()`** for the replacement.
2. The implementation at the call site will include **`force_bytes()`**
on both arguments to ensure type safety:
`secrets.compare_digest(force_bytes(val1), force_bytes(val2))`.
3. The definition of `constant_time_compare()` itself will **not be
modified** to avoid breaking third-party applications that rely on its
current type-coercion logic.

**Example of the change:**

Old internal Django code:`constant_time_compare(val1, val2)`
New internal Django code: `secrets.compare_digest(force_bytes(val1),
force_bytes(val2))`

I am proceeding with the internal refactoring now.
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:21>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Pravin
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

No, the `force_bytes()` is not needed in Django's use of
`constant_time_compare()`, see ticket:36572#comment:4. The problem is only
with third-party users of the function.
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:22>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Pravin
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Pravin):

Hi Tim,
I was in that dilema and I was not so confident in removing
{{{force_bytes}}}. But Since you confirmed it I can safely proceed ( i
will no use force_bytes ). Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:23>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Pravin
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Pravin):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:24>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Pravin
Type: | Status: assigned
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Pravin):

can i make triage stage to "Accepted" ?
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:25>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
--------------------------------------+------------------------------------

Reporter: Tim Graham | Owner: Pravin

Type: Cleanup/optimization | Status: assigned

Component: Utilities | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Pravin):

* stage: Someday/Maybe => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:26>

[Django]

#36546: Deprecate django.utils.crypto.constant_time_compare()
-------------------------------------+-------------------------------------

Reporter: Tim Graham | Owner: Pravin

Type: | Status: assigned
Cleanup/optimization |

Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:

| Someday/Maybe

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by JaeHyuckSa):

* stage: Accepted => Someday/Maybe

Comment:

Please make it so that only the ticket classifier can change the ticket
statu
--
Ticket URL: <https://code.djangoproject.com/ticket/36546#comment:27>