NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.7k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

miss-islington merged 7 commits intopython:masterfromtiran:bpo-40645-hmac-310

Mar 27, 2021

Merged

bpo-40645: use C implementation of HMAC#24920

miss-islington merged 7 commits intopython:masterfromtiran:bpo-40645-hmac-310

Mar 27, 2021

Conversation

Copy link

Member

tiran commentedMar 18, 2021•
edited by miss-islington
Loading

fix tests
add test scenarios for old/new code.

Signed-off-by: Christian Heimeschristian@python.org

https://bugs.python.org/issue40645

Automerge-Triggered-By: GH:tiran

bpo-40645: use C implementation of HMAC

5cb5319

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

the-knights-who-say-ni added the CLA signed label

Mar 18, 2021

bedevere-bot added the awaiting core review label

Mar 18, 2021

gpshead requested changes

Mar 18, 2021

View reviewed changes

Lib/hashlib.py OutdatedShow resolvedHide resolved

Lib/hmac.py Outdated

		_hashopensslisnotNoneand
		(digestname:=_hashlib._digestmod_to_name(digestmod))
		):
		self._init_hmac(key,msg,digestname)

Copy link

Member

gpsheadMar 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 OutdatedShow resolvedHide resolved

Lib/hmac.py Outdated

		_hashopensslisnotNoneand
		(digestname:=_hashlib._digestmod_to_name(digest))
		):
		return_hashopenssl.hmac_digest(key,msg,digestname)

Copy link

Member

gpsheadMar 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 added awaiting changes and removed awaiting core review labels

Mar 18, 2021

Copy link

bedevere-bot commentedMar 18, 2021

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

Copy link

MemberAuthor

tiran commentedMar 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 added2 commits

March 18, 2021 22:42

Better approach to convert digestmod to EVP

6dd9fa9

Recognize _hashlib.openssl_sha256 function object as "sha256".

Address review comments

3782553

gpshead requested changes

Mar 18, 2021

View reviewed changes

Lib/hmac.pyShow resolvedHide resolved

Modules/_hashopenssl.c OutdatedShow resolvedHide resolved

tiran added4 commits

March 19, 2021 09:22

Fix test cases

f76bf5d

Test with non-openssl digest module

504e08e

Add new internal exception to indicate unsupported constructor

83ae2d3

blurb

9ff2d86

tiran marked this pull request as ready for review

March 19, 2021 09:28

gpshead approved these changes

Mar 19, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels

Mar 19, 2021

tiran added the 🤖 automerge label

Mar 27, 2021

miss-islington merged commit933dfd7 intopython:master

Mar 27, 2021

bedevere-bot removed the awaiting merge label

Mar 27, 2021

Copy link

Member

pablogsal commentedMar 29, 2021•
edited by bedevere-bot
Loading

Seems that commit933dfd7 introduced some reference leaks:

commit933dfd7
Author: Christian Heimeschristian@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 100644bpo-40645.5pXhb-.rst">Misc/NEWS.d/next/Library/2021-03-19-10-22-17.bpo-40645.5pXhb-.rst

Copy link

Member

pablogsal commentedMar 29, 2021

Opened#25063 to address it

stratakis pushed a commit to stratakis/cpython that referenced this pull request

Aug 5, 2021

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

bpo-40645: use C implementation of HMAC (pythonGH-24920,pythonGH-25063…

62debb2

…,pythonGH-26079)This backports the feature and 2 subsequent bugfixesfrom:https://bugs.python.org/issue40645Signed-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

bpo-40645: use C implementation of HMAC (pythonGH-24920,pythonGH-25063…

6b98f14

…,pythonGH-26079)This backports the feature and 2 subsequent bugfixesfrom:https://bugs.python.org/issue40645Signed-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

bpo-40645: use C implementation of HMAC (pythonGH-24920,pythonGH-25063…

d0903aa

…,pythonGH-26079)This backports the feature and 2 subsequent bugfixesfrom:https://bugs.python.org/issue40645Signed-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>