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

[Wang Weijun]

> On Jan 5, 2016, at 6:59 AM, Sean Mullan <sean.mullan at oracle.com> wrote:
> 
> Here are some more comments on the API:
> 
> * EntropyInput:
> 
>  29  * An interface of a source of entropy input.
> 
> "interface" is implied, so you can just say "A source of entropy input." Also, I think this interface should be called "EntropySource". To me, "Input" means the byte array that is already populated, whereas "Source" produces those bytes. A sentence or two with more details in the description would also be helpful.

Well, as defined in Section 4 of SP 800-90Ar1, "Entropy Source" means real random source based on a physical device. What we need here is the "Randomness Source" or "Source of entropy input" (section 2 of 800-90B).

Shall we call it RandomnessSource?

> 
> Do we want to allow the generated entropy input to be longer than the size requested (see section 7.1 of sp800-90Ar1)? Perhaps, a method such as:
> 
>    byte[] getFullEntropy(int length)
> 
> might be more suitable, and specify that a byte array longer than the specified length may be returned.

This should be named getEntropy(), because full means every bit of the output contains one bit of entropy.

Such a method is OK for most cases, but except for one, CtrDRBG with no derivation function, where the entropy input must be full. Therefore a getFullEntropy() is needed anyway so I just use it everywhere. I also thought that every random source should have a conditioning module to convert non-full entropy bytes to full ones.

If this is not true, 2 methods are needed:

  byte[] getEntropy(int length);
  void getFullEntropy(byte[] entropy);

Detailed descriptions will be added on when and which is used.

And I don't think we need to provide any conditioning codes.

> 
> * EntropyNotAvailableException:
> 
>  37      * Creates one
> 
> Incomplete sentence.

Will fix it.

> 
> In the class description, add an @see link to EntropyInput.getFullEntropy.
> 
> * SecureRandomParameters:
> 
> It seems a little odd to have a class with one parameter that can be optional, and then accepting null as a value in the ctor, which typically will only needed by subclasses. I think it would be nicer to avoid exposing that in the API. Have you considered defining this as an interface, with a static method that returns an instance with an EntropyInput, ex:
> 
> public interface SecureRandomParameters {
> 
>    EntropyInput getEntropyInput();
> 
>    static SecureRandomParameters of(EntropyInput);
> }

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