without sounding like a broken record(!), can I ask that new exceptions
provide better information (where possible) ?
com/sun/crypto/provider/DHKeyPairGenerator.java
+ throw new InvalidParameterException(
+ "Keysize must be multiple of 64, and can only range " +
+ "from 512 to 8192 (inclusive)");
This is a new checkKeySize method - we should print what keysize was
attempted in the exception message! Granted - the code was borrowed from
old code but hopefully you can improve while here.
likewise in com/sun/crypto/provider/DHParameterGenerator.java (print the
keysize in InvalidParameterException)
Can we improve this exception message while here also ?
+ if ((exponentSize <= 0) || (exponentSize >= primeSize)) {
+ throw new InvalidAlgorithmParameterException(
+ "Exponent size must be positive and less than modulus size");
+ } catch (Exception ex) {
+ throw new ProviderException("Unexpected exception: " + ex);
Could we use the two arg constructor for ProviderException above and
pass ex as Throwable ?
===
sun/security/provider/DSAParameterGenerator.java
+ ("Prime size should be 512 - 1024, or 2048, 3072");
again, could we print the strength variable in above exception ?
===
sun/security/ssl/ServerHandshaker.java
throw new IllegalArgumentException(
- "Customized DH key size should be positive integer " +
- "between 1024 and 2048 bits, inclusive");
+ "Customized DH key size should not be less " +
+ "than 1024 bits");
Can we print customizedDHKeySize value ?
===
sun/security/pkcs11/P11KeyPairGenerator.java
Let's get the keySize printed in all InvalidAlgorithmParameterException
calls that you're editing.
Thanks,
Sean.
On 14/04/2016 12:12, Xuelei Fan wrote:
Thanks for the comments. Here is the new webrev:
http://cr.openjdk.java.net/~xuelei/8072452/webrev.03/
On 4/14/2016 7:25 AM, Valerie Peng wrote:
Here are some comments for the test changes:
<test/sun/security/pkcs11/KeyPairGenerator/TestDH2048.java>
- Not testing key size 768?
- line 82, would be useful to indicate version here, e.g. NSS (as of
version xxx) has a hard coded ...
- line 27, 4096 should be 8192?
Updated.
<test/sun/security/provider/DSA/TestAlgParameterGenerator.java>
- format changes only? No test for (3072, 256)?
I removed the update of this file.
<test/com/sun/crypto/provider/KeyAgreement/UnsupportedDHKeys.java>
- line 50, dhp8192(8191) looks like a typo?
- line 66, do we really need to generate the key pair here?
- line 71, seem redundant and potentially confusing as it's aligned
differently comparing to line70?
Updated.
<test/sun/security/pkcs11/KeyAgreement/SupportedDHKeys.java>
- line 63, why test for KeyAgreement here instead of KeyPairGenerator?
Better to use KeyPairGenerator. Updated.
<test/sun/security/pkcs11/KeyAgreement/UnsupportedDHKeys.java>
- line 53, dhp8192(8191) looks like a typo?
- line 64, why test for KeyAgreement here instead of KeyPairGenerator?
- line 75, do we really need to generate the key pair here?
- line 80, seem redundant and potentially confusing as it's aligned
differently comparing to line79?
Updated.
<test/sun/security/provider/DSA/SupportedDSAParaGen.java>
- Use "Param" instead of "Para" in the test name "SupportedDSAParaGen"?
Hm, nice name.
- Just curious, have u tried the specified timeout value across
different platforms?
Not really, I only tested on JPRT and the solaris-sparc desktop. This
test may be intermittent timeout failure in some circumstances. I added
the intermittent key tag.
- The file is new but the copyright year starts from 2012?
Updated.
As for the updated sources, all changes look fine. But while I was
looking at
src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java, I
have some more questions:
- line 145, do we need to check for upper limit here? It used to have
2048, should it be 8192 now? If yes, then line 1526 need to be enhanced
too.
I was wondering to remove the limit in case third party's implementation
support more size. Not necessary. Updated and limited to 8192 bits.
- line 1551, now that more DH sizes are supported, shouldn't we make
some changes here? Now it's either 1024 or 2048. The older impl on line
1550 uses 1024 as the lower limit and 2048 as the upper limit and will
keep the value as is if it's in-between the limits. If there is a reason
to keep 2048 as the upper limit, it'd be useful to add some comment here
for clarification.
This is mainly for compatibility and interoperability impact. Some old
deployed application may not support DH key size bigger than 2048. I
added back the comments.
Thanks,
Xuelei
Thanks,
Valerie
On 4/13/2016 7:23 AM, Xuelei Fan wrote:
Hi Valerie,
All comments are good to me and accepted in the new webrev:
http://cr.openjdk.java.net/~xuelei/8072452/webrev.02/
On 4/13/2016 8:26 AM, Valerie Peng wrote:
Hi Xuelei,
Mostly look good, just some comments in line...
<src/java.base/share/classes/com/sun/crypto/provider/DHKeyPairGenerator.java>
line 95-100, so essentially, we will use the built-in parameters as much
as possible and if none available, we will try to generate the
parameters for key sizes<= 1024, right? The current comment is a little
hard to parse. Maybe we can just explain it as below?
// Use the built-in parameters (ranging from 512 to 8192) when
available
this.params = ParameterCache.getCachedDHParameterSpec(keysize);
// Due to performance issue, only support DH parameters generation
// up to 1024 bits
if ((this.params == null)&& (keysize> 1024)) {
Updated.
<src/java.base/share/classes/sun/security/provider/DSAParameterGenerator.java>
- line 212, redundant?
Yes. Updated.
<src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java>
- line 277-303, the "params"argument for this particular method is
always null for non-RSA algorithms based on the usage. It is commented
on line 232 and several other places as indicated by the "// XXX sanity
check params" comments. So, more code changes will be needed here.
Either you can add another boolean hasParams and rely on that argument,
or change the specified argument type from RSAKeyGenParameterSpec to a
generic one and then cast it later when it is needed.
Good catch! Updated per the latter proposal.
<src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java>
- line 141 - 146 and line 1548 - 1554 are identical. Seems to me that
they can be customized a little as they are for different variable
assignment. The "customizedDHKeySize" variable on line 147 is from user,
right? Maybe what you really mean is something like the following?
// DH parameter generation can be extremely slow, best to use
one of
the
// supported pre-computed DH parameters (see DHCrypt class)
As for line 1548 - 1554, it can be as simple as:
// DH parameter generation can be extremely slow, make sure to use
one of the
// supported pre-computed DH parameters (see DHCrypt class)
Updated.
- line 148, can customizedDHKeySize be 1024? The old code accepts it but
the new code will error out. Is there a reason for rejecting 1024? The
check only rejects "<1024" but yet the exception message says the value
should be larger than 1024.
Lastly, "should larger" => "should be larger" (line 150).
1024 bits should not be rejected. Updated the exception message.
Thanks for the comments!
Xuelei
I will look at the tests next and send u comments separately.
Thanks,
Valerie
On 4/1/2016 8:53 AM, Xuelei Fan wrote:
Hi,
Please review this improvement update to support DHE sizes up to
8192-bits and DSA sizes up to 3072-bits:
http://cr.openjdk.java.net/~xuelei/8072452/webrev.00
This updated extends to support 3072-bits DH and DSA parameters
generation, and pre-computed DH parameters up to 8192 bits and
pre-computed DSA parameters up to 3072-bits.
Thanks,
Xuelei
