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

Reply via email to