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
