Re: RFR[11] JDK-8146293 "Add Support for RSA-PSS Signature Algorithm as in PKCS#1 v2.2"
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 OAEPwithandMGF1Paddong now
that we added support for SHA512/224 and SHA512/256.
java.base/.../javax/crypto/spec/OAEPParameterSpec.java
--
77-80 should be . 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
Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
Update: forget my comment on this finding, Sean. I'm already wrapping
IOE in IAPE when IOE gets thrown so it's better to go the route you
suggested. I didn't read the method carefully enough.
375 if (dv.tag == DerValue.tag_OctetString) {
376 // Get the nonce value
377 newNonce = dv.getOctetString();
Similar to a previous comment, I would just call
DerValue.getOctetString() directly and let it throw an Exception if
it is the wrong tag.
JN: Except that I'll still have to catch the IOException and make that
the cause of the InvalidAlgorithmParameterException. I don't think
CipherSpi.engineInit(int, Key, AlgorithmParameters, SecureRandom) can
directly throw IOException. If I have to wrap it inside IAPE, do you
still prefer to do it that way or is the existing way OK?
--Jamil
Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
Hi Sean, many thanks for taking a detailed look at the code. My
comments are in-line
On 5/1/2018 1:53 PM, Sean Mullan wrote:
On 5/1/18 1:20 PM, Sean Mullan wrote:
On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff
mostly:
* Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
complete encryption or decryption (similar language to what exists
already for AES-GCM encryption).
* Added an additional test case for ChaCha20NoReuse
* Made accessor methods for ChaCha20ParameterSpec final and
cleaned up
the code a bit based on comments from the field.
http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/
More comments:
- Poly1305.java
44 * @since 10
Not necessary, but should be 11 if so.
JN: Will fix
46 public final class Poly1305
Can this be package-private?
JN: Not sure. I will make it so and run through the tests. It will
probably work.
84 keyBytes.length);
122 BLOCK_LENGTH - blockOffset);
indent at least 4 more spaces
JJN: Will fix.
- ChaCha20Poly1305ParamTest.java
When you test for exceptions, you should throw an exception if the
method does not throw an exception. For example:
171 try {
172 apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
173 } catch (IllegalArgumentException iae) {
174 System.out.println("Caught expected exception: " + iae);
175 }
should be something like:
try {
apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
throw new Exception("init passed unexpectedly");
} catch (IllegalArgumentException iae) {
System.out.println("Caught expected exception: " + iae);
}
there are several other cases like this.
JN: Yikes! I normally do put an exception in the happy-path when I'm
expecting a failure. Major oversight here. I will fix this.
- ChaCha20KAT.java
looks good.
- ChaCha20NoReuse.java
looks good.
- ChaCha20Cipher.java
Exception messages say "bits" - should we use "bytes" to be consistent
with the API?
JN: Sure, I can do that.
262 counter = 1;
Here you set the counter field to 1, but this method can still throw
exceptions after that. Is there any risk of leaving the object in an
inconsistent state by setting counter=1 if this method fails to
complete successfully? Same comment on line 305.
JN: I don't think so, since any Cipher.init call that would drop into
these init routines expects the counter to be set to 1 (or to whatever
value came in from the AlgorithmParameterSpec). If an exception is
thrown during the init codepath, the object will remain in an
uninitialized state and cannot be used, so I don't see much danger
there. The only thing the user could do with an object in this
situation would be to try to initialize it again (hopefully with
parameters that don't cause it to fail).
301 if (newNonce.length != 12) {
302 throw new InvalidAlgorithmParameterException(
303 "ChaCha20 nonce must be 96 bits in length");
304 }
You don't need to check for this, since the ChaCha20ParamSpec class
does already (especially if you make it final).
JN: Yep, you're right. A throwback to an earlier rev of the code before
the ChaCha20ParameterSpec rigidly enforced this. I can remove it.
375 if (dv.tag == DerValue.tag_OctetString) {
376 // Get the nonce value
377 newNonce = dv.getOctetString();
Similar to a previous comment, I would just call
DerValue.getOctetString() directly and let it throw an Exception if it
is the wrong tag.
JN: Except that I'll still have to catch the IOException and make that
the cause of the InvalidAlgorithmParameterException. I don't think
CipherSpi.engineInit(int, Key, AlgorithmParameters, SecureRandom) can
directly throw IOException. If I have to wrap it inside IAPE, do you
still prefer to do it that way or is the existing way OK?
395 throw new UnsupportedOperationException("Invalid
mode: " +
Seems inconsistent with 321 which throws RuntimeExc.
JN: So it is. I'll change the UOE to RuntimeException.
401 if (newNonce == null) {
402 newNonce = new byte[12];
403 if (random != null) {
404 random.nextBytes(newNonce);
405 }
It looks like there is a possibility of a non-random nonce being used
if random is null at this point, which it could be if null is passed
as the random and params parameters. I don't think that is what is
intended. I think the right thing to do is throw an
InvalidAlgParamException if you get to this point and random is null.
JN: Well, the method comment is old/outdated, another throwback to an
older version that said it would use all zeroes in the
Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
Comments in-line:
On 5/1/2018 10:20 AM, Sean Mullan wrote:
On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff
mostly:
* Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
complete encryption or decryption (similar language to what exists
already for AES-GCM encryption).
* Added an additional test case for ChaCha20NoReuse
* Made accessor methods for ChaCha20ParameterSpec final and cleaned up
the code a bit based on comments from the field.
http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/
Some comments so far, will continue my review later ...
- Cipher.java
After the paragraph, I would add a reference for more info, ex:
"Please see http://www.ietf.org/rfc/rfc7539.txt;> RFC 7539
for more information on the ChaCha20 and ChaCha20-Poly1305
algorithms."
JN: Sounds good. I'll do that.
- SunJCE.java
81 "(implements RSA, DES, Triple DES, AES, Blowfish, ARCFOUR, RC2,
PBE, "
Should add the ChaCha20 algorithm to the provider information String
above.
JN: Yep, missed that. Will fix.
- ChaCha20ParameterSpec.java
Thinking more about this -- if you don't think there is any good
reason that a provider would want to subclass this, I would just make
the whole class final.
JN: Given what ChaCha20 takes as variable inputs, I cannot see what
anyone could gain from subclassing it. I can set it to final and update
the CSR.
60 * @throws IllegalArgumentException if {@code nonce} is not
12 bytes
61 * in length.
Remove period since this is a fragment that is not followed by a
sentence.
87 * @return the counter value.
Same as above.
JN: Will fix.
- ChaCha20Poly1305Parameters.java
52 * @since 11
We don't typically put @since lines in impl. classes.
JN: Consider it gone.
111 "ChaCha20-Poly1305 Parameter ASN.1 encoding
error");
102 if (val.tag == DerValue.tag_OctetString) {
103 // Get the nonce value
104 nonce = val.getOctetString();
I would probably just call val.getOctetString() directly and not
proactively check the tag, since it will throw IOE if it is the wrong
tag anyway. Then you don't need these lines:
109 } else {
110 throw new IOException(
111 "ChaCha20-Poly1305 Parameter ASN.1 encoding
error");
112 }
There are other reasons an IOException can be thrown, so it doesn't
seem necessary to have a special-case for one type of error.
JN: Sure, that seems reasonable. Will fix.
134 throw new IllegalArgumentException(
135 "Unsupported parameter format: " +
decodingMethod);
Although this seems like a reasonable exception to throw,
AlgorithmParametersSpi.engineInit is not spec'ed to throw this, so I
think you should throw an IOE instead.
202 throw new IllegalArgumentException(
203 "Unsupported encoding format: " +
encodingMethod);
Similar comment, AlgorithmParametersSpi.engineGetEncoded is not
spec'ed to throw this, so I think you should throw an IOE instead.
JN: Fair enough. I had figured IAE would be OK since it's an unchecked
exception and seemed more appropriate to the situation. But IOE is fine
too. Will fix.
--Jamil
Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
On 5/1/18 1:20 PM, Sean Mullan wrote:
On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff
mostly:
* Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
complete encryption or decryption (similar language to what exists
already for AES-GCM encryption).
* Added an additional test case for ChaCha20NoReuse
* Made accessor methods for ChaCha20ParameterSpec final and cleaned up
the code a bit based on comments from the field.
http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/
More comments:
- Poly1305.java
44 * @since 10
Not necessary, but should be 11 if so.
46 public final class Poly1305
Can this be package-private?
84 keyBytes.length);
122 BLOCK_LENGTH - blockOffset);
indent at least 4 more spaces
- ChaCha20Poly1305ParamTest.java
When you test for exceptions, you should throw an exception if the
method does not throw an exception. For example:
171 try {
172 apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
173 } catch (IllegalArgumentException iae) {
174 System.out.println("Caught expected exception: " + iae);
175 }
should be something like:
try {
apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
throw new Exception("init passed unexpectedly");
} catch (IllegalArgumentException iae) {
System.out.println("Caught expected exception: " + iae);
}
there are several other cases like this.
- ChaCha20KAT.java
looks good.
- ChaCha20NoReuse.java
looks good.
- ChaCha20Cipher.java
Exception messages say "bits" - should we use "bytes" to be consistent
with the API?
262 counter = 1;
Here you set the counter field to 1, but this method can still throw
exceptions after that. Is there any risk of leaving the object in an
inconsistent state by setting counter=1 if this method fails to complete
successfully? Same comment on line 305.
301 if (newNonce.length != 12) {
302 throw new InvalidAlgorithmParameterException(
303 "ChaCha20 nonce must be 96 bits in length");
304 }
You don't need to check for this, since the ChaCha20ParamSpec class does
already (especially if you make it final).
375 if (dv.tag == DerValue.tag_OctetString) {
376 // Get the nonce value
377 newNonce = dv.getOctetString();
Similar to a previous comment, I would just call
DerValue.getOctetString() directly and let it throw an Exception if it
is the wrong tag.
395 throw new UnsupportedOperationException("Invalid
mode: " +
Seems inconsistent with 321 which throws RuntimeExc.
401 if (newNonce == null) {
402 newNonce = new byte[12];
403 if (random != null) {
404 random.nextBytes(newNonce);
405 }
It looks like there is a possibility of a non-random nonce being used if
random is null at this point, which it could be if null is passed as the
random and params parameters. I don't think that is what is intended. I
think the right thing to do is throw an InvalidAlgParamException if you
get to this point and random is null.
508 this.keyBytes = newKeyBytes;
509 nonce = newNonce;
510
511 aadDone = false;
512 direction = opmode;
Here also you are setting various fields but the method may still throw
an Exception - any issues about leaving the object in an inconsistent
state? Preferably you should set these as the last thing you do before
returning.
--Sean
Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff mostly:
* Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
complete encryption or decryption (similar language to what exists
already for AES-GCM encryption).
* Added an additional test case for ChaCha20NoReuse
* Made accessor methods for ChaCha20ParameterSpec final and cleaned up
the code a bit based on comments from the field.
http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/
Some comments so far, will continue my review later ...
- Cipher.java
After the paragraph, I would add a reference for more info, ex: "Please
see http://www.ietf.org/rfc/rfc7539.txt;> RFC 7539 for
more information on the ChaCha20 and ChaCha20-Poly1305 algorithms."
- SunJCE.java
81 "(implements RSA, DES, Triple DES, AES, Blowfish, ARCFOUR, RC2,
PBE, "
Should add the ChaCha20 algorithm to the provider information String above.
- ChaCha20ParameterSpec.java
Thinking more about this -- if you don't think there is any good reason
that a provider would want to subclass this, I would just make the whole
class final.
60 * @throws IllegalArgumentException if {@code nonce} is not 12
bytes
61 * in length.
Remove period since this is a fragment that is not followed by a sentence.
87 * @return the counter value.
Same as above.
- ChaCha20Poly1305Parameters.java
52 * @since 11
We don't typically put @since lines in impl. classes.
111 "ChaCha20-Poly1305 Parameter ASN.1 encoding error");
102 if (val.tag == DerValue.tag_OctetString) {
103 // Get the nonce value
104 nonce = val.getOctetString();
I would probably just call val.getOctetString() directly and not
proactively check the tag, since it will throw IOE if it is the wrong
tag anyway. Then you don't need these lines:
109 } else {
110 throw new IOException(
111 "ChaCha20-Poly1305 Parameter ASN.1 encoding
error");
112 }
There are other reasons an IOException can be thrown, so it doesn't seem
necessary to have a special-case for one type of error.
134 throw new IllegalArgumentException(
135 "Unsupported parameter format: " +
decodingMethod);
Although this seems like a reasonable exception to throw,
AlgorithmParametersSpi.engineInit is not spec'ed to throw this, so I
think you should throw an IOE instead.
202 throw new IllegalArgumentException(
203 "Unsupported encoding format: " + encodingMethod);
Similar comment, AlgorithmParametersSpi.engineGetEncoded is not spec'ed
to throw this, so I think you should throw an IOE instead.
--Sean
JEP Proposal: EdDSA Signatures
The work for X25519/X448 is wrapping up[1], so I have started thinking about EdDSA. Please review the draft JEP[2] for EdDSA, and let me know what you think. I'm not really sure what priority to give the EdDSA work, and it is optional in TLS 1.3. So if you have a particular need or desire for EdDSA in the JDK, please let me know so I can figure what priority to give this task. Also let me know which variants of EdDSA you want---TLS only uses the "pure" variant, and I'm trying to decide whether it it is worthwhile to also implement the other variants. Of course, I'm also interested in technical feedback on the JEP. A lot of the goals and design are similar to the X25519/X448 JEP. [1] http://openjdk.java.net/jeps/324 [2] http://openjdk.java.net/jeps/8199231
Re: [8u-dev] 8193171 : keytool -list displays "JKS" for a PKCS12 keystore
Looks fine to me. regards, Sean. On 30/04/2018 20:16, Ivan Gerasimov wrote: Hello! This is 8u-dev only bug fix. It was noticed that the keytool from the latest JDK 8 update release displays type of PKCS12 keystore as JKS. Would you please help review the patch? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8193171 WEBREV: http://cr.openjdk.java.net/~igerasim/8193171/00/webrev/ Thanks in advance!
RFR 8202086 : Improve performance characteristics of sun.security.util.MemoryCache
Hello! This enhancement was generously contributed by Peter Levart [1]. The original webrev is found here [2]. The goal was to improve concurrent accessibility of the cache, while maintaining some additional limitations dictated by the spec. Would you please help review this fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202086 WEBREV: http://cr.openjdk.java.net/~igerasim/8202086/00/webrev/ Thanks in advance! [1] http://mail.openjdk.java.net/pipermail/security-dev/2017-November/016512.html [2] http://cr.openjdk.java.net/~plevart/jdk10-dev/8186628_ssl_session_cache_scalability/webrev.01/ -- With kind regards, Ivan Gerasimov
