Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove unused evp support for md5+sha1 #5106

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Feb 11, 2025

Release Summary:

Resolved issues:

related to #5105

Description of changes:

Read #5105 -- this change won't make sense in context without it. After this change:

Libcrypto Hash method Signing Method
openssl-1.0.2 Legacy Legacy
openssl-1.0.2-fips n/a n/a
awslc-fips EVP EVP-FIPS-140-3
openssl-3-fips n/a n/a
other Legacy Legacy

This PR tackles Task 1 from the related issue and cleans up the unused openssl-1.0.2-fips signing logic. This logic was used only for openssl-1.0.2-fips (in fips mode) because that was the only libcrypto:

  1. Using "EVP" hashes because s2n_use_evp_impl returned "true" because s2n-tls was in fips mode
  2. Using "Legacy" signing because s2n_hash_evp_fully_supported returned "false" because the EVP_MD_CTX_set_pkey_ctx and EVP_md5_sha1 methods are missing

There should be NO practical behavior change from this change.

Callouts

But why are we removing the EVP version instead of the legacy version?
Because MD5 is legacy. Even in our perfect end state where almost everything is using EVP, openssl-3.0-fips will still need to use the legacy methods for MD5. Might as well let openssl-1.0.2 use the same legacy code, rather than complicating the EVP code to support openssl-1.0.2's strange MD5+SHA1 needs.

Testing:

Unit tests.
Updated the CBMC tests to test with both the EVP and legacy setups.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Feb 11, 2025
@lrstewart lrstewart mentioned this pull request Feb 11, 2025
9 tasks
@lrstewart lrstewart force-pushed the openssl3fips_evp_1 branch 5 times, most recently from 72b2631 to 99196b9 Compare February 11, 2025 17:30
Comment on lines +20 to +21

bool s2n_evp_signing_supported()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This follows the model set by s2n_is_in_fips_mode and s2n_libcrypto_is_awslc

@lrstewart lrstewart marked this pull request as ready for review February 11, 2025 19:15
crypto/s2n_fips.c Outdated Show resolved Hide resolved
tests/cbmc/proofs/s2n_hmac_free/s2n_hmac_free_harness.c Outdated Show resolved Hide resolved
@lrstewart lrstewart added this pull request to the merge queue Feb 12, 2025
Merged via the queue into aws:main with commit f3ae011 Feb 13, 2025
45 checks passed
@lrstewart lrstewart deleted the openssl3fips_evp_1 branch February 13, 2025 00:37
lrstewart added a commit to lrstewart/s2n that referenced this pull request Feb 14, 2025
lrstewart added a commit to lrstewart/s2n that referenced this pull request Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants