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.
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:

using both PKCS#1 v1.5 and OAEP paddings
using both PKCS#1 v1.5 and OAEP (v2.2) paddings
49:  Add an extra line?
74/79:  Nit: maybe add some vertical space here for these other elements?
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.
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
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
60/88:  Do you want to add v1.2?

88:  Same comment about the missing AlgorithmParameterSpec.
62:  "...with additional algorithm parameters."  ?
98:  fragment.
Do you want to split the lines on this one as well, like the other two? Or make them all one line each?
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.
145:  Where did you come up with this convention for your aliases?


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

    RSASSA-PSS (name from PKCS#1)


but we have neither style.
* @since   10
* @since   11



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. 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.


Existing and new regression tests have been run and result looks fine.

Reply via email to