Skip to content
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

Jump to bottom

bpo-40645: use C implementation of HMAC #24920

Merged
merged 7 commits into from Mar 27, 2021
Merged

bpo-40645: use C implementation of HMAC #24920

merged 7 commits into from Mar 27, 2021

Conversation

tiran
Copy link
Member

@tiran tiran commented Mar 18, 2021 •
edited by miss-islington

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

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue40645

Automerge-Triggered-By: GH:tiran

@tiran
bpo-40645: use C implementation of HMAC
5cb5319 
Signed-off-by: Christian Heimes <christian@python.org>
gpshead
gpshead requested changes Mar 18, 2021
View changes
Lib/hashlib.py Outdated Show resolved Hide resolved
Lib/hmac.py Outdated
_hashopenssl is not None and
(digestname := _hashlib._digestmod_to_name(digestmod))
):
self._init_hmac(key, msg, digestname)
Copy link
Member

@gpshead gpshead Mar 18, 2021

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 Show resolved Hide resolved
Lib/hmac.py Outdated Show resolved Hide resolved
Lib/hmac.py Outdated Show resolved Hide resolved
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

@gpshead gpshead Mar 18, 2021

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

bedevere-bot commented Mar 18, 2021

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 Mar 18, 2021
@tiran
Better approach to convert digestmod to EVP
6dd9fa9 
Recognize _hashlib.openssl_sha256 function object as "sha256".
@tiran
Address review comments
3782553
gpshead
gpshead requested changes Mar 18, 2021
View changes
Lib/hmac.py Show resolved Hide resolved
Modules/_hashopenssl.c Outdated Show resolved Hide resolved
@tiran tiran marked this pull request as ready for review Mar 19, 2021
@tiran tiran added the 🤖 automerge PR will be merged once it's been approved and all CI passed label Mar 27, 2021
@miss-islington miss-islington merged commit 933dfd7 into python:master Mar 27, 2021
11 checks passed
@pablogsal
Copy link
Member

pablogsal commented Mar 29, 2021 •
edited by bedevere-bot

Seems that commit 933dfd7 introduced some reference leaks:

commit 933dfd7
Author: Christian Heimes christian@python.org
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 <christian@python.org>

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

pablogsal commented Mar 29, 2021

Opened #25063 to address it

kreathon pushed a commit to kreathon/cpython that referenced this pull request May 2, 2021
@tiran @kreathon
bpo-40645: use C implementation of HMAC (pythonGH-24920)
f82cd68 
- [x] fix tests
- [ ] add test scenarios for old/new code.

Signed-off-by: Christian Heimes <christian@python.org>
stratakis pushed a commit to stratakis/cpython that referenced this pull request Aug 5, 2021
@tiran @stratakis
bpo-40645: use C implementation of HMAC (pythonGH-24920)
10c1b5b 
- [x] fix tests
- [ ] add test scenarios for old/new code.

Signed-off-by: Christian Heimes <christian@python.org>
encukou pushed a commit to encukou/cpython that referenced this pull request Sep 8, 2021
@tiran @pablogsal @encukou
bpo-40645: use C implementation of HMAC (pythonGH-24920, pythonGH-25063…
62debb2 
…, pythonGH-26079)

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

Signed-off-by: Christian Heimes <christian@python.org>
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-Authored-By: Pablo Galindo <Pablogsal@gmail.com>
stratakis pushed a commit to stratakis/cpython that referenced this pull request Jan 18, 2022
@tiran @pablogsal @stratakis
bpo-40645: use C implementation of HMAC (pythonGH-24920, pythonGH-25063…
6b98f14 
…, pythonGH-26079)

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

Signed-off-by: Christian Heimes <christian@python.org>
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-Authored-By: Pablo Galindo <Pablogsal@gmail.com>
stratakis pushed a commit to stratakis/cpython that referenced this pull request Jan 21, 2022
@tiran @pablogsal @stratakis
bpo-40645: use C implementation of HMAC (pythonGH-24920, pythonGH-25063…
d0903aa 
…, pythonGH-26079)

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

Signed-off-by: Christian Heimes <christian@python.org>
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-Authored-By: Pablo Galindo <Pablogsal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead

Assignees
No one assigned
Labels
🤖 automerge PR will be merged once it's been approved and all CI passed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tiran @bedevere-bot @pablogsal @gpshead @the-knights-who-say-ni @miss-islington