Skip to content

bpo-40645: use C implementation of HMAC #24920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 27, 2021

Conversation

tiran
Copy link
Member

@tiran tiran commented Mar 18, 2021

  • fix tests
  • add test scenarios for old/new code.

Signed-off-by: Christian Heimes [email protected]

https://bugs.python.org/issue40645

Automerge-Triggered-By: GH:tiran

Lib/hmac.py Outdated
_hashopenssl is not None and
(digestname := _hashlib._digestmod_to_name(digestmod))
):
self._init_hmac(key, msg, digestname)
Copy link
Member

Choose a reason for hiding this comment

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

This makes the assumption that if we have _hashopenssl, all possible digests anyone could use will be available as part of openssl for use via its hmac implementation. I don't think that is necessarily true. A digestmod that supplies a string name can be supplied but not be something available in openssl.

presumably the easiest way around this while retaining the optimal openssl computation when possible is to catch an exception from _init_hmac (due to _hashopenssl.hmac_new raising) and fall back to _init_old instead.

a test should be added to cover this case. (via a custom digestmod-like-object with a name and blocksize perhaps)

Lib/hmac.py Outdated
_hashopenssl is not None and
(digestname := _hashlib._digestmod_to_name(digest))
):
return _hashopenssl.hmac_digest(key, msg, digestname)
Copy link
Member

Choose a reason for hiding this comment

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

fallback logic needed here as when digestname isn't supported by openssl.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@tiran
Copy link
Member Author

tiran commented Mar 18, 2021

Thanks for the feedback, @gpshead!

I'm not happy with the first draft. It's not elegant. It's easier to track builtin constructors in C and map them directly in _hashopenssl.c.

tiran added 2 commits March 18, 2021 22:42
Recognize _hashlib.openssl_sha256 function object as "sha256".
@tiran tiran marked this pull request as ready for review March 19, 2021 09:28
@pablogsal
Copy link
Member

pablogsal commented Mar 29, 2021

Seems that commit 933dfd7 introduced some reference leaks:

commit 933dfd7
Author: Christian Heimes [email protected]
Date: Sat Mar 27 14:55:03 2021 +0100

[bpo-40645](https://bugs.python.org/issue40645): use C implementation of HMAC (GH-24920)

𓋹 ./python.exe -m test test_hashlib -R 3:3
0:00:00 load avg: 5.67 Run tests sequentially
0:00:00 load avg: 5.67 [1/1] test_hashlib
beginning 6 repetitions
123456
......
test_hashlib leaked [1, 1, 1] references, sum=3
test_hashlib failed

== Tests result: FAILURE ==

1 test failed:
test_hashlib

Total duration: 8.7 sec
Tests result: FAILURE

- [x] fix tests
- [ ] add test scenarios for old/new code.

Signed-off-by: Christian Heimes <[email protected]>

Lib/hashlib.py | 1 +
Lib/hmac.py | 86 +++++++-----
Lib/test/test_hmac.py | 114 +++++++++-------
.../2021-03-19-10-22-17.bpo-40645.5pXhb-.rst | 2 +
Modules/_hashopenssl.c | 150 +++++++++++++++++++--
Modules/clinic/_hashopenssl.c.h | 38 +-----
6 files changed, 267 insertions(+), 124 deletions(-)
create mode 100644 bpo-40645.5pXhb-.rst">Misc/NEWS.d/next/Library/2021-03-19-10-22-17.bpo-40645.5pXhb-.rst

@pablogsal
Copy link
Member

Opened #25063 to address it

stratakis pushed a commit to stratakis/cpython that referenced this pull request Aug 5, 2021
- [x] fix tests
- [ ] add test scenarios for old/new code.

Signed-off-by: Christian Heimes <[email protected]>
encukou pushed a commit to encukou/cpython that referenced this pull request Sep 8, 2021
…, pythonGH-26079)

This backports the feature and 2 subsequent bugfixes
from: https://bugs.python.org/issue40645

Signed-off-by: Christian Heimes <[email protected]>
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
Co-Authored-By: Pablo Galindo <[email protected]>
stratakis pushed a commit to stratakis/cpython that referenced this pull request Jan 18, 2022
…, pythonGH-26079)

This backports the feature and 2 subsequent bugfixes
from: https://bugs.python.org/issue40645

Signed-off-by: Christian Heimes <[email protected]>
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
Co-Authored-By: Pablo Galindo <[email protected]>
stratakis pushed a commit to stratakis/cpython that referenced this pull request Jan 21, 2022
…, pythonGH-26079)

This backports the feature and 2 subsequent bugfixes
from: https://bugs.python.org/issue40645

Signed-off-by: Christian Heimes <[email protected]>
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
Co-Authored-By: Pablo Galindo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants