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 prf hmac impls #5148

Merged
merged 3 commits into from
Feb 28, 2025

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Feb 26, 2025

Release Summary:

Resolved issues:

related to #5143

Description of changes:

It looks like the PRF uses 3 different HMAC implementations, but in reality it only uses one. This PR removes the unused implementations for clarity.

The important code is:

s2n-tls/tls/s2n_prf.c

Lines 392 to 399 in 711ee0d

const struct s2n_p_hash_hmac *s2n_get_hmac_implementation()
{
#if defined(OPENSSL_IS_BORINGSSL) || defined(OPENSSL_IS_AWSLC)
return s2n_is_in_fips_mode() ? &s2n_evp_hmac_p_hash_hmac : &s2n_internal_p_hash_hmac;
#else
return s2n_is_in_fips_mode() ? &s2n_evp_pkey_p_hash_hmac : &s2n_internal_p_hash_hmac;
#endif
}

So we appear to have:

libcrypto hmac impl
awslc-fips s2n_evp_hmac_p_hash_hmac
awslc s2n_internal_p_hash_hmac
openssl-1.0.2-fips s2n_evp_pkey_p_hash_hmac
other s2n_internal_p_hash_hmac

BUT:

  1. FIPS mode is no longer supported for openssl-1.0.2-fips. So s2n_evp_pkey_p_hash_hmac is never used.
  2. awslc-fips doesn't actually use an hmac implementation for its prf. We only use the hmac implementation for our "custom" prf, and awslc-fips uses the libcrypto's PRF as required by FIPS 140-3. We branch here, with s2n_libcrypto_supports_tls_prf only/always true for awslc-fips because of this define.

So we ACTUALLY only have s2n_internal_p_hash_hmac.

Call-outs:

What about sslv3?
You might have noticed that we branch on sslv3 before we branch on on whether to use the libcrypto or custom PRF. That's not an issue. The sslv3 prf doesn't use the hmac implementation either (see the source. It's all hashes). Also, nothing we do to sslv3 is going to make it any more FIPS compliant :)

Testing:

Existing tests still pass.

I also proved that the other hmac implementation weren't being used by replacing their "final" methods with errors: c1f7183 Any meaningful use of hmac requires the "final" method, since that's the only way to actually produce output. The CI still passed (see the little green check next to the commit name-- it can be expanded to see the passing tests).

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 26, 2025
@lrstewart lrstewart force-pushed the openssl3fips_prf_remove branch 4 times, most recently from c1f7183 to 5a0917f Compare February 26, 2025 23:59
@lrstewart lrstewart changed the title Confirm whether 2/3 prf hmac impls can be removed refactor: remove unused prf hmac impls Feb 26, 2025
@lrstewart lrstewart force-pushed the openssl3fips_prf_remove branch from 5a0917f to 4708378 Compare February 27, 2025 00:32
@lrstewart lrstewart marked this pull request as ready for review February 27, 2025 00:34
@lrstewart lrstewart force-pushed the openssl3fips_prf_remove branch from 7838593 to 20a6819 Compare February 27, 2025 19:34
@lrstewart lrstewart force-pushed the openssl3fips_prf_remove branch from 1b16375 to 7f24d45 Compare February 28, 2025 04:02
@lrstewart lrstewart added this pull request to the merge queue Feb 28, 2025
Merged via the queue into aws:main with commit 3b16449 Feb 28, 2025
46 checks passed
@lrstewart lrstewart deleted the openssl3fips_prf_remove branch February 28, 2025 23:51
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