RFR[11] JDK-8146293 "Add Support for RSA-PSS Signature Algorithm as in PKCS#1 v2.2"
Bradford Wetmore
bradford.wetmore at oracle.com
Tue May 1 01:07:13 UTC 2018
I have not reviewed the tests, but the following is my deep-dive on the
code.
Mostly nits. I stopped making comments about javadoc style issues ("."
at ends of fragments, indentation problems), except where you were
making changes anyway. I realize the existing code has a bunch of
problems that we shouldn't gratuitously break.
>> 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);
>> 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.
> Thanks for checking on BouncyCastle, given the default is stated in the
> class javadoc to be 20 and the norm is to use byte as the unit, I feel
> the chance of other vendors using bits are very low. We can remind other
> vendors about this typo, but we should fix this regardless.
Ok.
>> RSAPrivateCrtKeySpec.java
>> -------------------------
>> 60/88: Do you want to add v1.2?
>>
> I think you mean v2.2. Actually, I prefer to only mention the version
> information in class javadoc. Easier to maintain this way. So, I
> actually removed the version info from the method javadoc of other
> classes for consistency.
Good plan. Thanks, and Ok.
java.base/.../java/security/interfaces/package-info.java
--------------------------------------------------------
Suggest mentioning RFC 8017, as "November 2016" doesn't make it easy to
find this particular version. The RSA version came out in October 27,
2012. Whether you want to add the hyperlinking is up to you, I notice
you've added in other places (e.g. RSAPrivateCrtKeySpec.java)
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.
152-209: The code in the other constructor is 99% the same, consider
moving this setup code to a common method.
173: Exactly the same wording here as the previous constructor. Should
we endeavor to make them different?
182/195/307/339-340: indention problems.
223: If you're going to fix this style problem, please fix the
indention problem also.
java.base/.../java/security/spec/package-info.java
--------------------------------------------------
Suggest at least mentioning RFC 8017.
java.base/.../java/security/spec/RSAKeyGenParameterSpec.java
------------------------------------------------------------
60:
public-exponent value with no key parameters
->
public-exponent value, and no key parameters
71:
public-exponent value and key parameters
->
public-exponent value, and key parameters
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.
* @param otherPrimeInfo triplets of the rest of primes, null can be
* specified if there are only two prime factors (p and q)
->
* @param otherPrimeInfo triplets of the rest of primes, null can be
* specified if there are only two prime factors (p
* and q)
java.base/.../javax/crypto/Cipher.java
--------------------------------------
If we go with RSA-PSS or RSASSA-PSS, do you still need this change?
java.base/.../javax/crypto/spec/OAEPParameterSpec.java
------------------------------------------------------
77-80 should be <pre>. It's rendering as a paragraph.
java.base/.../javax/crypto/spec/package-info.java
-------------------------------------------------
Suggest at least mentioning RFC 8017.
java.base/.../sun/security/rsa/PSSParameters.java
-------------------------------------------------
66/89/154/163/176/227/236: add @overrides?
115: switch should not indent the arms:
switch ... {
case "...": ... ;
default: ... ;
}
130: Shouldn't SHA-512/224 and /256 be here?
java.base/.../sun/security/rsa/RSAKeyFactory.java
-------------------------------------------------
202-208: This code is repeated 5 times, maybe a single check method?
206 and other places:
Expect an
->
Expected a
or
Expected a/an
java.base/.../sun/security/rsa/RSAPrivateKeyImpl.java
-----------------------------------------------------
Add @Overrides like you did in the previous?
java.base/.../sun/security/rsa/RSAPSSSignature.java
---------------------------------------------------
53:
@since 10
->
@since 11
309-313: Should this still be here?
75/190/231/406/443/479/497/and more...: Line > 80 chars
358/and more: Add @Overrides?
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.
java.base/.../sun/security/util/SignatureUtil.java
--------------------------------------------------
47/60-61/etc: Parameter continuation should be indented 8 spaces here.
Since this is a new file, can you please clean this up for readability? i.e.
public static void specialSetParameter(Signature sig,
AlgorithmParameters params)
throws InvalidAlgorithmParameterException,
ProviderException {
...deleted...
java.base/.../sun/security/x509/X509CertImpl.java
-------------------------------------------------
590-593: Parameter continuation should be indented 8 spaces here. This
whole file is a mess! :)
jdk.crypto.cryptoki/.../sun/security/pkcs11/P11Signature.java
-------------------------------------------------------------
@842: @Overrides?
jdk.crypto.mscapi/.../sun/security/mscapi/RSASignature.java
-----------------------------------------------------------
474: Extra *
491-492: Extra lines.
Brad
>> 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
>>
> I removed the <digest>withRSA-PSS aliasses and am considering removing
> the <digest>withRSAandMGF1 impls. The RSA-PSS (or RSASSA-PSS) scheme in
> PKCS#1 v2.2 passes in the digest as part of signature parameters
> (required) at runtime. Also, the oid corresponds to RSA-PSS unlike in
> PKCS#1 v1.5 where oid is defined for each digest with RSA signature
> scheme. So, I am having second thought on supporting the
> <digest>withRSAandMGF1 names. The RSA-PSS signature impl code can use
> less checks if we don't support these "friendly" names.
> As for the standard name, I didn't want to use RSA/PSS as the Cipher
> transformation string uses "/" in its syntax. As for RSASSA-PSS, it is
> also a little different from what we normally use. I don't have a strong
> preference on names though. I can change it to whatever the groups'
> preference is.
>
> Thanks,
> Valerie
>>
>>
>> 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
>
More information about the security-dev
mailing list