Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
ethomson merged 6 commits intolibgit2:mainfromunknown repositoryOct 21, 2024
Merged

Add OpenSSL-FIPS option to USE_SHA256 CMake flag#6906

ethomson merged 6 commits intolibgit2:mainfromunknown repositoryOct 21, 2024

Conversation

ghost
Copy link

Usage of the deprecatedSHA256_* OpenSSL API in a FIPS compliant environment results in OpenSSL's assertion failure with the following description:

OpenSSL internal error, assertion failed: Low level API call to digest SHA256 forbidden in FIPS mode!

This PR adds a possibility to use the OpenSSL'sEVP_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.

mdabrowskinand others added2 commitsOctober 9, 2024 14:53
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.
@ethomson
Copy link
Member

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.
@ethomson
Copy link
Member

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.

@ethomson
Copy link
Member

If you wouldn't mind taking a look at your leisure to make sure that everything makes sense. 🙏

@ghost
Copy link
Author

LGTM!

ethomson reacted with rocket emoji

@ethomsonethomson merged commit7e3535c intolibgit2:mainOct 21, 2024
51 of 53 checks passed
@ethomson
Copy link
Member

Great; thanks again!

@lstoppa
Copy link
Contributor

I have only one question about the changea7ca589

In particular, the way you setNULL

hash_cb_data.ctx = NULL;error = write_cb((char *)checksum, checksum_size, cb_data);if (error < 0)goto cleanup;cleanup:git_array_clear(object_entries_array);git_vector_dispose(&object_entries);git_str_dispose(&packfile_names);git_str_dispose(&oid_lookup);git_str_dispose(&object_offsets);git_str_dispose(&object_large_offsets);git_hash_ctx_cleanup(&ctx);return error;

is there any reason why we don't dohash_cb_data.ctx = NULL inside thecleanup label? If "it doesn't matter" when we do it, maybe for consistency we should consider to move it there?

@ethomson
Copy link
Member

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.)

@lstoppa
Copy link
Contributor

I see, thanks a lot for your explanation!

@ethomson
Copy link
Member

Happily, it was a very good question that should have been better explained in the code, commit message, or PR.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@ethomson@lstoppa@mdabrowskin

[8]ページ先頭

©2009-2025 Movatter.jp