NotificationsYou must be signed in to change notification settings
Fork32.1k
Star67.3k

gh-99108: Import SHA2-224 and SHA2-256 from HACL*#99109

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

gpshead merged 36 commits intopython:mainfrommsprotz:sha2_hacl

Feb 7, 2023

Merged

gh-99108: Import SHA2-224 and SHA2-256 from HACL*#99109

gpshead merged 36 commits intopython:mainfrommsprotz:sha2_hacl

Feb 7, 2023

Conversation

Copy link

Contributor

msprotz commentedNov 4, 2022•
edited by bedevere-bot
Loading

Issue#99108 is about replacing hashlib primitives (for the non-OpenSSL case) with verified implementations from HACL*. This is the first PR in the series, and focuses specifically on SHA2-256 and SHA2-224.

This PR imports Hacl_Streaming_SHA2 into the Python tree. This is the HACL* implementation of SHA2, which combines a core implementation of SHA2 along with a layer of buffer management that allows updating the digest with any number of bytes. This supersedes the previous implementation in the tree.

@franziskuskiefer was kind enough to benchmark the changes: in addition to being verified (thus providing significant safety and security improvements), this implementation also provides a sizeable performance boost!

---------------------------------------------------------------Benchmark                     Time             CPU   Iterations---------------------------------------------------------------Sha2_256_Streaming            3163 ns      3160 ns       219353     // this PRLibTomCrypt_Sha2_256          5057 ns      5056 ns       136234     // library used by Python currently

The changes in this PR are as follows:

import the subset of HACL* that covers SHA2-256/224 intoModules/_hacl
rewire sha256module.c to use the HACL* implementation

To review the changes, one should focus onsha256module.c, and possiblyHacl_Streaming_SHA2.h which is the API entry point. The rest of the files are part of HACL* and are cherry-picked straight from the HACL* repository, so as to minimize the amount of hand-edits and facilitate future refreshes of the code, should HACL* land some performance improvements.

The original code comes fromhttps://github.com/project-everest/hacl-star/tree/master/dist/gcc-compatible. The various .h files ininclude/,krmllib/, etc. will be shared across (hopefully) future primitives imported from HACL*. I can trim those files to minimize the amount of includes, but then this will make refreshing the code in the future harder (since the trimming will have to be redone for each refresh).

The code was originally authored by@karthikbhargavan and I;@polubelova and@R1kM provided a considerable amount of help over the past week, as we were overhauling our implementation to offer a significant performance boost (we want this first PR to be a good one!). Thanks to everyone involved in this team effort.

There are a couple remarks left in the source code, but for now, I think it's better to open up a PR and start discussing. As promised, tagging@alex for this PR. Thanks so much!

Issue:Replace built-in hashlib with verified implementations from HACL* #99108

msprotz added4 commits

November 4, 2022 14:25

Replace SHA2 implementation with verified code from HACL*

f941537

Fixup some casts

89712f6

And proper error handling for maximum hashing lengths exceeded

21fddf1

Remove extra file

6c5498e

msprotz requested review froma team andtiran ascode owners

November 4, 2022 22:00

bedevere-bot mentioned this pull request

Nov 4, 2022

Replace built-in hashlib with verified implementations from HACL*#99108

Closed

bedevere-bot added the awaiting review label

Nov 4, 2022

Copy link

bedevere-bot commentedNov 4, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

Copy link

ghost commentedNov 4, 2022•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.

See if using srcdir helps

77c682c

Copy link

bedevere-bot commentedNov 4, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

gpshead self-requested a review

November 5, 2022 01:54

msprotz added2 commits

November 7, 2022 09:13

Delete 11 un-needed files

1586188

Wrong direction for error checking

621ef0d

Copy link

bedevere-bot commentedNov 7, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

Remove another four files

81cac0f

Copy link

bedevere-bot commentedNov 7, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

better code quality

5134c6a

Copy link

bedevere-bot commentedNov 7, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

Fixup memory management mistake

f212447

Copy link

bedevere-bot commentedNov 7, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

Fix Linux build

6543fdd

Copy link

bedevere-bot commentedNov 7, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

gpshead reviewed

Nov 7, 2022

View reviewed changes

Copy link

Member

gpshead left a comment

Choose a reason for hiding this comment

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

It'd be useful for this kind of thing to include a script (somewhere under Tools?.. where to put it can be figured out later) that can generate and update the hacl-star vendored code implementation. As I assume we'll not want this to be a one time code drop but periodically update it, at least for every major Python release, as hacl-star evolves.

Copy link

Member

gpshead commentedNov 7, 2022

Please include a benchmark comparing to the OpenSSL based sha2 as well.

tiran reviewed

Nov 8, 2022

View reviewed changes

Copy link

Member

tiran left a comment

Choose a reason for hiding this comment

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

Please add all new headers toMODULE__SHA256_DEPS inMakefile.pre.in.

I would prefer if you keep thesha256module.c file in its current place and only replace libtom code with HACL calls. It's easier to review and keeps the history for untouched lines.

Modules/Setup.stdlib.in OutdatedShow resolvedHide resolved

Copy link

franziskuskiefer commentedNov 8, 2022

Please include a benchmark comparing to the OpenSSL based sha2 as well.

Adding to the numbers in the PR description#99109 (comment) we get the following. Note that OpenSSL is compiled without assembly support (no-asm no-hw) to get a fair comparison.

OpenSSL_Sha2_256           3000 ns         2999 ns       233849

Copy link

Member

gpshead commentedNov 8, 2022

Note that OpenSSL is compiled without assembly support (no-asm no-hw) to get a fair comparison.

LOL, thanks, but fair enough. 😄 This does at least answer my broader question about the feasibility ofalso getting rid of the OpenSSL backed_hashlib in the long run: not today.

Address review comments from@tiran; regenerate configure

a52e9ce

Update to the latest HACL* revision.

f33e2ef

Which seems to just be a copyright message update.

gpshead requested review fromerlend-aasland andcorona10 ascode owners

January 31, 2023 05:01

gpshead added7 commits

January 30, 2023 21:09

Add a README to the _hacl directory.

ce292e7

reword and ReSTify the news entry.

aa24fd4

Dynamically rename the Hacl_ C symbols.

29fd3c9

We do this with other third party code as well, it avoids linking anddynamic linking ODR violations when extension module or embeddingapplications also use their own version of the library code in question.+ Minor annotation on the test, use `python -m test -u cpu`+ Add a link to the readme.

missing file add & readme update.

0362c1a

_

0f33a6a

remove wrongly placed defines.

d7c1240

Merge branch 'main' into sha2_hacl

9ec37a6

Copy link

Member

gpshead commentedJan 31, 2023•
edited
Loading

Okay, I think this is ready. I pushed a few changes to the PR branch:

Improvements to refresh.sh.
A README.md in Modules/_hacl/ to go along with this third party code.
Used C#define hacks to "namespace" theHacl_ prefixed symbols from the .c file so that they can't cause ODR violations if a process winds up linking with other code that linked against its own version of HACL*.(we do this on some of our other third party things within CPython).

The list confined to its own .h file was manually generated and thus needs manual updating, but I doubt the list will change often. I'd like a test to confirm that noHacl_ C symbols exist in_sha256.so or_sha256.pyd... but that can remain a future todo wishlist item for now.

gpshead self-assigned this

Jan 31, 2023

gpshead added extension-modules

C modules in the Modules dir

3.12only security fixes labels

Jan 31, 2023

gpshead approved these changes

Jan 31, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels

Jan 31, 2023

erlend-aasland reviewed

Jan 31, 2023

View reviewed changes

Copy link

Contributor

erlend-aasland left a comment

Choose a reason for hiding this comment

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

One small style nit. Also, there's a cast warning on line 127 in Modules/sha256module.c.

Autoconf changes looks good to me.

Modules/sha256module.c OutdatedShow resolvedHide resolved

Stylistic fix in Modules/sha256module.c

92033ad

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Copy link

ContributorAuthor

msprotz commentedJan 31, 2023

Thanks both@gpshead and@erlend-aasland!

@erlend-aasland what warning are you seeing? Is it a cast from python's size_t to uint32_t? I wasn't able to reproduce (neither on gcc-11/x86_64/Linux, nor {clang,gcc-12}/x86_64/OSX), but will happily add a cast if that's what you're seeing.

Copy link

Contributor

erlend-aasland commentedJan 31, 2023•
edited
Loading

Yeah, it's a cast warning. You'll see it in the diff tab on the PR; it's one of the GitHub windows runners that's complaining, IIRC (I'm on mobile now).

Fix a warning on Windows builders

c670530

Copy link

ContributorAuthor

msprotz commentedFeb 7, 2023

I fixed the final review comments...@erlend-aasland do I need to re-request a review from you, or is there anything I can do? thanks!

gpshead approved these changes

Feb 7, 2023

View reviewed changes

Copy link

Member

gpshead left a comment•
edited
Loading

Choose a reason for hiding this comment

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

This PR is in good shape! But on its own - lets call this step "2a" - it'll be a little odd to ship in 3.12 if we don't also have some natural related followups to include as well:

2b) SHA384 & SHA512 support (sha512module.c and related hacl code).
3) SHA3 support (more generated hacl code).

I'll go ahead and merge this, and be responsible for reverting it if we decide we don't wind up just the shorter half of SHA2 using it if the follow-ons aren't ready yet before 3.12-beta1 in May.

Could you go ahead and prepare follow-on PRs?