HADOOP-18583. Native code to load OpenSSL 3.x symbols #5256
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
| @@ -35,8 +35,14 @@ static void (*dlsym_EVP_CIPHER_CTX_init)(EVP_CIPHER_CTX *); | |||
| #endif | |||
| static int (*dlsym_EVP_CIPHER_CTX_set_padding)(EVP_CIPHER_CTX *, int); | |||
| static int (*dlsym_EVP_CIPHER_CTX_test_flags)(const EVP_CIPHER_CTX *, int); | |||
| #if OPENSSL_VERSION_NUMBER >= 0x30000000L | |||
There was a problem hiding this comment.
- how about adding a comment to say "the names were changed so the probes have to change their type.
- do the actual typedefs need to be made exclusive? can see that if the actual typedef name was being recycled, but here they are being given new names
There was a problem hiding this comment.
- addressed in f5c9b44
- wasn't sure which name pops up in
hadoop checknativein case hadoop-common was compiled against OpenSSL 3.x and run with OpenSSL 1.x. Given that the symbols have same signature, the patch could be restricted to onlyJava_org_apache_hadoop_crypto_OpensslCipher_initIDs. Let me verify that...
There was a problem hiding this comment.
addressed 2. in 6c17fcf
| @@ -207,10 +233,20 @@ JNIEXPORT void JNICALL Java_org_apache_hadoop_crypto_OpensslCipher_initIDs | |||
| LOAD_DYNAMIC_SYMBOL(__dlsym_EVP_CIPHER_CTX_test_flags, \ | |||
| dlsym_EVP_CIPHER_CTX_test_flags, env, \ | |||
| openssl, "EVP_CIPHER_CTX_test_flags"); | |||
| #if OPENSSL_VERSION_NUMBER >= 0x30000000L | |||
There was a problem hiding this comment.
these are all static...the probes need to be compiled for openssl 2 vs 3. do we need this, or can it just look for either sets of symbols and be happy?
There was a problem hiding this comment.
Not quite sure - let me check.
There was a problem hiding this comment.
I need some more information here:
- With "look for either sets of symbols" do you mean moving the conditional symbol loading from compile time to runtime?
- What contract do we have for native code regarding OpenSSL ABIs?
The PR delivers the following contract:
- If compiled against OpenSSL X.Y API, it will only work at runtime if OpenSSL X.Y ABI is available.
To my knowledge, this is not a regression, looking at the other conditional symbol loads and their usage.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
OpenSSL 1.1.1 is not ABI compatible (and only partially API compatible) with OpenSSL 3.x. All the 3.x versions should be upwards ABI compatible - i.e. something compiled against 3.x will work with 3.(x+1). |
|
After digging a bit a more into Hadoop's usage of OpenSSL, I believe the PR fixes
Proper support of OpenSSL 3.x should at least review all of these usages. Supported platforms have at most OpenSSL 1.1.1, pull request verification builds run against OpenSSL 1.0.2. Perhaps file followup tickets? |
|
thanks for this. wildfly is a source of ongoing pain and pretty brittle across openssl versions. probably a full update is needed there. givent eh other places where things fail, is checknative correct in reporting incompatibility? as at least it is a fail-fast test for all of this. which means: fixing it should be the last action.
|
|
@packet23 @steveloughran , can you provide more details on the individual issues related to openssl 3 support? It would be nice if we create jira issues for tracking purpose. |
regarding crypton and checknative, we may to work on that. IMO checknative should look for openssl, at least if we add a -openssl argument. I don't think we have specific openssl issues. In #6425 I had to add error string matching for openssl 1 messages indicating stale https connections (these were surfacing deep in the AWS error stack). For 3.x it'd be good to know that these strings were the same -or update them. Maybe you can create an uber-jira "support openssl 3" one more thing: what is the openssl FIPS story? as for strict fips support we don't just want to talk to fips endpoints, we want to run on hosts where the ssl lib doesn't have the untrusted algorithms at all. |
|
Still face this problem when deploying hadoop on ubuntu 24.04, where the default version for openssl is 3.0.13. I tried to compile native libraries by myself with a ubuntu 24.04 based docker image, but still get the same error message... |
|
I've hit this too...forgot about the jira. If we do this switch, what happens on older openssl deployments --and do we care? |
steveloughran
requested changes
Oct 29, 2024
| @@ -169,9 +169,19 @@ JNIEXPORT void JNICALL Java_org_apache_hadoop_crypto_OpensslCipher_initIDs | |||
| "EVP_CIPHER_CTX_set_padding"); | |||
| LOAD_DYNAMIC_SYMBOL(dlsym_EVP_CIPHER_CTX_test_flags, env, openssl, \ | |||
| "EVP_CIPHER_CTX_test_flags"); | |||
| // name changed in OpenSSL 3 ABI - see History section in EVP_EncryptInit(3) | |||
| #if OPENSSL_VERSION_NUMBER >= 0x30000000L | |||
| LOAD_DYNAMIC_SYMBOL(dlsym_EVP_CIPHER_CTX_block_size, env, openssl, \ | |||
There was a problem hiding this comment.
let's have a single block for defining the string and then use
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
#define CTX_BLOCK_SIZE "EVP_CIPHER_CTX_get_block_size"
#define CTX_ENCRYPTING "EVP_CIPHER_CTX_is_encrypting"
...
#else
#define CTX_BLOCK_SIZE "EVP_CIPHER_CTX_block_size"
#define CTX_ENCRYPTING "EVP_CIPHER_CTX_encrypting"
...
#endif
LOAD_DYNAMIC_SYMBOL(dlsym_EVP_CIPHER_CTX_block_size, env, openssl,
CTX_BLOCK_SIZE);
Then the actual uses just refer to the macro definition as needed
- makes things less complex
- lines up for openssl 4 and any more changes.
There was a problem hiding this comment.
Will have a look this weekend.
There was a problem hiding this comment.
The requested change would not work on OpenSSL 1.0: EVP_CIPHER_CTX_encrypting is only available on OpenSSL 1.1. Would we need to support OpenSSL 1.0 still?
There was a problem hiding this comment.
Wait- I think I misunderstood you. We can decouple of course. Pushed a change addressing this and rebased as well.
packet23
force-pushed
the
HADOOP-18583
branch
from
6c17fcf
to
a30640c
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
packet23
force-pushed
the
HADOOP-18583
branch
from
5f70bdf
to
652e8f3
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
packet23
force-pushed
the
HADOOP-18583
branch
from
652e8f3
to
4e0ac98
Compare
|
💔 -1 overall
This message was automatically generated. |
steveloughran
approved these changes
Nov 5, 2024
| @@ -24,6 +24,57 @@ | |||
|
|
|||
| #include "org_apache_hadoop_crypto_OpensslCipher.h" | |||
|
|
|||
| /* | |||
steveloughran
changed the title
|
Merged to Hadoop trunk @packet23 can you cherrypick for branch-3.4 and submit as a new PR, so we can have yetus do a test run of it? If it is happy I will merge (there'll be no code reviews here, just a CI test) |
4 tasks
steveloughran
pushed a commit
that referenced
this pull request
Nov 7, 2024
Contributed by Sebastian Klemke
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of PR
Fix for HADOOP-18583. OpenSSL 3.x broke existing ABI - the symbols for
EVP_CIPHER_CTX_block_sizeandEVP_CIPHER_CTX_encryptingare now called
EVP_CIPHER_CTX_get_block_sizeandEVP_CIPHER_CTX_is_encryptingrespectively (c.f. commit ed576acdf591d4164905ab98e89ca5a3b99d90ab that landed in openssl-3.0.0-beta1)The PR changes the hadoop-common native code such that when compiled against OpenSSL 3.x, it can successfully load the OpenSSL 3.x symbols at runtime.
How was this patch tested?
A patched version of Hadoop 3.3.4 was compiled on an x86-64 Ubuntu 22.04 machine and
hadoop checknativewas invoked.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?