Skip to content

refactor: cleanup hash to better support multiple implementations #5258

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

Merged
merged 2 commits into from
Apr 21, 2025

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Apr 18, 2025

Release Summary:

Resolved issues:

related to #5257

Description of changes:

See #5257 for a high-level description of why I need to touch the hashing code.

Handling both hashes and raw input interchangeably requires a new hash implementation. However, our current hash code isn't organized in a way that makes it easy to get consistent behavior between implementations. Before I add the new implementation, I want to reorganize the existing code: the "s2n_hash_" methods should handle the fields shared between all implementations, and the specific "s2n_evp_hash_" methods should only handle the fields in the "digest" union that are specific to the EVP hashing implementation.

Testing:

There should be no behavior changes here, so existing tests should be sufficient. I'm just moving code around.

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 Apr 18, 2025
Comment on lines -416 to -418
/* Ensure that hash_impl is set, as it may have been reset for s2n_hash_state on s2n_connection_wipe.
* When in FIPS mode, the EVP API's must be used for hashes.
*/
Copy link
Contributor Author

@lrstewart lrstewart Apr 18, 2025

Choose a reason for hiding this comment

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

I'm convinced that this isn't currently true, and I'm not sure it was ever true? Wiping a connection doesn't erase the hash_impl-- and if it somehow did, it'd also likely somehow erase all the other state that hash_impl->free is supposed to cleanup and therefore cause a memory leak. hash_impl should be set on _new, when the implementation specific memory is allocated, and then never touched again.

The previous two hash_impls could be chosen between when s2n_init was called, so calling s2n_hash_set_impl repeatedly wasn't an issue. But if we're going to add more complex logic around choosing different hash_impls, then we need to be more clear about the s2n_hash_state lifecycle.

@lrstewart lrstewart marked this pull request as ready for review April 18, 2025 16:59
@lrstewart lrstewart requested review from goatgoose, jmayclin and CarolYeh910 and removed request for jmayclin April 18, 2025 16:59
@lrstewart lrstewart added this pull request to the merge queue Apr 21, 2025
Merged via the queue into aws:main with commit f21c94b Apr 21, 2025
46 checks passed
@lrstewart lrstewart deleted the mldsa_1b_1 branch April 21, 2025 17:22
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