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

[Sean Mullan]

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.

  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.

  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.

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

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.

109      * @param bytes output.

Too terse. How about "the array to be filled in with random bytes"

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.

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

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.

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

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