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.
##########
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:
> It would just help validate the change if the original intent can be
determined.
SHA1PRNG was the default when the old experimental crypto properties (now
removed) made it configurable in Accumulo's site configuration file, rather
than relying on Java security settings. It was leftover from that, and then
updated slightly in #617, although it could have been removed at that point,
since it was no longer configurable in Accumulo's site file, as the
experimental properties to configure it had already been removed for 2.0 in
#560.
The original intent was to provide a default for a configurable
implementation. After my changes, one can still change the implementation...
but would have to do so using standard Java security configuration files,
rather than some custom experimental configuration we previously had and have
now removed. My changes restore the original intent to make it configurable,
since it was only possible to use the hard-coded default since the properties
were removed in #560.
--
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]
