[GitHub] [accumulo] EdColeman commented on a change in pull request #2339: Update parent POM, plugins, LICENSE

[GitBox <gi...@apache.org>]

EdColeman commented on a change in pull request #2339:
URL: https://github.com/apache/accumulo/pull/2339#discussion_r741291297



##########
File path: core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java
##########
@@ -315,8 +313,8 @@ public FileDecrypter getDecrypter(Key fek) {
       private final byte[] initVector = new byte[GCM_IV_LENGTH_IN_BYTES];
 
       AESGCMFileEncrypter() {
-        this.fek = generateKey(sr, KEY_LENGTH_IN_BYTES);
-        sr.nextBytes(this.initVector);
+        this.fek = generateKey(random, KEY_LENGTH_IN_BYTES);
+        random.nextBytes(this.initVector);

Review comment:
       I do not have an issue with the change, but this instance (and that in CryptoUtils) seemed to go out of the way to allow a specific SecureRandom type to be used - could there be a specific reason?  I don't know, maybe some kind of external compatibility with something that would have been verified with particular algorithm - or somehow to prevent something that would weaken the effectiveness of the encryption in some use-cases?

##########
File path: core/src/main/java/org/apache/accumulo/core/spi/fs/RandomVolumeChooser.java
##########
@@ -19,14 +19,13 @@
 package org.apache.accumulo.core.spi.fs;
 
 import java.security.SecureRandom;
-import java.util.Random;
 import java.util.Set;
 
 /**
  * @since 2.1.0
  */
 public class RandomVolumeChooser implements VolumeChooser {
-  protected final Random random = new SecureRandom();

Review comment:
       Could this have been to allow extension if the RandomVolumeChooser was extended?

##########
File path: core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java
##########
@@ -315,8 +313,8 @@ public FileDecrypter getDecrypter(Key fek) {
       private final byte[] initVector = new byte[GCM_IV_LENGTH_IN_BYTES];
 
       AESGCMFileEncrypter() {
-        this.fek = generateKey(sr, KEY_LENGTH_IN_BYTES);
-        sr.nextBytes(this.initVector);
+        this.fek = generateKey(random, KEY_LENGTH_IN_BYTES);
+        random.nextBytes(this.initVector);

Review comment:
       Again - I agree with the change and I think what you are proposing is a move in a better direction.  What I wonder is why was this done originally - maybe it was unnecessary or was attempting something that it didn't quite materialize.  It would just help validate the change if the original intent can be determined.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org