- Notifications
You must be signed in to change notification settings - Fork2.5k
Add OpenSSL-FIPS option to USE_SHA256 CMake flag#6906
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Usage of the deprecated 'SHA256_*' OpenSSL API in a FIPS compliantenvironment results in OpenSSL's assertion failure with the followingdescription:"OpenSSL internal error, assertion failed: Low level API call to digest SHA256 forbidden in FIPS mode!"This commit adds a possibility to use the OpenSSL's 'EVP_MD*' API insteadof the deprecated 'SHA256_*' API, by extending the optional CMake flag'USE_SHA256' with the new option called 'OpenSSL-FIPS'.The new option is used to choose a hashing backend used by libgit2 tocalculate SHA256 hashes, in a similar way that currently existingoptions like 'OpenSSL', 'OpenSSL-Dynamic', 'mbedTLS' etc do.'OpenSSL-FIPS' is a fully opt-in option which is purposely notinterfering with the existing options, because, after running somebenchmarks, it's been discovered that using the 'EVP_MD*' API causeshashing to be a bit slower in comparison to using the deprecated'SHA256_*' API.Another change introduced in this commit is the enhancement of theNightly workflow (nightly.yml) which will cause libgit2 to beautomatically built with '-DUSE_SHA256="OpenSSL-FIPS"' CMake flag,on Linux, macOS and Windows.
Thanks for the PR 👀 |
Clear the FIPS-compatible OpenSSL context when completed; this will aidedebugging during improper usage.
Ensure that we don't update the finalized commit graph hash; clear itinstead.
Ensure that we don't update the finalized commit graph hash; clear itinstead.
I made a few minor changes - I wanted to make sure that we supported FIPS mode on SHA1 as well, to ensure that we had more test coverage. Doing so actually unearthed a problem in the MIDX and commit graph code where we were finalizing the hash context and then trying to update it. For most of the hashes that's actually okay, but not for the new FIPS compliant hash. |
If you wouldn't mind taking a look at your leisure to make sure that everything makes sense. 🙏 |
LGTM! |
7e3535c
intolibgit2:mainUh oh!
There was an error while loading.Please reload this page.
Great; thanks again! |
I have only one question about the changea7ca589 In particular, the way you set
is there any reason why we don't do |
It does matter; we need to clear itbefore the write callback. If it's null, the write callback will not try to calculate the hash, it will just do the write. This is important in the final step, which is writing the calculated hash. We need to not try to add the hash itself to the calculate of the hash. (In fact we've already finalized the hash context. In all the other hash implementations, it's okay to continue asking it to hash despite finalization, that's no longer true in the FIPS style routines.) |
I see, thanks a lot for your explanation! |
Happily, it was a very good question that should have been better explained in the code, commit message, or PR. |
Usage of the deprecated
SHA256_*
OpenSSL API in a FIPS compliant environment results in OpenSSL's assertion failure with the following description:This PR adds a possibility to use the OpenSSL's
EVP_MD*
API instead of the deprecatedSHA256_*
API, by extending the optional CMake flagUSE_SHA256
with the new option calledOpenSSL-FIPS
. The new option is used to choose a hashing backend used bylibgit2
to calculate SHA256 hashes, in a similar way that currently existing options likeOpenSSL
,OpenSSL-Dynamic
,mbedTLS
etc do.OpenSSL-FIPS
is a fully opt-in option which is purposely not interfering with the existing options, because, after running some benchmarks, it's been discovered that using theEVP_MD*
API causes hashing to be a bit slower in comparison to using the deprecatedSHA256_*
API.Another change introduced in this PR is the enhancement of the Nightly workflow (
nightly.yml
) which will causelibgit2
to be automatically built with-DUSE_SHA256="OpenSSL-FIPS"
CMake flag, on Linux, macOS and Windows.