GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-134696: align OpenSSL and HACL*-based hash functions constructors AC signatures by picnixz · Pull Request #134713 · python/cpython

Conversation

@picnixz

@picnixz picnixz commented May 26, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@picnixz
align clinic signatures of HACL* hash functions with OpenSSL ones
996d232
@picnixz
picnixz requested a review from encukou May 26, 2025 12:43
@picnixz
picnixz requested review from gpshead and tiran as code owners May 26, 2025 12:43
@bedevere-app 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 bedevere-app Bot added the awaiting core review label May 26, 2025
@picnixz
Merge remote-tracking branch 'upstream/main' into fix/shake/hacl-impl…
b35ce61 
…ementation-clinic-134696
@gpshead

gpshead commented May 26, 2025

Copy link
Copy Markdown
Member

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.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz

picnixz commented May 26, 2025

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
ensure backward compatibility
679f811
@picnixz

picnixz commented May 27, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz commented May 27, 2025
View reviewed changes
Comment thread Lib/hashlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
Update Lib/hashlib.py
569d63e
picnixz
picnixz commented May 27, 2025
View reviewed changes
Comment thread Lib/hashlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
remove impossible paths
1699b91
picnixz
picnixz commented May 27, 2025
View reviewed changes
Comment thread Lib/hashlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
Handle more ambiguous cases
a6f8ee7
picnixz
picnixz commented May 27, 2025
View reviewed changes
Comment thread Lib/test/test_hashlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
Update Lib/test/test_hashlib.py
f68ea02
picnixz
picnixz commented May 27, 2025
View reviewed changes
Comment thread Lib/hashlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
Update Lib/hashlib.py
0724bdd
picnixz
picnixz commented May 27, 2025
View reviewed changes
Comment thread Lib/hashlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
Update Lib/hashlib.py
18de16e
picnixz
picnixz commented May 27, 2025
View reviewed changes
Comment thread Lib/test/test_hashlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
Update Lib/test/test_hashlib.py
fdc384b
picnixz
picnixz commented May 27, 2025
View reviewed changes
Comment thread Lib/test/test_hashlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
fix tests
bcd6d4d
@picnixz

picnixz commented May 27, 2025

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz

picnixz commented May 30, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz picnixz added the DO-NOT-MERGE label May 30, 2025
@bedevere-app bedevere-app Bot added the awaiting merge label May 30, 2025
@picnixz

picnixz commented May 30, 2025

Copy link
Copy Markdown
Member Author

In a follow-up PR, I'll keep [data] and deprecate 'string'. I don't think I'll make it positional-only.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead mentioned this pull request May 31, 2025
Argument Clinic: add support for renaming a parameter #108271
Closed
gpshead
gpshead approved these changes May 31, 2025
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka

serhiy-storchaka commented May 31, 2025

Copy link
Copy Markdown
Member

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
picnixz merged commit c6e63d9 into python:main May 31, 2025
@picnixz
picnixz deleted the fix/shake/hacl-implementation-clinic-134696 branch May 31, 2025 07:37
@bedevere-app bedevere-app Bot removed the awaiting merge label May 31, 2025
@picnixz picnixz added needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes labels May 31, 2025
@miss-islington-app

miss-islington-app Bot commented May 31, 2025

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

miss-islington-app Bot commented May 31, 2025

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

miss-islington-app Bot commented May 31, 2025

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned picnixz May 31, 2025
@miss-islington-app

miss-islington-app Bot commented May 31, 2025

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz added a commit to picnixz/cpython that referenced this pull request May 31, 2025
@picnixz
[3.14] pythongh-134696: align OpenSSL and HACL*-based hash functions …
8235738 
…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 <[email protected]>
@bedevere-app

bedevere-app Bot commented May 31, 2025

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label May 31, 2025
picnixz added a commit to picnixz/cpython that referenced this pull request May 31, 2025
@picnixz
[3.13] pythongh-134696: align OpenSSL and HACL*-based hash functions …
4ba757a 
…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 <[email protected]>
@bedevere-app

bedevere-app Bot commented May 31, 2025

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label May 31, 2025
@picnixz picnixz changed the title gh-134696: align clinic signatures in HACL* modules with OpenSSL ones gh-134696: align OpenSSL and HACL*-based hash functions constructors AC signatures May 31, 2025
@bedevere-bot

bedevere-bot commented May 31, 2025

Copy link
Copy Markdown

⚠️⚠️⚠️ 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:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (c6e63d9) or if it is a false positive.
  5. 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz

picnixz commented May 31, 2025

Copy link
Copy Markdown
Member Author

Well. Of course. This build bot.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz added a commit to picnixz/cpython that referenced this pull request May 31, 2025
@picnixz
[3.14] pythongh-134696: align OpenSSL and HACL*-based hash functions …
ab91460 
…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 <[email protected]>
picnixz added a commit to picnixz/cpython that referenced this pull request May 31, 2025
@picnixz
[3.13] pythongh-134696: align OpenSSL and HACL*-based hash functions …
ad4c913 
…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 <[email protected]>
picnixz added a commit that referenced this pull request Jun 1, 2025
@picnixz
[3.14] gh-134696: align OpenSSL and HACL*-based hash functions constr…
777fd49 
…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)
picnixz added a commit that referenced this pull request Jun 1, 2025
@picnixz
[3.13] gh-134696: align OpenSSL and HACL*-based hash functions constr…
2c325e2 
…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)
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@picnixz @Pranjal095
pythongh-134696: align OpenSSL and HACL*-based hash functions constru…
cd0fc32 
…ctors AC signatures (python#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.
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@picnixz @taegyunkim
pythongh-134696: align OpenSSL and HACL*-based hash functions constru…
f7105df 
…ctors AC signatures (python#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@encukou encukou Awaiting requested review from encukou
@tiran tiran Awaiting requested review from tiran

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@picnixz picnixz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@picnixz @gpshead @serhiy-storchaka @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.