Design review: JEP 273: DRBG-Based SecureRandom Implementations

[Anthony Scarpino]

On 11/18/2015 05:32 AM, Sean Mullan wrote:
> The getInstance methods can now take a SecureRandomParameterSpec object
> (rather than an AlgorithmParameterSpec). They should throw
> InvalidAlgorithmParameterException (not IllegalArgumentException) if the
> parameters are null or not the right type to be consistent with other
> Spi classes.

Sorry for backtracking a bit, but when I look at the original proposal 
by Max,

    SecureRandom sr = SecureRandom.getInstance("HashDRBG");
    hd.configure(new DrbgSpec("SHA-512", -1, true, false,
          nonce, person), new MyEntropyInput());

I seem something like Cipher, where getInstance is getting the 
transformation and configure is like Cipher.init().  The contents of 
DrbgSpec do look like AlgorithmParameters, but in the context during the 
init.  I'm not sure moving all of this into getInstance is consistent 
with something like Cipher.

I'm not sure such things like the algorithm, strength, nonce, etc are 
right to be set via getInstance(). If it's possible to have limitations 
in support of a particular DRBG, where it only supported SHA256 but not 
SHA512 (for example, I don't know the spec), then maybe it does make 
sense to be a getInstance.

In the original design with configure(), was it possible to reconfigure? 
  Or was one to just throw away the instance and get a new one or reset()?

 > nextBytes should throw NPE if bytes is null. I also think it should
 > not allow additionalInput to be null (should throw NPE), because this
 > is an overloaded method. If the caller doesn't want to specify
 > additionalInput, then it should call nextBytes(byte[]).

I don't think there should be a nextBytes(byte[] bytes, byte[] 
additionalInput).  If the user wants to have additionalInput they should 
use reseed(byte[]) only like you specified below.  I think having 
nextBytes() also doing reseeding is confusing.

 > I would prefer 2 reseed methods, one that didn't take any arguments,
 > and one that takes an additionalInput (and throws NPE if it is null):
 >
 >  public void reseed()
 >  public void reseed(byte[] additionalInput)

I agreed.

Tony