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
