[
https://issues.apache.org/jira/browse/HDFS-15098?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17154413#comment-17154413
]
Vinayakumar B edited comment on HDFS-15098 at 7/9/20, 11:19 AM:
----------------------------------------------------------------
Thanks [~zZtai] for the contribution
Overall changes looks good. Following are my comments. Please check.
1. Adding this provider should be configurable. And update the document as
required.
As already mentioned by [~lindongdong] no need to add to JDK dirs. May be
Issue descreption can be updated.
so, following addition of Provider needs to be done only if its configured.
Because direct adding of {{BounctCatleProvider}} seems to change the existing
default behavior in some cases. Ex: {{TestKeyShell#createInvalidKeySize()}}
suppose to fail with keysize 56. But it passes when provider is BC. So it
should be used only on user's demand. So making it configurable would be wise
choise.
{code:java}
+ Security.addProvider(new BouncyCastleProvider());
{code}
In KeyProvider.java it can be added as below.
{code:java}
String jceProvider = conf.get(HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_KEY);
if (BouncyCastleProvider.PROVIDER_NAME.equals(jceProvider)) {
Security.addProvider(new BouncyCastleProvider());
}
{code}
In JceSm4CtrCryptoCodec.java should add on setConf() instead of constructor.
{code:java}
provider = conf.get(HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_KEY,
BouncyCastleProvider.PROVIDER_NAME);
final String secureRandomAlg = conf.get(
HADOOP_SECURITY_JAVA_SECURE_RANDOM_ALGORITHM_KEY,
HADOOP_SECURITY_JAVA_SECURE_RANDOM_ALGORITHM_DEFAULT);
if (BouncyCastleProvider.PROVIDER_NAME.equals(provider)) {
Security.addProvider(new BouncyCastleProvider());
}
{code}
2. With Above change, {{TestKeyShell#testInvalidKeySize()}} will not fail
anymore, as BC provider will not be added by default. So changes in
{{TestKeyShell}} can be reverted.
3. In {{TestCryptoCodec.java}}
Remove these lines from every test.
{code:java}
try {
KeyGenerator keyGenerator = KeyGenerator.getInstance("SM4");
} catch (Exception e) {
Assume.assumeTrue(false);
}
{code}
4. In {{TestCryptoCodec#testJceSm4CtrCryptoCodec}} change this config as below.
{code:java}
conf.set(HADOOP_SECURITY_CRYPTO_CODEC_CLASSES_SM4_CTR_NOPADDING_KEY,
JceSm4CtrCryptoCodec.class.getName());{code}
Uncomment following lines
{code:java}
//cryptoCodecTest(conf, seed, count,
//jceSm4CodecClass, opensslSm4CodecClass, iv);
{code}
{code:java}
//cryptoCodecTest(conf, seed, count,
//jceSm4CodecClass, opensslSm4CodecClass, iv);
{code}
5. Avoid import statements with * in all classes. import only required classes
directly.
6. {{HdfsKMSUtil.getCryptoCodec()}} is not logging {{JceSm4CTRCodec}}. May be
can log all classnames, when its not null without checking the instanceof ?
7. I can see lot of code is same between AES and SM4 codecs, except the
classnames and algorithm names. May be refactoring would help to reduce the
duplicate code.
8. I think in {{hdfs.proto}} SM4 enum value can be changed to 3 directly.
{code}enum CipherSuiteProto {
UNKNOWN = 1;
AES_CTR_NOPADDING = 2;
SM4_CTR_NOPADDING = 3;
}{code}
9. In {{OpenSecureRandom.c}} following functions' declarations and definitions
can be kept within {{OPENSSL_VERSION_NUMBER < 0x10100000L}} block. i.e.
following fuctions should be used only when {{OPENSSL_VERSION_NUMBER <
0x10100000L}} is true:
{code}
static void locks_setup(void)
static void locks_cleanup(void)
static void pthreads_locking_callback(int mode, int type, char *file, int
line)
static unsigned long pthreads_thread_id(void)
{code}
was (Author: vinayrpet):
Thanks [~zZtai] for the contribution
Overall changes looks good. Following are my comments. Please check.
1. Adding this provider should be configurable. And update the document as
required.
As already mentioned by [~lindongdong] no need to add to JDK dirs. May be
Issue descreption can be updated.
so, following addition of Provider needs to be done only if its configured.
Because direct adding of {{BounctCatleProvider}} seems to change the existing
default behavior in some cases. Ex: {{TestKeyShell#createInvalidKeySize()}}
suppose to fail with keysize 56. But it passes when provider is BC. So it
should be used only on user's demand. So making it configurable would be wise
choise.
{code:java}
+ Security.addProvider(new BouncyCastleProvider());
{code}
In KeyProvider.java it can be added as below.
{code:java}
String jceProvider = conf.get(HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_KEY);
if (BouncyCastleProvider.PROVIDER_NAME.equals(jceProvider)) {
Security.addProvider(new BouncyCastleProvider());
}
{code}
In JceSm4CtrCryptoCodec.java should add on setConf() instead of constructor.
{code:java}
provider = conf.get(HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_KEY,
BouncyCastleProvider.PROVIDER_NAME);
final String secureRandomAlg = conf.get(
HADOOP_SECURITY_JAVA_SECURE_RANDOM_ALGORITHM_KEY,
HADOOP_SECURITY_JAVA_SECURE_RANDOM_ALGORITHM_DEFAULT);
if (BouncyCastleProvider.PROVIDER_NAME.equals(provider)) {
Security.addProvider(new BouncyCastleProvider());
}
{code}
2. With Above change, {{TestKeyShell#testInvalidKeySize()}} will not fail
anymore, as BC provider will not be added by default. So changes in
{{TestKeyShell}} can be reverted.
3. In {{TestCryptoCodec.java}}
Remove these lines from every test.
{code:java}
try {
KeyGenerator keyGenerator = KeyGenerator.getInstance("SM4");
} catch (Exception e) {
Assume.assumeTrue(false);
}
{code}
4. In {{TestCryptoCodec#testJceSm4CtrCryptoCodec}} change this config as below.
{code:java}
conf.set(HADOOP_SECURITY_CRYPTO_CODEC_CLASSES_SM4_CTR_NOPADDING_KEY,
JceSm4CtrCryptoCodec.class.getName());{code}
Uncomment following lines
{code:java}
//cryptoCodecTest(conf, seed, count,
//jceSm4CodecClass, opensslSm4CodecClass, iv);
{code}
{code:java}
//cryptoCodecTest(conf, seed, count,
//jceSm4CodecClass, opensslSm4CodecClass, iv);
{code}
5. Avoid import statements with * in all classes. import only required classes
directly.
6. {{HdfsKMSUtil.getCryptoCodec()}} is not logging {{JceSm4CTRCodec}}. May be
can log all classnames, when its not null without checking the instanceof ?
7. I can see lot of code is same between AES and SM4 codecs, except the
classnames and algorithm names. May be refactoring would help to reduce the
duplicate code.
> Add SM4 encryption method for HDFS
> ----------------------------------
>
> Key: HDFS-15098
> URL: https://issues.apache.org/jira/browse/HDFS-15098
> Project: Hadoop HDFS
> Issue Type: New Feature
> Affects Versions: 3.4.0
> Reporter: liusheng
> Assignee: zZtai
> Priority: Major
> Labels: sm4
> Attachments: HDFS-15098.001.patch, HDFS-15098.002.patch,
> HDFS-15098.003.patch, HDFS-15098.004.patch, HDFS-15098.005.patch,
> HDFS-15098.006.patch, HDFS-15098.007.patch, HDFS-15098.008.patch
>
>
> SM4 (formerly SMS4)is a block cipher used in the Chinese National Standard
> for Wireless LAN WAPI (Wired Authentication and Privacy Infrastructure).
> SM4 was a cipher proposed to for the IEEE 802.11i standard, but has so far
> been rejected by ISO. One of the reasons for the rejection has been
> opposition to the WAPI fast-track proposal by the IEEE. please see:
> [https://en.wikipedia.org/wiki/SM4_(cipher)]
>
> *Use sm4 on hdfs as follows:*
> 1.download Bouncy Castle Crypto APIs from bouncycastle.org
> [https://bouncycastle.org/download/bcprov-ext-jdk15on-165.jar]
> 2.Configure JDK
> Place bcprov-ext-jdk15on-165.jar in $JAVA_HOME/jre/lib/ext directory,
> add "security.provider.10=org.bouncycastle.jce.provider.BouncyCastleProvider"
> to $JAVA_HOME/jre/lib/security/java.security file
> 3.Configure Hadoop KMS
> 4.test HDFS sm4
> hadoop key create key1 -cipher 'SM4/CTR/NoPadding'
> hdfs dfs -mkdir /benchmarks
> hdfs crypto -createZone -keyName key1 -path /benchmarks
> *requires:*
> 1.openssl version >=1.1.1
> 2.configure Bouncy Castle Crypto on JDK
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
