RFR[11] JDK-8146293 "Add Support for RSA-PSS Signature Algorithm as in PKCS#1 v2.2"

[Bradford Wetmore]

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.


Signature.java
--------------
Copyright year update.


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.


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!


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?


javax/crypto/spec/package-info.java
-----------------------------------
Copyright year update.


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.


RSAUtil.java
------------
44:  Extra ","


RSAPadding.java
---------------
106/109:  Make final?


RSASignature.java
-----------------
281.  Indent 4 more spaces.


MGF1.java
---------
31.  If you wouldn't mind adding RFC here?


RSAPSSSignature.java
--------------------
191:  Would you mind inserting a comment that you "skip the JCA overhead"?

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.


TestOAEPWithParams.java
-----------------------
50:  Should we also add SHA-384, SHA-512 here?


Offsets.java
------------
43:  Should we also add all of the missing RSA variants as well? 
SHA{1,224,256...}withRSA


Thanks,

Brad