Design and impl review: JEP 273: DRBG-Based SecureRandom Implementations

[Wang Weijun]

> On Jan 6, 2016, at 1:21 AM, Sean Mullan <sean.mullan at oracle.com> wrote:
> 
> Here are some more comments on the API. I will send some comments on the impl next.
> 
> * DrbgParameters
> 
>  38  * A DRBG mechanism should extend this class.
> 
> Is this sentence necessary? None of the builtin DRBG mechs extend this class.

I was wrong.

> 
> 175          * If this method is not called, the implementation will select
> 176          * a default {@code EntropyInput}.
> 
> I think you need to be more specific about what you mean by "the implementation" (and in other methods of this class). It is not the implementation of this class, it is the DRBG SecureRandom implementations.

OK.

> 
> 236          * Requests that a derivation function to be used by the DRBG mechanism.
> 
> s/to be used/is to be used/
> 
> 280          * @return teh newly built {@code DrbgParameters} object
> 
> s/teh/the/
> 
> The get methods should state whether they can return null or not. Some of the methods should be more specific about whether they return or make copies of mutable parameters.

OK.

> 
> * SecureRandomSpi
> 
> 67     protected SecureRandomSpi() {
> 
> This seems like an incompatible change to me. The previous version had a default public constructor. I think this needs to be public.

OK.

> 
> 75      *               This argument can be {@code null}.
> 
> Why can't we call the SecureRandomSpi ctor that takes no args when the parameter is null? It seems it would be cleaner to do that, but there is probably a good reason.

SecureRandom.getInstance("HashDRBG") will call new HashDrbg(null), which calls a series of super(null) until SecureRandomSpi. The @implSpec of the class has more details.

I can revisit the change in Provider.java. Maybe the following will work:

  if (param == null) {
     findCtorWithNoArg().newInstance();
  } else {
     findCtorWithArg(argClazz).newInstance(param);
  }

> 
> 109      * @param bytes output.
> 
> Too terse. How about "the array to be filled in with random bytes"

OK.

> 
> 110      * @param additionalInput additional input. {@code null} if none.
> 
> I think you should throw NPE if this is null because callers should call engineNextBytes(byte[]) in that case.

OK.

I'll need to implement engineNextBytes(1,2) and engineNextBytes(1) in all implementations and probably let them call something like nextBytesInternal().

> 
> 110, 134: both of these methods need an @throws UnsupportedOperationException

OK.

> 
> 134      * @param additionalInput additional string, {@code null} if none.
> 
> I think it would be better to add 2 engineReseed methods, one which takes no arguments and one with an additionalInput arg that throws NPE if it is null. This would also be consistent with the corresponding methods on SecureRandom.

OK.

> 
> * SecureRandom
> 
>  68  * {@link SecureRandomParameters} parameter. For example, a DRBG
>  69  * can be initialized with a {@link DrbgParameters} object.
> 
> s/a DRBG/a DRBG implementation/
> 
> 87  * <p> Except for the one created by {@link #SecureRandom(byte[])}, a newly
> 
> s/one/instance/
> 
>  88  * instantiated SecureRandom object is not seeded.  Call one of {@code reseed}
>  89  * or {@code setSeed} methods to seed it. If none is called, the first call
> 
> s/Call one of/Call the/
> s/methods/method/
> s/If none/If neither method/
> 
> 92  * at instantiate time through a {@code SecureRandomParameters} object.
> 
> s/instantiate/instantiation/ (or creation)
> 
> 94  * This self-seeding will not occur if one of {@code reseed} or {@code setSeed}
>  95  * methods was previously called.
> 
> s/one of/the/
> s/methods/method/
> 
> 97  * <p> A SecureRandom can be reseeded at any time by calling one of the
> 
> s/one of the/the/
> s/methods/method/
> 
> 883      * {@code additionalInput} may contain entropy but its main usage is to
> 884      * provide diversity.
> 
> s/entropy but/entropy, its/

OK.

Thanks
Max

> 
> --Sean
> 
> On 12/16/2015 02:20 AM, Wang Weijun wrote:
>> Webrev updated:
>> 
>>    http://cr.openjdk.java.net/~weijun/8051408/webrev.02/
>>    http://cr.openjdk.java.net/~weijun/8051408/webrev.02/specdiff/java/security/package-summary.html
>> 
>> Changes:
>> 
>> 1. DrbgParameters has a Builder now
>> 
>> 2. No more default implementation for reseed()
>> 
>> 3. Synchronization is now inside implementation, on engineXXX() methods
>> 
>> 4. engineConfigure() now throws InvalidAlgorithmParameterException instead of IllegalArgumentException
>> 
>> 5. CtrDRBG uses up all input in engineSetSeed()
>> 
>> Thanks
>> Max