-
Notifications
You must be signed in to change notification settings - Fork 722
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: always use EVP hashing #5121
Conversation
429374c
to
f3fb17c
Compare
f3fb17c
to
911c7ca
Compare
/* The s2n hash implementation is abstracted to allow for separate implementations, using | ||
* either OpenSSL's low-level algorithm-specific API's or OpenSSL's EVP API's. | ||
/* The s2n hash implementation is abstracted to allow for separate implementations. | ||
* Currently the only implementation uses the EVP APIs. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we only have one implementation now, but it seemed like a bit of a waste to rip out the mechanism to handle multiple implementations. We might need another implementation someday, and I don't think this abstraction adds much complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running our s2n_mem_usage_test with outputs, I see "VmData after handshakes" as 24391680 before this change and 23609344 after. With 252 connections used for testing, that's ~3k bytes saved per connection.
Should the thresholds in s2n_mem_usage_test
be reduced at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's enough of a benefit for that.
Min VmData diff allowed: 21288960
Max VmData diff allowed: 28385280
So 24391680 was pretty much right in the middle of that window, and 23609344 doesn't seem like enough of an adjustment to deal with updating the test, which can be tricky to get right for all envs :(
Maybe I should update it in a separate PR, so we can easily revert it if it causes issues? There are some other tweaks I'd like to try to make to that test to make it less difficult to deal with.
Release Summary:
Reduced connection memory usage by an estimated 4 to 5 percent
Resolved issues:
related to #5105
Description of changes:
This PR completely removes the legacy hashing logic, leaving all libcryptos using EVP.
The state of the library after this change:
Call-outs:
As a fun little bonus, this should also save some memory. The low-level hash state increased the size of s2n_hash_state by ~200 bytes. We maintain 8 hashes for the handshake transcript. That's 200 * 8 = 1600 bytes saves 🎉 Each s2n_hmac_state also has 4 hashes, but I'm not sure how many hmacs we keep around. At least one for the client and server, so that's another 200 * 4 * 2 = 1600 bytes?
Running our s2n_mem_usage_test with outputs, I see "VmData after handshakes" as 24391680 before this change and 23609344 after. With 252 connections used for testing, that's ~3k bytes saved per connection , or 4.5% by my math.
Testing:
Existing tests pass, so hashing continues to work with all tested libcryptos.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.