Code Review Request 8072452 Support DHE sizes up to 8192-bits

Thu Apr 14 22:45:41 UTC 2016

I have no more comments.
Thanks,
Valerie

On 4/14/2016 4:12 AM, 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