RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Wang Weijun]

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

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.

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

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

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

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

> 
>>> 752:  I don't think this addition made it into your CCC.
>> 
>> *** The 3rd part of spec in CCC only mentions changes related to DRBG. I think this one is trivial and should not be included there.
> 
> Likely needs to be a separate CCC then.

I included this as another part of the spec.

>> 
>> I'll use "3KeyTDEA (known as DEDede in JCE)".
> 
> Assuming a typo?  DEDede->DESede

Maybe auto-corrected by mail client?

> 
> You're still leaving the DESede alias, right?

It is still there.

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

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

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

Thanks
Max