Please find comments in line.
On 5/18/2018 4:29 PM, Bradford Wetmore wrote:
Mostly minor, but a couple substantive comments.
I skimmed the tests this time, but didn't hit them as hard.
On 5/14/2018 1:20 PM, Valerie Peng wrote:
Hi Brad,
The latest webrev is at
http://cr.openjdk.java.net/~valeriep/8146293/webrev.04/
The only difference between webrev.03 and webrev.04 is the review
comments from the CSR.
SignedObject.java
-----------------
Now that SignedObject is no longer in the scope of the CSR, we talked
about updating the class javadoc with an example about setting the
parameters before passing the Signature object in. I didn't see that,
so did you want to at least give an example of it here? I don't
expect that would require a separate CSR.
Well, I debated about this and feel that it's probably better to leave
this for later once we are set about the recommended usage for
SignedObject.
Signature.java
--------------
Copyright year update.
Fixed.
PSSParameterSpec.java
---------------------
Nit: add vertical space between description and params. 98/99, 104/105.
125: Here you do need a period, since it's the end of a sentence and
you need to separate the two sentence. Periods after both sentences.
162/164: If you're going to fix some of the other spots in this file,
you might also add spaces here.
Ok.
RSAPrivateCrtKeySpec.java
-------------------------
Out of curiosity, here and in a couple of other places, what prompted
you to completely reword these constructor sentences? It's always
bothered me, and I noticed it was done between webrev.01 and .02. It
was approved in the CSR, so yay!
You commented about it. So I made the changes.
RSAPublicKeySpec.java
RSAPrivateKeySpec.java
----------------------
95: Shouldn't you move the null statement to the @return line
instead of the method summary? e.g.
* Returns the parameters associated with this key.
*
* @return the parameters associated with this key, or null if not
present?
Existing javadoc seems to be using either. I prefer to keep it as is.
javax/crypto/spec/package-info.java
-----------------------------------
Copyright year update.
Fixed.
SignerInfo.java
PKCS10.java
---------------
This is what we talked about earlier. In X509CRL.java,
X509CRLImpl.java, X509Certificate.java, X509CertImpl.java, you are
setting the parameters after the init calls, but here you are doing
them before. Probably should be consistent.
Correct. Good catch.
RSAUtil.java
------------
44: Extra ","
Fixed.
RSAPadding.java
---------------
106/109: Make final?
Ok.
RSASignature.java
-----------------
281. Indent 4 more spaces.
Fixed.
MGF1.java
---------
31. If you wouldn't mind adding RFC here?
Ok.
RSAPSSSignature.java
--------------------
191: Would you mind inserting a comment that you "skip the JCA
overhead"?
Ok.
264: -> PSSParameterSpec.TRAILER_FIELD_BC instead of hardcoding 1?
418: Thanks for adding all of the "stepX" comments. That really
helps readability! I'm assuming no material changes were made here?
I looked at the impl code heavily on the last review, but not as much
this time.
No, only comments are added.
TestOAEPWithParams.java
-----------------------
50: Should we also add SHA-384, SHA-512 here?
I am not so sure as the key size is only 768. We can bump the size up
and add SHA-384, SHA512, but since other tests in the same directory
covers SHA-382 and SHA-512, I only added SHA-512/224 and SHA-512/256 to
this test.
Offsets.java
------------
43: Should we also add all of the missing RSA variants as well?
SHA{1,224,256...}withRSA
Ok, I added more but left SHA1 out as it's sunsetting and existing coverage.
Will re-run mach5 again. Webrev updated at:
http://cr.openjdk.java.net/~valeriep/8146293/webrev.05/
Thanks,
Valerie
Thanks,
Brad
