RFR[11] JDK-8146293 "Add Support for RSA-PSS Signature Algorithm as in PKCS#1 v2.2"
Valerie Peng
valerie.peng at oracle.com
Wed May 2 02:13:01 UTC 2018
Hi Brad,
Thanks for the review. Please find comments in line. I adopted most if
not all of your comments.
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.
On 4/30/2018 6:07 PM, Bradford Wetmore wrote:
>
> 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);
>
I added TRAILER_FIELD_BC constant for this as constant names are
generally all upper case. Not too pretty and would welcome suggestions.
> 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)
>
Updated. I didn't include any link here as that seems to be the
convention for package-info.java. Probably not that important comparing
to the public classes like RSA*Key.
>
> 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.
> 152-209: The code in the other constructor is 99% the same, consider
> moving this setup code to a common method.
Done.
> 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.
> 182/195/307/339-340: indention problems.
>
> 223: If you're going to fix this style problem, please fix the
> indention problem also.
All Fixed.
> java.base/.../java/security/spec/package-info.java
> --------------------------------------------------
> Suggest at least mentioning RFC 8017.
Done.
> 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
Done.
> 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.
>
> * @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?
Yes, this change is for supporting OAEPwith<digest>andMGF1Paddong now
that we added support for SHA512/224 and SHA512/256.
>
> java.base/.../javax/crypto/spec/OAEPParameterSpec.java
> ------------------------------------------------------
> 77-80 should be <pre>. It's rendering as a paragraph.
Fixed.
> java.base/.../javax/crypto/spec/package-info.java
> -------------------------------------------------
> Suggest at least mentioning RFC 8017.
Done.
>
> 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: ... ;
> }
Fixed.
>
> 130: Shouldn't SHA-512/224 and /256 be here?
Good catch. Added.
>
> java.base/.../sun/security/rsa/RSAKeyFactory.java
> -------------------------------------------------
> 202-208: This code is repeated 5 times, maybe a single check method?
Sure, done.
> 206 and other places:
> Expect an
> ->
> Expected a
> or
> Expected a/an
Ok.
>
> java.base/.../sun/security/rsa/RSAPrivateKeyImpl.java
> -----------------------------------------------------
> Add @Overrides like you did in the previous?
Done.
>
> java.base/.../sun/security/rsa/RSAPSSSignature.java
> ---------------------------------------------------
> 53:
> @since 10
> ->
> @since 11
Fixed.
> 309-313: Should this still be here?
Removed.
>
> 75/190/231/406/443/479/497/and more...: Line > 80 chars
Fixed.
> 358/and more: Add @Overrides?
Done.
> 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.
>
> 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...
Done.
>
> java.base/.../sun/security/x509/X509CertImpl.java
> -------------------------------------------------
> 590-593: Parameter continuation should be indented 8 spaces here.
> This whole file is a mess! :)
Fixed. I only updated my changes. It'd make the webrev un-readable if I
address all the indentation inconsistency.
>
>
> jdk.crypto.cryptoki/.../sun/security/pkcs11/P11Signature.java
> -------------------------------------------------------------
> @842: @Overrides?
Added.
>
>
> jdk.crypto.mscapi/.../sun/security/mscapi/RSASignature.java
> -----------------------------------------------------------
> 474: Extra *
>
> 491-492: Extra lines.
Removed.
Valerie
>
> 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