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

[Valerie Peng]

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