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
