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

[Bradford Wetmore]

> 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