[
https://issues.apache.org/jira/browse/HADOOP-14705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16131015#comment-16131015
]
Rushabh S Shah commented on HADOOP-14705:
-----------------------------------------
Reviewed patch#4 since patch#5 didn't apply cleanly when I started reviewing.
+KMS#reencryptEncryptedKeys()+
# Do we need to increment AdminCallsMeter ?
Its not clear from the class which calls are supposed to be admin and which
ones are non-admin.
Since we are planning to create a new thread in namenode for batch
re-encrypting, I think it would be admin call.
Please comment otherwise.
# We can remove the try catch block around {{user.doAs}} context and let the
outer try catch block handle the exception propagating from the {{doAs}} call.
+KeyAuthorizationKeyProvider#reencryptEncryptedKeys(List<EncryptedKeyVersion>
ekvs)+
# The following chunk of code is redundant.
{noformat}
if (keyName == null) {
keyName = ekv.getEncryptionKeyName();
} else {
if (!keyName.equals(ekv.getEncryptionKeyName())) {
throw new IllegalArgumentException(String.format(
"multiple keyname '%s' '%s' found for reencryptEncryptedKeys",
keyName, ekv.getEncryptionKeyName()));
}
{noformat}
We already do this check in {{KMS#reencryptEncryptedKeys}}
+KMSUtil.java+
* parseJSONEncKeyVersions
** Why are we using LinkedList ?
We are just traversing the list using listIerator() and caling Iterator#set().
set() operation is O(1) in both cases.
The memory footprint is more in LinkedList compared to ArrayList.
* toJSON(KeyProvider.KeyVersion keyVersion) and {{toJSON(EncryptedKeyVersion
encryptedKeyVersion)}} are still using LinkedHashMap.
We can make it HashMap.
+TestKeyProviderCryptoExtension#testReencryptEncryptedKeys()+
# Minor nit:
{noformat}
assertEquals("Version name of EEK should be EEK",
KeyProviderCryptoExtension.EEK,
ekv.getEncryptedKeyVersion().getVersionName());
{noformat}
The error message should be "Version name should be EEK"
# Another minor nit.
assertEquals("Name of EEK should be encryption key name",
Something doesn't seems right with this error message.
According to me, it should be EEK's key name should be fookey.
# Following code.
{noformat}
if (Arrays.equals(ekv.getEncryptedKeyVersion().getMaterial(),
encryptionKey.getMaterial())) {
fail("Encrypted key material should not equal decrypted key material");
}
{noformat}
Instead you can use
{noformat}
Assert.assertNotEquals("Encrypted key material should not equal decrypted key
material", new String(ekv.getEncryptedKeyVersion().getMaterial(),new
String(encryptionKey.getMaterial()))
{noformat}
# Following code.
{noformat}
encryptionKey = kp.createKey(ENCRYPTION_KEY_NAME, SecureRandom.getSeed(16),
options);
final KeyVersion kv = kpExt.decryptEncryptedKey(ekv);
// Following lines are in patch.
if (Arrays.equals(kv.getMaterial(), encryptionKey.getMaterial())) {
fail("Encrypted key material should not equal encryption key material");
}
{noformat}
This comparison didn't made sense to me.
This is comparing the secret that is stored in the backend with the material
that is being generated while calling decryptEncryptedKey()
These both are bound to be different.
I think we are comparing apples to oranges.
# I would like to see the following test case.
The material returned after decrypting original ekv and decrypting new ekv
should be same.
{noformat}
for (int i = 0; i < ekvs.size(); ++i) {
final EncryptedKeyVersion ekv = ekvs.get(i);
final EncryptedKeyVersion orig = ekvsOrig.get(i);
KeyVersion decryptedEkv = kpExt.decryptEncryptedKey(ekv);
KeyVersion origEkv = kpExt.decryptEncryptedKey(orig);
Assert.equals(decryptedEkv.getMaterial(), origEkv.getMaerial());
}
{noformat}
> Add batched reencryptEncryptedKey interface to KMS
> --------------------------------------------------
>
> Key: HADOOP-14705
> URL: https://issues.apache.org/jira/browse/HADOOP-14705
> Project: Hadoop Common
> Issue Type: Improvement
> Components: kms
> Reporter: Xiao Chen
> Assignee: Xiao Chen
> Attachments: HADOOP-14705.01.patch, HADOOP-14705.02.patch,
> HADOOP-14705.03.patch, HADOOP-14705.04.patch, HADOOP-14705.05.patch,
> HADOOP-14705.06.patch
>
>
> HADOOP-13827 already enabled the KMS to re-encrypt a {{EncryptedKeyVersion}}.
> As the performance results of HDFS-10899 turns out, communication overhead
> with the KMS occupies the majority of the time. So this jira proposes to add
> a batched interface to re-encrypt multiple EDEKs in 1 call.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
