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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
