RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

Thu Apr 28 00:50:41 UTC 2016

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 for your careful review of our comments.  A few minor points.

On 4/27/2016 10:20 AM, Wang Weijun wrote:
> Webrev updated:
>
> http://cr.openjdk.java.net/~weijun/8051408/webrev.12
> http://cr.openjdk.java.net/~weijun/8051408/webrev.12/spec
> http://cr.openjdk.java.net/~weijun/8051408/webrev.12/specdiff

 > 212-213: Still talks about 800-90 instead of the Sun provider
 > document.

Looks good.  Thanks.

 > 238 #   securerandom.drbg.config=
 > CTR_DRBG,AES-256,256,pr_and_reseed,use_df

You could change the second "256" to "192", just to show that the 
requested strength doesn't have to be the max the AES strength.

Thanks for taking care of the long lines!  Very much appreciated.

DrbgParameters.java
===================
You might want to add a "(800-90Ar1)" here, like you would after 
defining an acronym.

Did you want to add an implNote that the "java.security" file or the 
Security Property configures the choice entropy/mechanism/etc?

AbstractDrbg.java
=================
Thanks for the rename and the step# comments.  Much easier to follow!

> 1am now and I will reread the changes tomorrow. This is the result of 3 days' intense work and I hope I cover every comment from you.
>
> Big changes:
>
> 1. DrbgCavp moved to open as a manual test (because the test vector is huge), and created a closed one to launch it automatically.
>
> 2. healthTest() stub added.
>
> 3. debug() shows type/identity
>
> 4. Rename of variables to be more 800-90Ar1 style, step-by-step comments added
>
> 5. status() not called anymore, no internal state shown >

> 6. Internal state not serialized. Some words in DrbgParameters on this.
>
> 7. s/Instantiate/Instantiation/g.
>
> Still no NPE for SecureRandom#nextBytes(null). Another bug.
 >
> Some comments below:
>
>>>> I didn't see any checks for repeated input parameters.  IIRC, if you get
>>>> entropy repeating, that's a failure case.
>>>
>>> *** I thought that would be health check for an entropy source. How do you expect me to check it? A cache for recent inputs? Not very practical.
>>
>> Section 6.5 of 800-90B.  Or maybe we should just assume the entropy data to be valid and not worry about this?
>
> 800-90B? That's out of this JEP. We just assume the data is perfect.

Ok.

>>>> 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.
>
>
> Oh sorry, I'm having difficulties breaking them because the type name is already too long (For example, "BiFunction<? super Object, ? super Object, ? extends Object>") and I am not sure where to break it. How about I only break it partially? Say
>
> public synchronized Object merge(Object key, Object value,  BiFunction<? super Object, ? super Object, ? extends Object>  remappingFunction) {
> ->
> public synchronized Object merge(Object key, Object value,
>         BiFunction<? super Object, ? super Object, ? extends Object>  remappingFunction) {
>
> The 2nd line is still longer than 80 chars.
>
> Or I do not touch it at all and file another bug?

I would say file another bug, but I would break it:

public synchronized Object merge(Object key, Object value,
         BiFunction<? super Object, ? super Object, ? extends Object>
         remappingFunction) {

or

public synchronized Object merge(Object key, Object value,
         BiFunction<? super Object, ? super Object, ? extends Object>
             remappingFunction) {

>>>> java/security/SecureRandom.java
>>>> ===============================
>>
>>>> 660:  Reading the next method, I wonder if we should we also add a
>>>> @throws NPE here for then bytes is null?  It's undocumented currently.
>>>
>>> *** Wow. It was there and Xuelei suggested me removing it. The reason is that SunPKCS11's nextBytes happily accepted null (and ignore it).  Although I see no benefits doing that (SunPKCS11 has not use this chance to do any instantiate work etc), maybe it's safe to not require non-null here.
>>
>> Instead we should probably file a spec bug instead and get the SunPKCS11 fixed.
>
> 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 putback.

>>>> XXX: what do you do bout large size requests or things that can't be
>>>> reseeded?
>>>
>>> This won't happen in our implementation so I haven't mentioned them.
>>>
>>> For large size nextBytes() it's an implementation's duty to generate again and again until all is obtained.
>>
>> At one time, we had talked about modifying nextBytes(byte[]) internally keep regenerating until there was enough to return to avoid imposing this assumption on new code.  For example, if someone had a 90K Hash_DRBG/SHA* request (64K per call), the framework would call generate twice before turning the concatenated buffer array.
>>
>> But this assumption is fine as well.  It needs to be documented in both nextBytes().  Something along those lines.  "If the underlying implementation is prohibited from supplying a full arrays worth of data, the application must repeatedly call..."
>
> * If the underlying implementation is prohibited from supplying a
> * full arrays worth of data, the application must repeatedly call
> * its generation algorithm until all elements in {@code bytes} are
> * filled with random data.

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

>>>> 64:  Is this serialVersionUID a placeholder, or the final value?
>>>
>>> *** My understanding is that serialVersionUID calculated with serialno is to be compatible with an earlier version that has no this field. In a new file with serialVersionUID, it can anything and I prefer one that is easy to understand.
>>
>> I wasn't sure if you were going to take the final version of the file and run it through serialver to get the actual value.
>
> Is that a must?

Joe Darcy says:  "Is it a new class?  You can either just pick a number, 
or use serialver."  So take your pick.

>> I just found an RFE, and added my comment to it:
>>
>>    https://bugs.openjdk.java.net/browse/JDK-8143863
>
> Not sure if it's a good idea. Do we need HEX string for int[] and long[]?
>
> Also, for byte[], we have multiple formats. HexDumpEncoder is already good enough for human eyes, here we need something for a problem.

Yeah, hard to say.  I just hate seeing the same code in multiple places.

>>>> sun/security/provider/CtrDrbg.java
>>>> ==================================
>>>>
>>>> 51:  transient here?
>>>
>>> *** Already transient.
>>
>> Should it be transient?  It's not in the other files.
>
> MessageDigest in HashDrbg and Mac in HmacDrbg are all transient, because we don't want to serialize it. Is there anything wrong?
>
> That said, many more becomes transient since we don't want to serialize the internal state.

I missed it.

Thanks,
Brad