-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Use EVP_MAC for HMAC if it is available #118856
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
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
|
Wouldn't using |
That's basically what "portable" builds are. However some linux distros do not want us to do that. They want us to actually link against the distro's OpenSSL package. So we are in a situation where we support "both". Non-portable builds use the headers from the system it is compiled on, and portable builds use dlopen / dlsym. runtime/src/native/libs/System.Security.Cryptography.Native/opensslshim.c Lines 198 to 203 in e3127ed
|
bartonjs
left a comment
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.
Is this just an incremental change on the road to being fully consistent with 3.0 API, or is there a secondary gain (e.g. perf)?
It's largely a step toward moving toward OpenSSL 3 APIs, and #118807 highlighted a case where it's important to them for FIPS reasons. |
| // Forward declarations - shim API must not depend on knowing layout of these types. | ||
| typedef struct hmac_ctx_st HMAC_CTX; | ||
|
|
||
| struct _DN_MAC_CTX { |
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.
Why not an union? Isn't only one needed at the same time?
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.
Because the union doesn't encode what kind of union it is. We would need to carry a flag of some kind.
I toyed around with the idea of something like:
union DN_HMAC_CTX_UNION {
HMAC_CTX* legacy;
EVP_MAC_CTX* mac;
};
struct _DN_HMAC_CTX {
union DN_HMAC_CTX_UNION ctx; // Or inline it in the struct, whatever.
int legacy;
};But that's doing the same thing with a little more indirection. When I peeked at the compiled results it ended up being a struct with the same size (16) because of alignment and padding.
I don't love the idea of using g_evpMacHmacInit as the discriminator instead, but I suppose its possible.
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.
Is legacy ever used when the new APIs are available? I assumed it was never the case then.
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 don't love the idea of using g_evpMacHmacInit as the discriminator instead, but I suppose its possible.
I like that the union with the global as a discriminator gets rid of the extra malloc/free (not "allocation is bad", but "less paperwork is good"); but I'm content with what you have here, and we can always squeeze it down 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.
Pull Request Overview
This PR modernizes the native HMAC implementation to use OpenSSL 3.0's EVP_MAC API when available, while maintaining backward compatibility with the deprecated HMAC APIs for portable builds and older OpenSSL versions.
Key changes:
- Introduces a new DN_MAC_CTX structure that can hold either legacy HMAC_CTX or modern EVP_MAC_CTX
- Updates all HMAC functions to use EVP_MAC APIs when available, falling back to legacy APIs otherwise
- Adds runtime detection for EVP_MAC availability using pthread_once initialization
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| pal_hmac.h | Updates function signatures to use DN_MAC_CTX instead of HMAC_CTX and removes implementation-specific comments |
| pal_hmac.c | Implements dual-path logic using EVP_MAC APIs when available, with fallback to legacy HMAC APIs |
| osslcompat_30.h | Adds OSSL_MAC_PARAM_DIGEST constant and EVP_MD_get0_name function declaration |
| opensslshim.h | Adds EVP_MD_get0_name to the function pointer table for dynamic loading |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| } | ||
| else | ||
| { | ||
| printf("OpenSSL version could not be determiend.\n"); |
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.
| printf("OpenSSL version could not be determiend.\n"); | |
| printf("OpenSSL version could not be determined.\n"); |
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.
It's just for debugging CI. That whole commit is going to get reverted.
|
This is... complex for OpenSSL OpenSSL 3.0.0-3.0.2. Will close for now so CI doesn't keep spinning and re-open when ready. |
Yes, EVP_HMAC is also optionally hardware accelerated where there is HMAC specific accelerators available, unlike the "manual" digest operations that the old APIs use. |
This changes our native HMAC implementation to use EVP_MAC instead of the deprecated HMAC APIs, if it is available.
This doesn't quite get us far as being able to build with
OPENSSL_NO_DEPRECATEDor raisingOPENSSL_API_COMPATto 3.0 - the legacy APIs are still needed for portable builds.We could do something like this:
But that would only help the non-portable scenario where someone builds against OpenSSL 3. We probably need to continue to have the functions for portable builds for now.
Ideally we would just drop OpenSSL 1.1 before we need to come up with a way to handle deprecated OpenSSL 1.1 APIs.
Closes #118807
Contributes to #46526