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-99108: Import SHA2-224 and SHA2-256 from HACL* by msprotz · Pull Request #99109 · python/cpython

Conversation

@msprotz

@msprotz msprotz commented Nov 4, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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 PR
LibTomCrypt_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 into Modules/_hacl
  • rewire sha256module.c to use the HACL* implementation

To review the changes, one should focus on sha256module.c, and possibly Hacl_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 from https://github.com/project-everest/hacl-star/tree/master/dist/gcc-compatible. The various .h files in include/, 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
msprotz added 4 commits November 4, 2022 14:25
@msprotz
Replace SHA2 implementation with verified code from HACL*
f941537
@msprotz
Fixup some casts
89712f6
@msprotz
And proper error handling for maximum hashing lengths exceeded
21fddf1
@msprotz
Remove extra file
6c5498e
@msprotz
msprotz requested review from a team and tiran as code owners November 4, 2022 22:00
@bedevere-bot bedevere-bot mentioned this pull request Nov 4, 2022
Replace built-in hashlib with verified implementations from HACL* #99108
Closed
@bedevere-bot bedevere-bot added the awaiting review label Nov 4, 2022
@bedevere-bot

bedevere-bot commented Nov 4, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

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

ghost commented Nov 4, 2022 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msprotz
See if using srcdir helps
77c682c
@bedevere-bot

bedevere-bot commented Nov 4, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead self-requested a review November 5, 2022 01:54
msprotz added 2 commits November 7, 2022 09:13
@msprotz
Delete 11 un-needed files
1586188
@msprotz
Wrong direction for error checking
621ef0d
@bedevere-bot

bedevere-bot commented Nov 7, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msprotz
Remove another four files
81cac0f
@bedevere-bot

bedevere-bot commented Nov 7, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msprotz
better code quality
5134c6a
@bedevere-bot

bedevere-bot commented Nov 7, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msprotz
Fixup memory management mistake
f212447
@bedevere-bot

bedevere-bot commented Nov 7, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msprotz
Fix Linux build
6543fdd
@bedevere-bot

bedevere-bot commented Nov 7, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead reviewed Nov 7, 2022
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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gpshead

gpshead commented Nov 7, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tiran
tiran reviewed Nov 8, 2022
View reviewed changes

@tiran tiran 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.

Please add all new headers to MODULE__SHA256_DEPS in Makefile.pre.in.

I would prefer if you keep the sha256module.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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/Setup.stdlib.in Resolved

Uh oh!

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

franziskuskiefer commented Nov 8, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

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

gpshead commented Nov 8, 2022

Copy link
Copy Markdown
Member

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 of also getting rid of the OpenSSL backed _hashlib in the long run: not today.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msprotz
Address review comments from @tiran; regenerate configure
a52e9ce
@gpshead
Update to the latest HACL* revision.
f33e2ef 
Which seems to just be a copyright message update.
@gpshead
gpshead requested review from corona10 and erlend-aasland as code owners January 31, 2023 05:01
gpshead added 7 commits January 30, 2023 21:09
@gpshead
Add a README to the _hacl directory.
ce292e7
@gpshead
reword and ReSTify the news entry.
aa24fd4
@gpshead
Dynamically rename the Hacl_ C symbols.
29fd3c9 
We do this with other third party code as well, it avoids linking and
dynamic linking ODR violations when extension module or embedding
applications 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.
@gpshead
missing file add & readme update.
0362c1a
@gpshead
_
0f33a6a
@gpshead
remove wrongly placed defines.
d7c1240
@gpshead
Merge branch 'main' into sha2_hacl
9ec37a6
@gpshead

gpshead commented Jan 31, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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" the Hacl_ 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 no Hacl_ C symbols exist in _sha256.so or _sha256.pyd... but that can remain a future todo wishlist item for now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead self-assigned this Jan 31, 2023
@gpshead gpshead added extension-modules C modules in the Modules dir 3.12 only security fixes labels Jan 31, 2023
gpshead
gpshead approved these changes Jan 31, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Jan 31, 2023
erlend-aasland
erlend-aasland reviewed Jan 31, 2023
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

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

Autoconf changes looks good to me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/sha256module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@msprotz @erlend-aasland
Stylistic fix in Modules/sha256module.c
92033ad 
Co-authored-by: Erlend E. Aasland <[email protected]>
@msprotz

msprotz commented Jan 31, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Jan 31, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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).
msprotz 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.
@msprotz
Fix a warning on Windows builders
c670530
@msprotz

msprotz commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead approved these changes Feb 7, 2023
View reviewed changes

@gpshead gpshead left a comment •
edited
Loading

Uh oh!

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

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@gpshead
gpshead merged commit 1fcc0ef into python:main Feb 7, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 7, 2023
@msprotz

msprotz commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

Yup! I'll respond on the issue so that we don't scatter the overall discussion across individual PRs.
erlend-aasland 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.
@msprotz msprotz mentioned this pull request Feb 8, 2023
gh-99108: Import SHA2-384/512 from HACL* #101707
Merged
@asottile asottile mentioned this pull request Feb 11, 2023
add hacl2 files to sha256module in Modules/Setup #101829
Closed
@arhadthedev arhadthedev mentioned this pull request Apr 1, 2023
gh-102204: Fix endianness issue in AIX #102206
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments
@gpshead gpshead gpshead approved these changes
@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner
+1 more reviewer
@tiran tiran tiran left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@gpshead gpshead

Labels

3.12 only security fixes extension-modules C modules in the Modules dir

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.

7 participants

@msprotz @bedevere-bot @gpshead @franziskuskiefer @tiran @gvanrossum @erlend-aasland
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.