-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-17609. Make SM4 support optional for OpenSSL native code. #3019
Conversation
I manually tested this by
|
I need additional fix to make falling back to JceSm4CtrCryptoCodec work. Tests should be fixed too. |
On CentOS 8, only
On Ubuntu 18.04, all tests of TestCryptoCodec were executed since SM4 is supported by OpenSSL.
|
On CentOS 8, OpensslAesCtrCryptoCodec is available while OpensslSm4CtrCryptoCodec is not.
|
On Ubuntu 18.04, OpensslSm4CtrCryptoCodec was available and worked.
|
💔 -1 overall
This message was automatically generated. |
…enSSL does not support SM4.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
The cc warnings are not related to the patch. I'm working on false positive cc warnings on YETUS-1107. |
any progress on YETUS-1107 @iwasakims ? |
@busbey I filed apache/yetus#227. Thanks for reminding me of this.. |
...n-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/OpensslSm4CtrCryptoCodec.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
The test failures are not related. HADOOP-18046 was filed for TestIPC. cc warnings will be addressed after bumping Yetus to 0.14.0 (containing apache/yetus#227). @jojochuang Are you ok to merge this? |
I was able to test this locally and it worked as expected. |
I'd suggest making it easy to control with a Maven system property like |
@snmvaughan Thanks for testing this.
Please file another JIRA issue for you proposal. I'm not intending to disable SM4 even if the platform support it. |
@jojochuang @iwasakims |
@zhengchenyu I'm still willing to fix this and waiting for +1 from another committer. |
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.
did a quick review; hadn't noticed it still needed attention
+1 pending the changes
...op-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/crypto/OpensslCipher.c
Outdated
Show resolved
Hide resolved
return JNI_FALSE; | ||
} | ||
|
||
if (alg == AES_CTR && (dlsym_EVP_aes_256_ctr != NULL && dlsym_EVP_aes_128_ctr != NULL)) { |
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.
so this requires both aes 128 and aes 256?
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.
Yes. Both is loaded in loadAesCtr
.
...p-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestOpensslCipher.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
Related unit tests passed on my local environment. Manual testing looks fine as before on current trunk with the patch. |
for my reminder: In order to test SM4 codec, we need to put the jar of Bouncy Castle Provider on $JAVA_HOME/jre/lib/ext and add a line to $JAVA_HOME/jre/lib/security/java.security as described in the comment of HADOOP-15098.
|
The error does not appear to involve unit test.
|
Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: Wei-Chiu Chuang <[email protected]> (cherry picked from commit 2a50911)
I merged this to trunk and branch-3.4. Thanks, @steveloughran. |
…che#3019) Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: Wei-Chiu Chuang <[email protected]> (cherry picked from commit 2a50911)
…che#3019) Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: Wei-Chiu Chuang <[email protected]>
…che#3019) Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: Wei-Chiu Chuang <[email protected]>
https://issues.apache.org/jira/browse/HADOOP-17609
This replaces #2847.
After HDFS-15098, OpensslCipher does not work with OpenSSL >= 1.1.1 without SM4 support. RHEL/CentOS 8 provides such openssl package. The OpensslCipher on such environment should be usable if users do not need SM4 feature.