RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Bradford Wetmore]

On 4/27/2016 7:16 PM, Wang Weijun wrote:
>
>> On Apr 28, 2016, at 8:50 AM, Bradford Wetmore <bradford.wetmore at oracle.com> wrote:
>>
>> I won't have the cycles to do another full review, but just looked over things I mentioned in my previous review, and things are looking good! I have to jump back to my other projects.
>
> Thanks, there are still several thing to discuss. The one important is about nextBytes. Read below.
>
>> DrbgParameters.java
>> ===================
>>
>> Did you want to add an implNote that the "java.security" file or the Security Property configures the choice entropy/mechanism/etc?
>
> The @implNote already has
>
> * The mechanism name and DRBG algorithm name are determined by the
> * {@linkplain Security#getProperty(String) security property}
> * {@code securerandom.drbg.config}. The default choice is Hash_DRBG
> * with SHA-256.
>
> and
>
> * This implementation reads fresh entropy from the system default entropy
> * source determined by the security property {@code securerandom.source}.
>
> Do I need to mention "java.security"?

I don't know what I was thinking, maybe in another file?  Obviously it 
is there.  Please ignore this comment. :(

>>>>>> java/security/Provider.java
>>>>>> ===========================
>>>>
>>>>>> I'm really tempted to file a bug to clean up the really long lines added
>>>>>> by the lambda folks.  Ugh...This make reviewing changes in frame-based
>>>>>> webrevs hard.
>
>> I would say file another bug
>
> https://bugs.openjdk.java.net/browse/JDK-8155575 filed. I added you as a watcher.

Great, thanks.

>>>>>> java/security/SecureRandom.java
>>>>>> ===============================
>>>>
>>>>>>
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8155191 filed.
>>
>> Thanks.  Are you going to handle as a separate putback, or part of this one?  The timing is probably better (an additional CCC) if you do later, but it does add overhead because it would require a separate outback.
>
> A separate push.

Ok.

>>>>>> XXX: what do you do bout large size requests or things that can't be
>>>>>> reseeded?
>>>>>
>>
>> This wording is a little awkward, given the currently available APIs. Since we don't have a pos/offset nextBytes() variant, so maybe something like:
>>
>> * If the underlying implementation (for example: DRBG) is prohibited
>> * from supplying the requested amount of data in a single call, the
>> * application should retry by breaking the request into multiple
>> * smaller requests.
>
> I think there is a problem here.
>
> We should not ask an application to call it multiple times because it does not know what the max size is. Instead, the implementation should always fill in all bytes. I'd like to add some words in SecureRandomSpi#engineNextBytes:
>
>   Some random number generators can only generate a limit amount
>   of random bytes per invocation. If the size of {@code bytes}
>   is greater than this limit, the implementation should invoke
>   the process multiple times to generate enough random bytes
>   in a single {@code engineNextBytes} call.

We were on the same page after all.  I completely misunderstood your 
point in a previous email.  What you have is fine (change 
limit->limited), or a minor suggestion below (take it or leave it):

Some random number generators can only generate a limit amount
of random bytes per invocation. If the size of {@code bytes}
is greater than this limit, the implementation should invoke
the process multiple times to generate enough random bytes
in a single {@code engineNextBytes} call.
->
Some random number generators can only generate a limited amount
of random bytes per invocation. If the size of {@code bytes}
is greater than this limit, the implementation should invoke
its generation process multiple times to completely fill the
buffer before returning from this method.

>>>>> *** I am not sure what you are asking of reseeding. UnsupportedOperationException not enough?
>>>>
>>>> My comment was that if I specify PR here and the impl doesn't support reseeding, is that an IllegalArgument exception?
>>>
>>> It will be UnsupportedOperationException. I add a line saying the method must not be implemented.
>>
>> I wonder, would you ever have a situation where reseed() might be overridden, but still illegal in some case.  If so, maybe...  If not, never mind.
>>
>> @throws UnsupportedOperationException if the underlying provider
>>         implementation has not overridden this method.
>> ->
>> @throws UnsupportedOperationException if the underlying provider
>>         implementation has not overridden this method or does not
>>         support this operation.
>
> I added in SecureRandomSpi#engineReseed:
>
> * Do not override this method if the implementation does not
> * support reseeding.

Ok.

Brad