Design review: JEP 273: DRBG-Based SecureRandom Implementations

[Sean Mullan]

A few more comments -

The name of the new interface should be SecureRandomParameterSpec 
instead of SecureRandomSpec.

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.

You will also need to add a protected (or public?) constructor to 
SecureRandomSpi that takes a SecureRandomParameterSpec parameter. 
CertStoreSpi is a good example to follow.

DrbgSpec should really be named DrbgParameterSpec to be consistent with 
other AlgorithmParameterSpec subclasses.

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

This avoids code having to always do this in the (likely) common case 
where additionalInput is not specified:

   sr.reseed(null);

In SecureRandomSpi, I think there is a typo: engineGenerate should be 
named engineNextBytes, and it should be protected (not public).

I think the new methods in SecureRandomSpi need a default impl and it 
should be specified. They should probably throw UnsupportedOperationExc 
by default. If so, this should be added in @throws clause and default 
behavior documented in an @implSpec.

I still need to review the EntropyInput and DrbgSpec classes ...

Thanks,
Sean

On 11/13/2015 08:43 PM, Wang Weijun wrote:
>
>> On 2015年11月14日, at 上午1:56, Sean Mullan <sean.mullan at oracle.com> wrote:
>>
>> On 11/12/2015 07:58 PM, Wang Weijun wrote:
>>>> What happens if configure is called more than once, or
>>>> simultaneously by more than one thread?
>>> The state is reset. The last one rules. The implementation can be
>>> made synchronized.
>>>
>>> * This method can be called multiple times. After each call, this *
>>> {@code SecureRandom} object must be reseeded.
>>>
>>>>>
>>>>> Instead of a configure method, I would suggest adding new
>>>>> getInstance methods that take an AlgorithmParameterSpec. This
>>>>> should simplify the implementation.
>>> getInstance() has 3 flavors, (), (String) and (Provider). Too many
>>> new methods to add.
>>
>> Only 3 more. It would seem to make the implementation simpler, less state and synchronization to worry about.
>
> OK.
>
>>
>>>>> I also think it might be cleaner and simpler to make EntropyInput
>>>>> an input parameter of DrbgSpec so that you could have a single
>>>>> AlgorithmParameterSpec parameter (instead of an AlgParamSpec and
>>>>> EntropyInput) for the getInstance method.
>>> EntropyInput as a separated parameter means it applies to other
>>> SecureRandom implementations and not only DRBG. For example, SHA1PRNG
>>> can also have a specified entropy source. It is also useful to
>>> describe what reSeed() means.
>>
>> Ok but it's really just an input parameter, right?
>>
>> Maybe, you want something like:
>>
>> public interface SecureRandomSpec extends AlgorithmParameterSpec {
>>
>>     EntropyInput getEntropyInput();
>> }
>>
>> public class DrbgSpec implements SecureRandomSpec {
>>
>> }
>
> Sound good.
>
> I'll update the APIs.
>
> Thanks
> Max
>
>>
>> --Sean
>