gh-134696: align OpenSSL and HACL*-based hash functions constructors AC signatures #134713

picnixz · 2025-05-26T12:43:13Z

~~This is a breaking change but this breaking change is needed. Because this is a really bad breaking change if we were to use Python 3.13 in production, I will not backport it to 3.13.~~
I made the choice of following what is documented rather than what was historically done: https://docs.python.org/3/library/hashlib.html#hashlib.md5.

Historically, we could write hashlib.md5(string=b'abc') (and this would be the only way to use a keyword argument here for the message to hash), but since the docs mention [data] as a regular argument (not positional-only), we should follow them. Also, I would like to make them positional-only in 3.17 (with a warning in 3.15).

I will however backport this to 3.14 because it's time we make the hashlib API more consistent.

Issue: Clinic signatures of HACL* hash functions are inconsistent with OpenSSL implementation #134696

…ementation-clinic-134696

gpshead · 2025-05-26T15:07:09Z

While I doubt much code uses the keyword argument name on these, the non-breaking way to make this change is to accept both names and raise an error if both are supplied as well as a deprecation warning in 3.15 onwards when the undesired 'string=' one is supplied.

picnixz · 2025-05-26T15:13:50Z

While I doubt much code uses the keyword argument name on these, the non-breaking way to make this change is to accept both names and raise an error if both are supplied as well as a deprecation warning in 3.15 onwards when the undesired 'string=' one is supplied.

I actually had a helper function for hashlib.new but honestly I think it's a bit too much work :( is there an easy way for AC to do it automatically? otherwise I'll need to wrap all openssl_* functions with this additional logic and I don't think it's worth it (but again, we might want to keep full bc here). I'll work on that tomorrow a bit more to see the exact additional complexity.

picnixz · 2025-05-27T11:19:09Z

@gpshead With these modifications, we should have full backwards compatibility

Lib/hashlib.py

Lib/test/test_hashlib.py

Lib/hashlib.py

Lib/test/test_hashlib.py

picnixz · 2025-05-27T16:14:06Z

I'll be away for a few days (like every week) but I want to merge this one myself.

picnixz · 2025-05-30T12:16:57Z

I found a better way to do it offline so let's forget about this complicated stuff

picnixz · 2025-05-30T21:12:40Z

In a follow-up PR, I'll keep [data] and deprecate 'string'. I don't think I'll make it positional-only.

gpshead

Agreed, I think this is a practical solution to this today.

We do have some deprecation warning support in AC already -https://devguide.python.org/development-tools/clinic/#how-to-deprecate-passing-parameters-positionally-or-by-keyword

which you could use for the string= kwarg as is in this PR if I understand the above doc correctly.

#108271 is the AC feature that'd probably have made this a bit easier.

serhiy-storchaka · 2025-05-31T07:32:02Z

Since NULL indicates that the argument was not passed, we can easy do deprecation in the helper function. Argument Clinic support is needed to rename or remove parameters of the type that has no invalid value (e.g. int).

miss-islington-app · 2025-05-31T07:37:59Z

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app · 2025-05-31T07:37:59Z

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington-app · 2025-05-31T07:38:02Z

Sorry, @picnixz, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c6e63d9d351f6d952000ec3bf84b3a7607989f92 3.13

miss-islington-app · 2025-05-31T07:38:05Z

Sorry, @picnixz, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c6e63d9d351f6d952000ec3bf84b3a7607989f92 3.14

…constructors AC signatures (pythonGH-134713) OpenSSL and HACL*-based hash functions constructors now support both `data` and `string` parameters. Previously these constructor functions inconsistently supported sometimes `data` and sometimes `string`, while the documentation expected `data` to be given in all cases. (cherry picked from commit c6e63d9) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

bedevere-app · 2025-05-31T07:45:21Z

GH-134961 is a backport of this pull request to the 3.14 branch.

…constructors AC signatures (pythonGH-134713) OpenSSL and HACL*-based hash functions constructors now support both `data` and `string` parameters. Previously these constructor functions inconsistently supported sometimes `data` and sometimes `string`, while the documentation expected `data` to be given in all cases. (cherry picked from commit c6e63d9) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

bedevere-app · 2025-05-31T07:55:05Z

GH-134962 is a backport of this pull request to the 3.13 branch.

bedevere-bot · 2025-05-31T08:52:58Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 RHEL8 FIPS Only Blake2 Builtin Hash 3.x (no tier) has failed when building commit c6e63d9.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/469/builds/11257) and take a look at the build logs.
Check if the failure is related to this commit (c6e63d9) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/469/builds/11257

Failed tests:

test_hashlib

Failed subtests:

test_clinic_signature - test.test_hashlib.HashLibTestCase.test_clinic_signature

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-fips-x86_64.no-builtin-hashes-except-blake2/build/Lib/test/test_hashlib.py", line 255, in test_clinic_signature
    constructor(b'')
    ~~~~~~~~~~~^^^^^
ValueError: [digital envelope routines: EVP_DigestInit_ex] disabled for FIPS


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-fips-x86_64.no-builtin-hashes-except-blake2/build/Lib/hashlib.py", line 160, in __hash_new
    return _hashlib.new(name, *args, **kwargs)
           ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
ValueError: [digital envelope routines: EVP_DigestInit_ex] disabled for FIPS


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-fips-x86_64.no-builtin-hashes-except-blake2/build/Lib/test/test_hashlib.py", line 255, in test_clinic_signature
    constructor(b'')
    ~~~~~~~~~~~^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-fips-x86_64.no-builtin-hashes-except-blake2/build/Lib/test/test_hashlib.py", line 145, in c
    return hashlib.new(__algorithm_name, *args, **kwargs)
           ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-fips-x86_64.no-builtin-hashes-except-blake2/build/Lib/hashlib.py", line 166, in __hash_new
    return __get_builtin_constructor(name)(*args, **kwargs)
           ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-fips-x86_64.no-builtin-hashes-except-blake2/build/Lib/hashlib.py", line 123, in __get_builtin_constructor
    raise ValueError('unsupported hash type ' + name)
ValueError: unsupported hash type md5

picnixz · 2025-05-31T09:18:45Z

Well. Of course. This build bot.

…constructors AC signatures (pythonGH-134713) OpenSSL and HACL*-based hash functions constructors now support both `data` and `string` parameters. Previously these constructor functions inconsistently supported sometimes `data` and sometimes `string`, while the documentation expected `data` to be given in all cases. (cherry picked from commit c6e63d9) (cherry picked from commit 379d0bc) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

…uctors AC signatures (GH-134713) (#134961) OpenSSL and HACL*-based hash functions constructors now support both `data` and `string` parameters. Previously these constructor functions inconsistently supported sometimes `data` and sometimes `string`, while the documentation expected `data` to be given in all cases. (cherry picked from commit c6e63d9) (cherry picked from commit 379d0bc)

…uctors AC signatures (GH-134713) (#134962) OpenSSL and HACL*-based hash functions constructors now support both `data` and `string` parameters. Previously these constructor functions inconsistently supported sometimes `data` and sometimes `string`, while the documentation expected `data` to be given in all cases. (cherry picked from commit c6e63d9) (cherry picked from commit 379d0bc)

align clinic signatures of HACL* hash functions with OpenSSL ones

996d232

picnixz requested a review from encukou May 26, 2025 12:43

picnixz requested review from gpshead and tiran as code owners May 26, 2025 12:43

bedevere-app bot mentioned this pull request May 26, 2025

Clinic signatures of HACL* hash functions are inconsistent with OpenSSL implementation #134696

Closed

bedevere-app bot added the awaiting core review label May 26, 2025

Merge remote-tracking branch 'upstream/main' into fix/shake/hacl-impl…

b35ce61

…ementation-clinic-134696

ensure backward compatibility

679f811