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

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

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



##########
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 provided an explanation in the commit message / description above. See "Remove use of explicit SHA1PRNG implementation of SecureRandom". SHA1PRNG is, by far, a weaker implementation than whatever /dev/urandom (the default) would give, and specifying the SUN implementation explicitly bypasses user's ability to configure their own Java security providers. In future, the SUN implementation may not even be available. If a specific provider/implementation is desired, it should be configured in the Java security config files, not hard-coded here, bypassing the secure settings set by a system administrator.

##########
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:
       I thought about this, but decided against exposing it. There's no reason a subclass needs to rely on this specific instance of the Random. And, exposing the field to sub-classes really restricts our ability to evolve implementation (fields are harder to evolve than methods). If it becomes necessary/useful to expose this, it can be done in a getter method that can be overridden. But, it doesn't seem to be necessary at this time.




-- 
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