-
Notifications
You must be signed in to change notification settings - Fork 727
refactor: always use EVP hashing #5121
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,43 +37,28 @@ typedef enum { | |
S2N_HASH_ALGS_COUNT | ||
} s2n_hash_algorithm; | ||
|
||
/* The low_level_digest stores all OpenSSL structs that are alg-specific to be used with OpenSSL's low-level hash API's. */ | ||
union s2n_hash_low_level_digest { | ||
MD5_CTX md5; | ||
SHA_CTX sha1; | ||
SHA256_CTX sha224; | ||
SHA256_CTX sha256; | ||
SHA512_CTX sha384; | ||
SHA512_CTX sha512; | ||
struct { | ||
MD5_CTX md5; | ||
SHA_CTX sha1; | ||
} md5_sha1; | ||
}; | ||
|
||
/* The evp_digest stores all OpenSSL structs to be used with OpenSSL's EVP hash API's. */ | ||
struct s2n_hash_evp_digest { | ||
struct s2n_evp_digest evp; | ||
/* Always store a secondary evp_digest to allow resetting a hash_state to MD5_SHA1 from another alg. */ | ||
struct s2n_evp_digest evp_md5_secondary; | ||
}; | ||
|
||
/* s2n_hash_state stores the s2n_hash implementation being used (low-level or EVP), | ||
* the hash algorithm being used at the time, and either low_level or high_level (EVP) OpenSSL digest structs. | ||
/* s2n_hash_state stores the state and a reference to the implementation being used. | ||
* Currently only EVP hashing is supported, so the only state are EVP_MD contexts. | ||
*/ | ||
struct s2n_hash_state { | ||
const struct s2n_hash *hash_impl; | ||
s2n_hash_algorithm alg; | ||
uint8_t is_ready_for_input; | ||
uint64_t currently_in_hash; | ||
union { | ||
union s2n_hash_low_level_digest low_level; | ||
struct s2n_hash_evp_digest high_level; | ||
} digest; | ||
}; | ||
|
||
/* 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. | ||
*/ | ||
Comment on lines
-75
to
62
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
struct s2n_hash { | ||
int (*alloc)(struct s2n_hash_state *state); | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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.