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

Fri Apr 13 19:25:06 UTC 2018

My biggest comment below are the algorithm names you've chosen.  BC and 
Android use a different naming convention (details below), and I don't 
think folks will be able to drop in those alternate crypto impls and use 
our TLS impl without some modifications of one/other code.

Mostly everything else is typos.

Unfortunately I'm on break next week, so I got as far as I could.

I got all of the non-sun/security/rsa code and it looks ok.

I got through some of sun/security/rsa code.  I did about 4 of these 
thoroughly, and took some brief looks in other files.

I didn't hit the tests unfortunately.

As a general note, throughout your code I'd suggest netbeans to discover 
stylistic issues, there were lots of little suggestions that could help. 
  I didn't have time to flag all of them, but here's a representative 
sample in the first file listed.  e.g.

PSSParameters.java
------------------
61:  Make final?

@Overrides throughout?

BigInteger is actually needed?  (unused import)

101:  Extra ; here.

122:  You can use a switch on strings here.

240:  You could tighten the code a bit with:

     sb.append("....")
         .append("...")
         .append("..."

RSACipher.java
--------------
using both PKCS#1 v1.5 and OAEP paddings
->
using both PKCS#1 v1.5 and OAEP (v2.2) paddings

MGF1ParameterSpec.java
----------------------
49:  Add an extra line?
74/79:  Nit: maybe add some vertical space here for these other elements?

PSSParameterSpec.java
---------------------
Maybe add trailerFieldBC(1)?

Similar vertical space comments?

157/202:  We talked about this in person, but I wanted to mention here 
for a wider audience.  I had concerns about this typo, and any interop 
problems this might bring.  I looked over the Bouncy Castle impl, and it 
appears as though they also assumed it to be bytes, not bits.  Can you 
check with the other vendors who might have their own PSS 
implementations and verify this is not going to be a problem?  I talked 
with our CSR lead (Joe Darcy), he doesn't think it should be a problem 
if other impls are using bytes.

213:  Usually @param/@return fragments don't end with a .  But would 
mean updating the rest of the file.  Keep (if you so decided) or change.

RSAKeyGenParameterSpec.java
---------------------------
I see/understand what you're doing underneath, but the following reads a 
little awkward without the context.  Maybe:

public-exponent value and null key parameters
->
public-exponent value and no key parameters

RSAMultiPrimePrivateCrtKeySpec.java
-----------------------------------
80-87/126-132:  Any chance of reformatting the @exception text?  Very 
strange to read.

107:  If you really want to list out every input for the constructors, 
you should probably mention the AlgorithmParameterSpec here, otherwise 
the two methods are exactly the same.

63/107:  You seem to be doing it in other classes, so suggest you also 
update
v2.1
->
v2.2

RSAPrivateCrtKeySpec.java
-------------------------
60/88:  Do you want to add v1.2?

88:  Same comment about the missing AlgorithmParameterSpec.

RSAPrivateKeySpec.java
----------------------
62:  "...with additional algorithm parameters."  ?

RSAPublicKeySpec.java
---------------------
98:  fragment.

OAEPParameterSpec.java
----------------------
Do you want to split the lines on this one as well, like the other two? 
Or make them all one line each?

RSAPrivateCrtKeyImpl.java
-------------------------
must be null
->
Must be null

178/183/188/193/198/203/208/213/219/223/229:  Do you want to add in the 
@overrides for this class instead of "// see JCE doc"?

230:  This is the "SunRsaSign" provider, but yo are using "Sun" here.

SunRsaSignEntries.java
----------------------
145:  Where did you come up with this convention for your aliases?

     SHA1withRSA-PSS

I see Bouncy Castle[1] and Android[2] are both using:

     SHA*withRSA/PSS
     RSASSA-PSS (name from PKCS#1)

[1] 
https://github.com/bcgit/bc-java/blob/master/prov/src/main/java/org/bouncycastle/jcajce/provider/asymmetric/RSA.java
[2] https://developer.android.com/reference/java/security/Signature.html

but we have neither style.

MGF1.java
---------
36:
* @since   10
->
* @since   11

Thanks!

Brad

On 3/27/2018 6:40 PM, Valerie Peng wrote:
> Hi Brad,
> 
> Can you please help review the changes for RSA-PSS support? I also added 
> some minor enhancement which add 2 more digest algorithms for OAEP padding.
> There are quite some changes involved. The main changes are in the 
> SunRsaSign provider, i.e. sun.security.rsa packages. I reused existing 
> RSAKeyFactory, RSAKeyPairGenerator, and the RSA KeyImpl classes as much 
> as possible. However, given that RSA-PSS signatures requires parameters, 
> I put its implementation in a separate class, i.e. RSAPSSSignature.java.
> 
> RFE: https://bugs.openjdk.java.net/browse/JDK-8146293
> Webrev: http://cr.openjdk.java.net/~valeriep/8146293/webrev.00/
> 
> Existing and new regression tests have been run and result looks fine.
> Thanks,
> Valerie