Deleting the cruft that doesn't need further discussion.
On 4/24/2016 9:13 PM, Wang Weijun wrote:
http://cr.openjdk.java.net/~weijun/8051408/webrev.11/
http://cr.openjdk.java.net/~weijun/8051408/webrev.11/spec
http://cr.openjdk.java.net/~weijun/8051408/webrev.11/specdiff
Thanks for the review. I copied your DRBG.txt below with my comments.
Most are accepted, some I mentioned my previous thoughts.
Sorry, I've not been as up to date on previous discussions.
Some needs clarification or discussion (does not mean I don't accept them),
tagged with *** so you can search for them.
BTW, the CAVP test is on test/closed. I'll send you a private mail.
We may need to include part/all of it in the open, depending on how
strict we want to be following the health testing requirement.
Overall:
========
IMPORTANT: By 800-90Ar1, sections 8.3/8.5, I don't think outputting the
internal state is allowed, even for debugging. Please recheck this and
let's discuss.
*** Not very sure. Someone can always use reflection to look at the internal
states.
Well the main reason I printed out the internal state is to compare it with the
CAVP text file when there is something wrong. Surely I can remove them now.
Hm...we should talk to the person I mentioned in my other email, he
might have an opinion on this.
Also by the 800-90Ar1 spec, and given that SecureRandom is supposedly
serializable, I don't think
serialization to recover the exact state is (or should be) allowed:
The internal state for one DRBG instantiation shall not be used as
the internal state for a different instantiation.
I'll/we'll need to think about this some more.
*** Since after s11n/des11n it's *another* instance, seems we should not
remember the states. A consequence is that it has to be reinitialized.
I think we're on the same page here.
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?
I didn't see any health tests. What is your plan for that?
*** If by health test your means the CAVP known-output tests, I am going to put
it into test/closed since it's reading a huge (13MB) external file and should
be stored on an artifact server.
That said, I'll be happy to extract a subset of it and make it a public test.
TBD in another mail.
123-128: Ordering still the same.
NativePRNG, SHA1PRNG, and DRBG.
I didn't realize your were asking me to be consistent. I only updated the first
appearance. Will fix.
Yes. Thanks.
Applications can request for different instantiation parameters
->
Applications can request different instantiation parameters
I thought "request for" is a correct phrase. Will fix.
It's a little awkward.
212-213: Still talks about 800-90 instead of the Sun provider document.
*** How just list all names here? I already listed all mech names.
Ok. It should probably also go into the Sun provider doc, where we talk
about what algorithms/modes/etc are available.
I notice the particular value of "3KeyTDEA" is mentioned later in this
file, but not the other string values.
CtrDrbg.java.
Not sure what you were saying here.
java/security/Provider.java
===========================
Copyright date.
Already updated. You reading webrev.10?
I might have started on this! Sorry!
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.
There aren't many in this file. I'll break them.
Thanks!
java/security/SecureRandom.java
===============================
54,78,406,439,513,etc: > 80 chars. I won't list any more, but expect it
might occur in other files as well.
You expect me to be 100% strict with 80 chars. Will do.
<soapbox>
Yes please! Sorry, it's a pet peeve of mine and some others. I do hear
the usual "Get a more modern terminal emulator", but that's not the
issue. The problem is real estate. Even with a 24" monitor
(1900x1200), doing a side-by-side webrev (i.e. frames) comparison on a
160 character line means I've got to scroll both sides. It's even worse
on a laptop with less real estate. A 3 column merge is nigh impossible.
And what if you need to print a hard copy?
I realize some breaks are impossible, but if you're regularly indenting
a large amount, you probably will want to refactor your code.
</soapbox>
72: I don't know for sure about this, but I don't think you need a <p>
following a <blockquote>. It gives extra vertical space in the output.
I noticed this several places in the resulting javadoc.
I don't see any visual difference between the 2 styles.
Turns out the extra space is a empty line in pre (try Select All and you'll see).
I'll put </pre> at the end of the previous line.
Ah yes! Thanks.
592: I've not be happy with the wording both here and in SecureRandomSpi.
The value returned here may not be the same as the one used to instantiate
it.
I'm wondering if maybe something like this would be clearer:
Returns the effective {@link SecureRandomParameters}
that is actually used to instantiate this {@code SecureRandom}.
->
Returns the effective SRP for this SecureRandom instance.
OK.
In fact, I was deliberately trying to express the meaning that the returned
value here could be different from the one passed into getInstance(). But maybe
as your suggested, not mentioning instantiate at all is clearer.
Anyway I think I've emphasized this many times in other places.
You have, but this seems to be contradictory to that. My point is that
some people might see "used to instantiate" as the parameter that was
actually passed in, rather than the derived value.
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.
673-677: You probably know this, but according to:
http://www.oracle.com/technetwork/articles/java/index-137868.html
a @param ends with a period if another sentence follows it. But
it's done throughout this file, so suggest you not change all the other
places in this push, but you may want to keep it consistent within
your new method.
*** Please confirm if I understand correctly: No period if it's only a clause
or an expression.
So I should remove the period at the end of 674.
See the section starting:
When writing a phrase, do not capitalize and do not end with a
period:
There's several rules there.
676: You mention unrecognizable/unsupported, does that include
illegal? For example, if prediction_resistance_flag was not set at
instantiate, but Prediction_reistance_request was set in this method?
I'll add "illegal", although "unsupported" kinds implies it. Otherwise, what is the difference of
"unrecognizable" and "unsupported"? We know this parameter defined for DRBG but we haven't yet
supported it?
unsupported/unrecognizable are likely the same: I would expect that to
mean they sent us an object that our implementation just doesn't know
what to do with. e.g. they extended and sent us a new subclass of
SecureRandomParameters that isn't a NextBytes. whereas illegal is that
they sent us a value that can't be allowed at runtime. e.g. pr was set
in this object, but pr wasn't requsted in the initiate.
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..."
*** 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?
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.
171: I think we should leave this part out, or at least warn that it
may not be supported in all distributions.
A provider is free to add other DRBG implementations with specific
configurations using different SecureRandom algorithm names (for
example, "Hash_DRBG/SHA-512")
Leave this out. I couldn't resist making suggestions on these old names that
are removed now.
Ok thanks. Since we didn't have, encouraging others to add them seem
strange.
I really think all of the Implementations Notes here should be placed
in the Sun Provider document. If you want to leave the section saying
"see the DRBG section of the Sun Providers", that's fine. Some
additional notes about this section whereever it ends up.
Correct. The @implSpec was written to comply with 800-90Ar1's 11.1 and will be
places in the Sun Provider doc. I was planning to add a reference here after
the target is complete.
Ok.
In java.security, you call it "3KeyTDEA", but here the string is
"DESede". Which is it?
I'll use "3KeyTDEA (known as DEDede in JCE)".
Assuming a typo? DEDede->DESede
You're still leaving the DESede alias, right?
472:
@throws NullPointerException if {@code capacity} is {@code null}.
->
@throws NullPointerException if {@code capability} is {@code null}.
Oops.
No problem, I've done that once or twice (or...). Funny how you can
look at something a million times, but you just believe what you want it
to say instead of what it actually says. ;)
java/security/SecureRandomParameters.java
=========================================
31:
A specific type of {@code SecureRandom} can implement this interface.
Don't you mean that different types of *SecureRandomParameter* can be
created and passed to SR's?
Yes, different for DRBG and NRBG, for example.
My point is that you'll be updating SecureRandom with
SecureRandomParameter here.
sun/security/provider/AbstractDrbg.java
=======================================
98,368,369, etc. Lines > 80. This is throughout your code. Hard to
read on a laptop with narrow display.
I'll shorted the comments, but sometimes for codes I am OK with a little more
than 80 chars. I can make it strict.
There are some places that it's difficult to make clean break, but
fortunately they are pretty few.
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.
107: In 800-90Ar1, many of the variables have specific names. They
don't generally follow our conventions, but would you consider using a
close derivative? For example, highestSupportedSecurityStrength and
maxPersonalizationStringLength? That would make a little more clear
how the DRBG variables map to the implementation. I notice you've used
those names in other places, but wasn't sure if that was by design or
not.
I use the name in 800-90Ar1 in comments but prefer a more-Java-like variable
name.
> If you like I can be more close.
It makes it a little easier to read.
145: Should you mention that MAX_INT is actually smaller than the
amount allowed by the various algorithms? Or should the implementation
be long-based throughout?
*** Since a Java array can only holds MAX_INT of elements, we won't be able to
support long.
Yes, of course! :)
> I can add an explanation.
Sound good.
Also, there are several places here and in other files (DRBG.java)
where you do multiple function calls where the return value won't be
changing: first to check legality, then to actually capture/use the value.
See lines 348/365/366. Can't these calls be combined?
Yes I can.
As you see, when the return value is a cloned array (line 359) I only call
once. When it's a primitive I think an extra call is not too bad.
Just seems like a simple enhancement.
355: Are you somewhere calling this DRBG.nextBytes() multiple times if
the output size is too big to be handled by a single DRBG call?
No. It should but since here maxNbLength is MAX_INT I omit the call.
Ok.
*** I created these maxNbLength/maxAiLength/maxPsLength variables to be more
aligned with pseudo codes in 800-90Ar1, but honestly they have no real use in
the impl and I sometimes ignore it.
Ah.
460: would you want to call security "min_entropy" to match the spec?
Sure, and I will change min/max there to minLength/maxLength.
Thanks.
483: Can't this be final? Netbeans is complaining about a non-final
defaultES at 503. Also private?
Yes and yes.
551: What is register() used for?
Useless. I was doing an experiment with lines in SunEntries
new HashDRBG(null).register(map)
and those attributes can be use by an app to detect capabilities.
We usually put those into the SunEntries directly so it can be used by
the JCE selection mechanism.
642: We have this problem in JSSE: in various places, you have debug
strings like "instantiate", but in a multi-SR environment, you'll have
no idea which one is in play. Do you want to use '"instantiate" + this'?
*** Is "this" enough? this.toString() shows the configuration. Do you want
System.identityHashCode()? I can add it right into toString().
Something that makes things quickly understandable which object is being
used would be helpful. SeanC would be a good one to ask, as he's more
involved in the supportability side. I'm just remembering trying to
debug some JSSE debug output, and we had multiple thread/sockets debug
output all interleaved together. It was impossible to parse.
674: can be final. Do you want to use the ES to generate an initial
value for the monotonically increasing sequence number?
*** I don't think it's necessary, as long as it changes with duplication.
Ok.
692: Isn't there some preexisting code somewhere we can use? This seems
like a common need, and I recall having written something similar a
while ago.
No colon/space/newline? I don't know.
I just found an RFE, and added my comment to it:
https://bugs.openjdk.java.net/browse/JDK-8143863
sun/security/provider/CtrDrbg.java
==================================
51: transient here?
*** Already transient.
Should it be transient? It's not in the other files.
141: AES-192/256 is indeed supported, it's just that the unlimited
policy files aren't installed. I would suggest a better error message
here, like "algorithm not available (policy)" or something similar.
OK.
"algorithm not available (because policy)". I recently read a article about modern
English do not use "because of" anymore.
Hm...ok.
422: What is this for?
SecretKeySpec for DESede use 24 bytes but k in CTR_DRBG is 21 bytes.
Ah, parity bits.
sun/security/provider/DRBG.java
===============================
60: A final constant string should be cap'd. propName -> PROPANE
OK.
er...PROP_NAME or PROPNAME, not PROPANE! ;)
93: space missing "part:"
Do we need a space here?
for (String s : names)
*** BTW, where is a modern coding style guide?
I don't think there is anything recent. I took this style from:
8/technotes/guides/language/foreach.html
which follows from the ? operator.
sun/security/provider/HmacDrbg.java
===================================
By 8.3/8.5, I don't think outputting V/C is allowed, even in debug.
Please recheck this.
76: Don't you need to re-init() this again? It's supposed to be the
current value of K.
*** In Step 2, v is updated but not k.
But k was updated in step 1 of HMAC_DRBG, or are K/V only updated at the
exit of the method? I'm guessing if you got it to pass, you must be right.
sun/security/provider/MoreDrbgParameters.java
=============================================
48: if null, does a default get selected?
Yes, the one in the security property. I should also mentioned when passed into
HashDrbg/HmacDrbg/CtrDrbg it is ignored.
In the others, you say "a default XXX will be used", but it's missing here.
com/sun/crypto/provider/SunJCE.java
===================================
I wasn't able to find any HmacSHA512/* OIDs at oid-info.com.
http://oid-info.com/get/1.2.840.113549.2
Just the ones for MessageDigest, which you have in SunEntries.
Neither can I find in other places. I even asked on Stack Overflow
http://stackoverflow.com/questions/33605411/oids-for-hmacsha512-224-and-hmacsha512-256
I saw your question when I was looking, but didn't realize that was you! ;)
I hope this helps some more.
Brad
