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

[Valerie Peng]

Webrev has been updated with review comments from Sean and Brad:
http://cr.openjdk.java.net/~valeriep/8146293/webrev.03/

Ran through Mach5, things look good.
Thanks,
Valerie

On 5/3/2018 3:55 PM, Bradford Wetmore wrote:
>
>> I have incorporated your comments into the webrev but I am still not 
>> done with it yet due to name switch from RSA-PSS to RSASSA-PSS.
>> Will send out another email once webrev is updated.
>
> Thanks, looking forward to it.
>
>> On 4/30/2018 6:07 PM, Bradford Wetmore wrote:
>>>>> PSSParameterSpec.java
>>>>> ---------------------
>>>>> Maybe add trailerFieldBC(1)?
>>>> Not sure what do u want me to add. A constants for TrailerFieldBC, 
>>>> or else?
>>>
>>> Yes that was my thought.  A constant field for TrailerFieldBC. When 
>>> I was trying to construct a PSSParameterSpec, the integer value to 
>>> use is "1", but if you don't read ASN.1 well enough, one might be 
>>> tempted to pass "0xbc" instead.  If you weren't using DEFAULT, the 
>>> call would be like this:
>>>
>>>         PSSParameterSpec pssSpec = new PSSParameterSpec(
>>>                 "SHA-256", "MGF1", MGF1ParameterSpec.SHA256, 20, 1);
>>> ->
>>>         PSSParameterSpec pssSpec = new PSSParameterSpec(
>>>                 "SHA-256", "MGF1", MGF1ParameterSpec.SHA256, 20,
>>>                 PSSParameterSpec.trailerBC);
>>>
>> I added TRAILER_FIELD_BC constant for this as constant names are 
>> generally all upper case. Not too pretty and would welcome suggestions.
>
> Egads, that was a DWIMNWIS[1] move.  Yes, needs to be cap'd of course. :)
>
> [1] https://www.urbandictionary.com/define.php?term=dwimnwis
>
> Thanks for considering.
>
>>> java.base/.../java/security/SignedObject.java
>>> ---------------------------------------------
>>> 59:  In the javadoc, it looks like there is a missing opening brace 
>>> that isn't closed with the one at line 64.  However, the one at line 
>>> 64 is not showing up in the generated output either. Weird.
>> I think the {@ on line 59 pairs up with the } on line 64. Everything 
>> in between is shown as code.
>
> Oops, that is the closing brace for the @code on line 57.
>
> One more minor nit?  Can you add braces around the so.verify "if" arm:
>
>  if (so.verify(verificationKey, verificationParams, verificationEngine))
> ...deleted...
> ->
>  if (so.verify(verificationKey, verificationParams, 
> verificationEngine)) {
>      try {
>          Object myobj = so.getObject();
>      } catch (java.lang.ClassNotFoundException e) {};
>      try {
>          Object myobj = so.getObject();
>      } catch (java.lang.ClassNotFoundException e) {};
> }
>
>>> 173: Exactly the same wording here as the previous constructor. 
>>> Should we endeavor to make them different?
>> The wordings look different to me. The former one explicitly states 
>> that no signature parameters is used.
>
> In your other new constructors (e.g. 
> RSAPrivateKeySpec.java/RSAPublicKeySpec.java), your new opening 
> sentence called out the difference, but this one didn't.  Just 
> wondering if you wanted to make it consistent.  I'm ok either way.
>
>>> java.base/.../java/security/spec/RSAMultiPrimePrivateCrtKeySpec.java
>>> --------------------------------------------------------------------
>>> A general nit throughout your APIs, when you're doing multi-line 
>>> things for your @params/@return/@exception, usually should be 
>>> indented to help readability of the source.
>> Alright, I will try to add indentation to improve readability to the 
>> part that I touched.
>
> Thanks.  It bothers me just enough to notice it, but understand why we 
> don't want to go through our javadocs just for this kind of change. 
> Kinda wish it was on the technical debt list, but plenty of other 
> things to do!  :)
>
>>> java.base/.../javax/crypto/Cipher.java
>>> --------------------------------------
>>> If we go with RSA-PSS or RSASSA-PSS, do you still need this change?
>> Yes, this change is for supporting OAEPwith<digest>andMGF1Paddong now 
>> that we added support for SHA512/224 and SHA512/256.
>
> Oh, of course.  Thanks.
>
>>> 448-: I appreciated the comments.  When people come up with their 
>>> own optimizations of a relatively straightforward algorithm, it can 
>>> be confusing to follow.  I'm guessing you're not going to be 
>>> checking for input limitations?  (i.e. step 1)  A few more comments 
>>> to help map the RFC to your code would be good, especially in the 
>>> verify/decode. e.g. 536-558.
>> I will add some more comments on this.
>
> Thanks.
>
> Brad
>